CommandLine Page Cycler

CommandLine Page Cycler


Usage:
$ <chrome> --visit-urls=<url_txt_file> [--record-stats=<stats_txt_file>] --visit-urls-count=<repeat_count (int)> [--user-data-dir=<user_data_dir>] [--record-mode | --playback-mode]

This will iterate through the list of URLs, gathering usage statistics. It is meant for use with the PageCycler RecordAPI and UI, as well as commandline use.

TBR=aa@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=140188

[Aaron Boodman, 2012-03-20 19:28:16]

lgtm

I did not do a full review because I've already looked at this. If you think
there are particular areas I should pay careful attention to, please let me
know.

https://chromiumcodereview.appspot.com/9667009/diff/7002/chrome/browser/chrom...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/9667009/diff/7002/chrome/browser/chrom...
chrome/browser/chrome_browser_main.cc:1394: unsigned int iterations = 0;
We don't use unsigned integers in Chrome, except for size_t, and only then when
interacting with the standard library.

https://chromiumcodereview.appspot.com/9667009/diff/7002/chrome/browser/chrom...
chrome/browser/chrome_browser_main.cc:1398: page_cycler->Run(iterations);
Nit: define interations outside the check for kVisitURLsCount and default it to
1, and then only have one call to page_cycler->Run().

https://chromiumcodereview.appspot.com/9667009/diff/7002/chrome/browser/page_...
File chrome/browser/page_cycler/page_cycler.cc (right):

https://chromiumcodereview.appspot.com/9667009/diff/7002/chrome/browser/page_...
chrome/browser/page_cycler/page_cycler.cc:71: if (is_main_frame &&
validated_url.spec() != chrome::kUnreachableWebDataURL)
Man, that is a bummer. It would be easy for code elsewhere in Chrome to change
and for this to stop working. I don't have a suggestion for improvement though.

https://chromiumcodereview.appspot.com/9667009/diff/7002/chrome/browser/page_...
File chrome/browser/page_cycler/page_cycler.h (right):

https://chromiumcodereview.appspot.com/9667009/diff/7002/chrome/browser/page_...
chrome/browser/page_cycler/page_cycler.h:35: FilePath errors_file,
Consider just making these setters that the caller should specify before calling
Run().

https://chromiumcodereview.appspot.com/9667009/diff/7002/chrome/browser/page_...
chrome/browser/page_cycler/page_cycler.h:38: // Begin running the page cycler;
post the ReadURLsFromFile task on the IO
Avoid duplicating implementation (such as post the blahblah task to blah thread)
details in comments.

Comments should be about what happens, not how.

[chebert, 2012-04-09 23:52:01]

https://chromiumcodereview.appspot.com/9667009/diff/7002/chrome/browser/page_...
File chrome/browser/page_cycler/page_cycler.h (right):

https://chromiumcodereview.appspot.com/9667009/diff/7002/chrome/browser/page_...
chrome/browser/page_cycler/page_cycler.h:55: const std::vector<GURL>* urls() {
return &urls_; }
I've seen functions that call testing functions with a for_test() at the end. So
like:

urls_for_test()

This pretty much guarantees that no one will use it, especially if they are
scanning through comments quickly.

[Devlin, 2012-05-23 03:17:30]

Here's the latest of PageCycler.  It should be passing all tests on all
platforms (with the exception of the typical flakiness).  It runs on Mac,
Windows, and Linux.  We would appreciate a quick once-over to make sure we
didn't change anything negatively since the last time this was reviewed.

Thanks in advance.

[Aaron Boodman, 2012-05-29 21:39:08]

https://chromiumcodereview.appspot.com/9667009/diff/28003/chrome/browser/page...
File chrome/browser/page_cycler/page_cycler_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9667009/diff/28003/chrome/browser/page...
chrome/browser/page_cycler/page_cycler_browsertest.cc:59: void
InitPageCycler(FilePath urls_file,
Insert blank line here.

https://chromiumcodereview.appspot.com/9667009/diff/28003/chrome/browser/page...
chrome/browser/page_cycler/page_cycler_browsertest.cc:132: PageCycler*
page_cycler_;
Please create a constructor so that this can be initialized to NULL.

https://chromiumcodereview.appspot.com/9667009/diff/28003/chrome/browser/page...
chrome/browser/page_cycler/page_cycler_browsertest.cc:164: dest_data_dir);
This should line up with the first param.

https://chromiumcodereview.appspot.com/9667009/diff/28003/chrome/browser/page...
chrome/browser/page_cycler/page_cycler_browsertest.cc:415: // NOTE: This test
operates by comparing the size of a generated cache
This is probably going to be too brittle. Also I wonder if it's consistent
across platforms. I can't think of any good way to test this ... I think you can
just skip it.

