Record API fix adding FirstRun, and allowing repeat-count in capture (plus some browsertests along …

Record API fix adding FirstRun, and allowing repeat-count in capture (plus some browsertests along for the ride).


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140904

[Aaron Boodman, 2012-05-16 23:37:00]

http://codereview.chromium.org/10386134/diff/5001/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.h (right):

http://codereview.chromium.org/10386134/diff/5001/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.h:82: virtual void RunPageCycler();
Does this need to be virtual?

http://codereview.chromium.org/10386134/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_record_api.cc (right):

http://codereview.chromium.org/10386134/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_record_api.cc:103: if
(!file_util::PathExists(user_data_dir_)) {
You can use the --no-first-run command line switch to suppress that behavior.

http://codereview.chromium.org/10386134/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_record_api.cc:178:
base::Int64ToString(repeat_count_));
params should all line up.

http://codereview.chromium.org/10386134/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_record_api.h (right):

http://codereview.chromium.org/10386134/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_record_api.h:93: int repeat_count_;
Why would you want to repeat the capture phase?

http://codereview.chromium.org/10386134/diff/5001/chrome/browser/page_cycler/...
File chrome/browser/page_cycler/page_cycler.cc (right):

http://codereview.chromium.org/10386134/diff/5001/chrome/browser/page_cycler/...
chrome/browser/page_cycler/page_cycler.cc:5: #include
"chrome/browser/page_cycler/page_cycler.h"
Not reviewing this because I think I already reviewed it for devlin...?

http://codereview.chromium.org/10386134/diff/5001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10386134/diff/5001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:4012: 'test/base/test_switches.cc',
What do you use from test_switches in the chrome/browser? Same question
regarding chrome_process_util? (I see the chrome_process_util include in
page_cycler, but it's not clear what is being used).

[clintstaley, 2012-05-31 01:54:30]

http://codereview.chromium.org/10386134/diff/5001/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.h (right):

http://codereview.chromium.org/10386134/diff/5001/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.h:82: virtual void RunPageCycler();
On 2012/05/16 23:37:00, Aaron Boodman wrote:
> Does this need to be virtual?

I usually make large methods virtual since the polymorphic overhead is minimal
in C++, and it's more flexible that way.  Is this not wanted, tho?

http://codereview.chromium.org/10386134/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_record_api.cc (right):

http://codereview.chromium.org/10386134/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_record_api.cc:103: if
(!file_util::PathExists(user_data_dir_)) {
On 2012/05/16 23:37:00, Aaron Boodman wrote:
> You can use the --no-first-run command line switch to suppress that behavior.

Now he tells me :).  Sorry -- I should have done better research.

http://codereview.chromium.org/10386134/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_record_api.cc:178:
base::Int64ToString(repeat_count_));
On 2012/05/16 23:37:00, Aaron Boodman wrote:
> params should all line up.

Done.

http://codereview.chromium.org/10386134/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_record_api.h (right):

http://codereview.chromium.org/10386134/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_record_api.h:93: int repeat_count_;
On 2012/05/16 23:37:00, Aaron Boodman wrote:
> Why would you want to repeat the capture phase?

This was one of the necessary changes.  Apparently there is an incompatibility
with the captured cache if the repeat count differs between capture and replay. 
I've forgotten the details, but after some discussion we decided we had to put
the repeat count in the capture.

http://codereview.chromium.org/10386134/diff/5001/chrome/browser/page_cycler/...
File chrome/browser/page_cycler/page_cycler.cc (right):

http://codereview.chromium.org/10386134/diff/5001/chrome/browser/page_cycler/...
chrome/browser/page_cycler/page_cycler.cc:5: #include
"chrome/browser/page_cycler/page_cycler.h"
On 2012/05/16 23:37:00, Aaron Boodman wrote:
> Not reviewing this because I think I already reviewed it for devlin...?

Right.  Just a Git collision here.

http://codereview.chromium.org/10386134/diff/5001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10386134/diff/5001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:4012: 'test/base/test_switches.cc',
On 2012/05/16 23:37:00, Aaron Boodman wrote:
> What do you use from test_switches in the chrome/browser? Same question
> regarding chrome_process_util? (I see the chrome_process_util include in

GetRunningChromeProcesses uses the chrome_process_util
test_switches is used by chrome_process_util.

But, I'll change that duplicate .cc listing to a .h :)

> page_cycler, but it's not clear what is being used).

[clintstaley, 2012-06-04 21:36:16]

Aaron, here's a more-focused CL for record_api_fix.  There is still one block of
non-record-api code in chrome_browser_main that can be skipped in the review. 
All other h/cc files need review.

[Aaron Boodman, 2012-06-05 04:17:03]

http://codereview.chromium.org/10386134/diff/20001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_function_registry.cc (right):

http://codereview.chromium.org/10386134/diff/20001/chrome/browser/extensions/...
chrome/browser/extensions/extension_function_registry.cc:241:
RegisterFunction<CaptureURLsFunction>();
Please be consistent with the formatting of the other functions.

http://codereview.chromium.org/10386134/diff/20001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_record_api.cc (right):

http://codereview.chromium.org/10386134/diff/20001/chrome/browser/extensions/...
chrome/browser/extensions/extension_record_api.cc:105: /*
Kill dead code.

http://codereview.chromium.org/10386134/diff/20001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_record_api.h (right):

http://codereview.chromium.org/10386134/diff/20001/chrome/browser/extensions/...
chrome/browser/extensions/extension_record_api.h:93: int repeat_count_;
Why does capture need the concept of 'repeat count'?

[clintstaley, 2012-06-05 20:46:08]

Changes for last few comments from Aaron

http://codereview.chromium.org/10386134/diff/20001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_function_registry.cc (right):

http://codereview.chromium.org/10386134/diff/20001/chrome/browser/extensions/...
chrome/browser/extensions/extension_function_registry.cc:241:
RegisterFunction<CaptureURLsFunction>();
On 2012/06/05 04:17:04, Aaron Boodman wrote:
> Please be consistent with the formatting of the other functions.

Done.

http://codereview.chromium.org/10386134/diff/20001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_record_api.h (right):

http://codereview.chromium.org/10386134/diff/20001/chrome/browser/extensions/...
chrome/browser/extensions/extension_record_api.h:93: int repeat_count_;
On 2012/06/05 04:17:04, Aaron Boodman wrote:
> Why does capture need the concept of 'repeat count'?

Because of the vagaries of the record mode and playback mode features of Chrome.
 One cannot do a playback of multiple repetitions unless the record on which it
was based also had the same multiple repetitions.  (Devlin indicates you and he
discussed this a while back.).  Thus, the capture must set up the repeat count,
and the playback follows suit.

[Aaron Boodman, 2012-06-06 18:29:57]

lg

[Aaron Boodman, 2012-06-06 18:30:02]

lgtm

[commit-bot: I haz the power, 2012-06-06 18:58:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clintstaley@chromium.org/10386134/37001

[commit-bot: I haz the power, 2012-06-06 18:58:41]

Presubmit check for 10386134-37001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    base

Presubmit checks took 8.3s to calculate.

[commit-bot: I haz the power, 2012-06-06 19:12:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clintstaley@chromium.org/10386134/39001

[commit-bot: I haz the power, 2012-06-06 20:35:37]

Failed to apply patch for chrome/common/extensions/docs/samples.json:
While running patch -p1 --forward --force;
patching file chrome/common/extensions/docs/samples.json
Hunk #1 FAILED at 155.
Hunk #2 FAILED at 229.
Hunk #3 succeeded at 2139 (offset 29 lines).
2 out of 3 hunks FAILED -- saving rejects to file
chrome/common/extensions/docs/samples.json.rej

[commit-bot: I haz the power, 2012-06-06 22:56:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clintstaley@chromium.org/10386134/40018

[commit-bot: I haz the power, 2012-06-07 00:35:03]

Change committed as 140904