https://chromiumcodereview.appspot.com/9667009/diff/28003/chrome/browser/page...
File chrome/browser/page_cycler/page_cycler_unittest.cc (right):

https://chromiumcodereview.appspot.com/9667009/diff/28003/chrome/browser/page...
chrome/browser/page_cycler/page_cycler_unittest.cc:40: if (file_a.size() !=
file_b.size())
I believe that you can just use std::equal?

https://chromiumcodereview.appspot.com/9667009/diff/28003/chrome/browser/page...
chrome/browser/page_cycler/page_cycler_unittest.cc:56: if
(file_contents[file_contents.size() - 1] == '\n') {
No curly brace is required here.

https://chromiumcodereview.appspot.com/9667009/diff/28003/chrome/browser/page...
chrome/browser/page_cycler/page_cycler_unittest.cc:57: file_contents =
file_contents.substr(0, file_contents.size() - 1);
file_contents.resize()

https://chromiumcodereview.appspot.com/9667009/diff/28003/chrome/browser/page...
chrome/browser/page_cycler/page_cycler_unittest.cc:265:
ASSERT_TRUE(LinesAreEqual(errors_output, errors_expected));
Why not just compare the file contents as strings?

https://chromiumcodereview.appspot.com/9667009/diff/28003/chrome/browser/page...
chrome/browser/page_cycler/page_cycler_unittest.cc:266:
file_util::Delete(errors_output_file_path(), false);
Did you consider using ScopedTempDir here too?

[Aaron Boodman, 2012-05-29 23:22:12]

LGTM ...

[Aaron Boodman, 2012-05-30 01:08:42]

https://chromiumcodereview.appspot.com/9667009/diff/28003/chrome/browser/page...
File chrome/browser/page_cycler/page_cycler_unittest.cc (right):

https://chromiumcodereview.appspot.com/9667009/diff/28003/chrome/browser/page...
chrome/browser/page_cycler/page_cycler_unittest.cc:40: if (file_a.size() !=
file_b.size())
On 2012/05/29 21:39:08, Aaron Boodman wrote:
> I believe that you can just use std::equal?

In case this wasn't clear, I meant you can just use std::equals on the
std::vector instances.

But now that I look at it, you can also just do std::vector::operator==. Whee.

[Devlin, 2012-05-30 01:45:15]

Latest with all requested fixes.

http://codereview.chromium.org/9667009/diff/28003/chrome/browser/page_cycler/...
File chrome/browser/page_cycler/page_cycler_browsertest.cc (right):

http://codereview.chromium.org/9667009/diff/28003/chrome/browser/page_cycler/...
chrome/browser/page_cycler/page_cycler_browsertest.cc:59: void
InitPageCycler(FilePath urls_file,
On 2012/05/29 21:39:08, Aaron Boodman wrote:
> Insert blank line here.

Done.

http://codereview.chromium.org/9667009/diff/28003/chrome/browser/page_cycler/...
chrome/browser/page_cycler/page_cycler_browsertest.cc:132: PageCycler*
page_cycler_;
On 2012/05/29 21:39:08, Aaron Boodman wrote:
> Please create a constructor so that this can be initialized to NULL.

Done.

http://codereview.chromium.org/9667009/diff/28003/chrome/browser/page_cycler/...
chrome/browser/page_cycler/page_cycler_browsertest.cc:164: dest_data_dir);
On 2012/05/29 21:39:08, Aaron Boodman wrote:
> This should line up with the first param.

Done.

http://codereview.chromium.org/9667009/diff/28003/chrome/browser/page_cycler/...
chrome/browser/page_cycler/page_cycler_browsertest.cc:415: // NOTE: This test
operates by comparing the size of a generated cache
Skipped for now; will look into a better way of doing this for a future CL.

On 2012/05/29 21:39:08, Aaron Boodman wrote:
> This is probably going to be too brittle. Also I wonder if it's consistent
> across platforms. I can't think of any good way to test this ... I think you
can
> just skip it.

http://codereview.chromium.org/9667009/diff/28003/chrome/browser/page_cycler/...
File chrome/browser/page_cycler/page_cycler_unittest.cc (right):

http://codereview.chromium.org/9667009/diff/28003/chrome/browser/page_cycler/...
chrome/browser/page_cycler/page_cycler_unittest.cc:40: if (file_a.size() !=
file_b.size())
On 2012/05/29 21:39:08, Aaron Boodman wrote:
> I believe that you can just use std::equal?

Done.

http://codereview.chromium.org/9667009/diff/28003/chrome/browser/page_cycler/...
chrome/browser/page_cycler/page_cycler_unittest.cc:56: if
(file_contents[file_contents.size() - 1] == '\n') {
On 2012/05/29 21:39:08, Aaron Boodman wrote:
> No curly brace is required here.

Done.

http://codereview.chromium.org/9667009/diff/28003/chrome/browser/page_cycler/...
chrome/browser/page_cycler/page_cycler_unittest.cc:57: file_contents =
file_contents.substr(0, file_contents.size() - 1);
On 2012/05/29 21:39:08, Aaron Boodman wrote:
> file_contents.resize()

Done.

http://codereview.chromium.org/9667009/diff/28003/chrome/browser/page_cycler/...
chrome/browser/page_cycler/page_cycler_unittest.cc:265:
ASSERT_TRUE(LinesAreEqual(errors_output, errors_expected));
On 2012/05/29 21:39:08, Aaron Boodman wrote:
> Why not just compare the file contents as strings?

Done.

http://codereview.chromium.org/9667009/diff/28003/chrome/browser/page_cycler/...
chrome/browser/page_cycler/page_cycler_unittest.cc:266:
file_util::Delete(errors_output_file_path(), false);
On 2012/05/29 21:39:08, Aaron Boodman wrote:
> Did you consider using ScopedTempDir here too?

Done.

[Aaron Boodman, 2012-05-30 01:50:38]

LG stands

[commit-bot: I haz the power, 2012-05-30 02:25:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/9667009/31003

[commit-bot: I haz the power, 2012-05-30 02:25:51]

Can't process patch for file
chrome/test/data/page_cycler/cached_data_dir/Default/Web Data-journal.
Unexpected git diff output name Data-journal
b/chrome/test/data/page_cycler/cached_data_dir/Default/Web Data-journal.

[commit-bot: I haz the power, 2012-05-31 03:12:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/9667009/37003

[commit-bot: I haz the power, 2012-05-31 03:12:56]

Presubmit check for 9667009-37003 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:
    chrome/browser/ui
    chrome/browser/profiles

Presubmit checks took 1.8s to calculate.

[commit-bot: I haz the power, 2012-05-31 05:16:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/9667009/32004

[commit-bot: I haz the power, 2012-05-31 06:06:19]

Try job failure for 9667009-32004 (retry) on mac_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-05-31 18:35:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/9667009/32004

[commit-bot: I haz the power, 2012-05-31 19:26:23]

Try job failure for 9667009-32004 (retry) on mac_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Devlin, 2012-06-02 00:44:55]

Checked to make sure; this test is passing locally on Mac, both on debug and
release builds.  Continuing to try...

On 2012/05/31 19:26:23, I haz the power (commit-bot) wrote:
> Try job failure for 9667009-32004 (retry) on mac_rel for step "unit_tests".
> It's a second try, previously, step "unit_tests" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-06-02 00:45:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/9667009/32004

[commit-bot: I haz the power, 2012-06-02 01:49:35]

Try job failure for 9667009-32004 (retry) (retry) on mac_rel for step
"unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Aaron Boodman, 2012-06-02 08:46:25]

I tried landing this for you, but it failed in the same way on the real bots:
http://build.chromium.org/p/chromium/builders/Mac10.6%20Tests%20%282%29/build...

There can sometimes be interactions between tests. When you tested this locally,
did you run the entire test suite or just the failing test? You should run the
entire suite. That may make it reproducible.

[Devlin, 2012-06-03 00:11:36]

On 2012/06/02 08:46:25, Aaron Boodman wrote:
> I tried landing this for you, but it failed in the same way on the real bots:
>
http://build.chromium.org/p/chromium/builders/Mac10.6%2520Tests%2520%25282%25...
> 
> There can sometimes be interactions between tests. When you tested this
locally,
> did you run the entire test suite or just the failing test? You should run the
> entire suite. That may make it reproducible.

You were right about the test interactions - the way we were closing the browser
was messing it up.  It's fixed now, and has been passing bots again (except
Windows, which had the error
"Cannot open include file: 'content/test/accessibility_test_utils_win.h': No
such file or directory" - pretty sure that one's not us, and it was passing
windows before.  I put in another run at the trybots to check, but the issue
might not be fixed yet).

Unfortunately, the issue's closed now - any way it can be reopened to try
committing for the nth time?

Again, thanks for all your help, and sorry this is being so difficult to get...

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

Once a code review is closed, you need to create a new one to revisit it. You
can clear the review association from your git branch with `git cl issue 0`.

I've re-uploaded this to: https://chromiumcodereview.appspot.com/10491010 and
relanded it for you. Wish us luck!