Measure loading time of several tabs.

Measure loading time of several tabs.
    
This is a new set of JSON automation calls which return the individual load times for each tab opened at startup.

BUG=44129
TEST=none

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

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

[Patrick Horn, 2010-06-03 03:15:45]

Hi Huan,

This is a rough draft of the test I'm working on. I'm curious about your
comments.

One interesting thing I noticed with my 16 thread machine is that the test ran
pretty fast even with 20 tabs. However, just to test, I added a call to
SetProcessorAffinity(1) to restrict it to a single core, and then I was able to
notice that loading 20 tabs was significantly slower.

Here are the initial results on my machine. I'm pasting the three ones I think
are interesting.

[==========] Running 5 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 5 tests from SessionRestoreUITest
This one restores 5 tabs, and waits for them all to load.
[ RUN      ] SessionRestoreUITest.AllFewTabsPerf
session-restore-few-all-any     763     ms
[       OK ] SessionRestoreUITest.AllFewTabsPerf (2193 ms)
This one restores 20 tabs, and waits for 4 to load:
[ RUN      ] SessionRestoreUITest.SomeManyTabsPerf
session-restore-many-any        2025    ms
session-restore-many-focused    2027    ms
[       OK ] SessionRestoreUITest.SomeManyTabsPerf (5387 ms)
This one loads 20 tabs but waits for the focused tab to load:
[ RUN      ] SessionRestoreUITest.FocusedManyTabsPerf
session-restore-many-focused-focused    555     ms
[       OK ] SessionRestoreUITest.FocusedManyTabsPerf (5014 ms)

As it turns out, the focused tab in all the examples loads significantly faster
than other tabs, so I suspect there is already logic for this case.

-Patrick

[Peter Kasting, 2010-06-03 03:37:14]

Adding sky

[Paweł Hajdan Jr., 2010-06-03 08:21:14]

Drive-by with some test comments.

http://codereview.chromium.org/2559001/diff/1/2
File chrome/browser/sessions/session_restore_uitest.cc (right):

http://codereview.chromium.org/2559001/diff/1/2#newcode86
chrome/browser/sessions/session_restore_uitest.cc:86: int num_wait_tabs=0);
nit: The style guide forbids default parameters. Also, a comment would be nice.

http://codereview.chromium.org/2559001/diff/1/2#newcode178
chrome/browser/sessions/session_restore_uitest.cc:178: scoped_refptr<TabProxy>
tab_proxy(browser_proxy->GetTab(num_tabs-1));
You should ASSERT_TRUE on the tab_proxy to avoid a crash of entire ui_tests in
case of failure.

http://codereview.chromium.org/2559001/diff/1/2#newcode189
chrome/browser/sessions/session_restore_uitest.cc:189:
ASSERT_TRUE(active_tab_index == num_tabs-1);
nit: This should be ASSERT_EQ to provide a better error message.

[sky, 2010-06-03 16:10:55]

http://codereview.chromium.org/2559001/diff/1/2
File chrome/browser/sessions/session_restore_uitest.cc (right):

http://codereview.chromium.org/2559001/diff/1/2#newcode84
chrome/browser/sessions/session_restore_uitest.cc:84: void
RunTabPerformanceTest(std::string testname,
const std::string&

http://codereview.chromium.org/2559001/diff/1/2#newcode137
chrome/browser/sessions/session_restore_uitest.cc:137: void
SessionRestoreUITest::RunTabPerformanceTest(
Move this before the tests.

http://codereview.chromium.org/2559001/diff/1/2#newcode139
chrome/browser/sessions/session_restore_uitest.cc:139: // assert (num_wait_tabs
> 0);
DCHECK_GT

http://codereview.chromium.org/2559001/diff/1/2#newcode148
chrome/browser/sessions/session_restore_uitest.cc:148:
automation()->GetBrowserWindow(0));
indent by 4.

http://codereview.chromium.org/2559001/diff/1/2#newcode152
chrome/browser/sessions/session_restore_uitest.cc:152:
browser_proxy->AppendTab(url2_.Resolve(std::string("?")+IntToString(i)));
space before/after the +

http://codereview.chromium.org/2559001/diff/1/2#newcode162
chrome/browser/sessions/session_restore_uitest.cc:162:
SetProcessAffinityMask(GetCurrentProcess(), 0x1);
Does this change the affinity of the ui test process, or chrome?

http://codereview.chromium.org/2559001/diff/1/2#newcode182
chrome/browser/sessions/session_restore_uitest.cc:182:
ASSERT_TRUE(tab_proxy->WaitForTabToBeRestored(action_max_timeout_ms()));
I'm not sure what you're trying to measure, but by the time you ask chrome for
the tab proxy chrome may have already restored the tab.

If you're trying to measure the time it takes to finish loading a tab it would
be better to have chrome track the times for you. session_restore could easily
track this information, but you only need it during automation so it would be
best to pass in some switch that some code to do the tracking.

http://codereview.chromium.org/2559001/diff/1/2#newcode185
chrome/browser/sessions/session_restore_uitest.cc:185: base::TimeTicks::Now() -
start_focused_tab_test;
indent by 4.

http://codereview.chromium.org/2559001/diff/1/3
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/2559001/diff/1/3#newcode266
chrome/test/ui/ui_test.cc:266: initial_loads_time_ = TimeTicks::Now();
Seems like chrome should be sending back times to you, otherwise they are never
going to be true (IPC overhead).

[huanr, 2010-06-03 21:47:02]

1. Make changes to comply the chrome code style:
http://dev.chromium.org/developers/coding-style

2. I agree with Scott's comments on time measurement. It is better to send back
the time from browser side. Originally I thought we could follow the current
browser start up test which only measures the time on test automation side. But
we have more variables to deal with in session restore case, and it is actually
better if we also fix the current browser start up test. That may reduce the
flakiness we observed on performance bot.

3. Scott just made some change that serializes tab loading. It will be
interesting to run the test before and after his change.

4. We may consider using more interesting pages later, like pages in
trunk/data/page_cycler.

5. It will be helpful to verify that we are still hitting disk cache code even
for file URLs.

If no one objects, I propose you fix all code style issues first and make
initial check in. Then you can work on a separate CL to address other issues. I
can apply for a provisional committer with initial check in so you can use try
service.

[Patrick Horn, 2010-06-03 22:01:35]

Hi all,
Comments inline.

On Thu, Jun 3, 2010 at 9:10 AM, <sky@chromium.org> wrote:

>
> http://codereview.chromium.org/2559001/diff/1/2
> File chrome/browser/sessions/session_restore_uitest.cc (right):
>
> http://codereview.chromium.org/2559001/diff/1/2#newcode84
> chrome/browser/sessions/session_restore_uitest.cc:84: void
> RunTabPerformanceTest(std::string testname,
> const std::string&
>
> http://codereview.chromium.org/2559001/diff/1/2#newcode137
> chrome/browser/sessions/session_restore_uitest.cc:137: void
> SessionRestoreUITest::RunTabPerformanceTest(
> Move this before the tests.
>
> http://codereview.chromium.org/2559001/diff/1/2#newcode139
> chrome/browser/sessions/session_restore_uitest.cc:139: // assert
> (num_wait_tabs > 0);
> DCHECK_GT
>
> http://codereview.chromium.org/2559001/diff/1/2#newcode148
> chrome/browser/sessions/session_restore_uitest.cc:148:
> automation()->GetBrowserWindow(0));
> indent by 4.
>
> http://codereview.chromium.org/2559001/diff/1/2#newcode152
> chrome/browser/sessions/session_restore_uitest.cc:152:
> browser_proxy->AppendTab(url2_.Resolve(std::string("?")+IntToString(i)));
> space before/after the +
>
> http://codereview.chromium.org/2559001/diff/1/2#newcode162
> chrome/browser/sessions/session_restore_uitest.cc:162:
> SetProcessAffinityMask(GetCurrentProcess(), 0x1);
> Does this change the affinity of the ui test process, or chrome?
>

It does change the affinity of the ui test process, but the affinity
propogates into child processes--hence why it also applies to all the
renderers. I then change it back after the new browser is launched.

I weighed this against changing base::LaunchApp in process_util_win.cc to
take in an affinity mask, since it's the only function with access to the
spawned HPROCESS. I didn't think it makes sense in general to have calls to
affinity inside the chrome browser process, since this is just for a test
case.


> http://codereview.chromium.org/2559001/diff/1/2#newcode182
> chrome/browser/sessions/session_restore_uitest.cc:182:
> ASSERT_TRUE(tab_proxy->WaitForTabToBeRestored(action_max_timeout_ms()));
> I'm not sure what you're trying to measure, but by the time you ask
> chrome for the tab proxy chrome may have already restored the tab.
>
> My intent was to basically take the time until LOAD_STOP was reached in
num_wait_tabs tabs, and also for the focused tab. I did change it to include
the time for the GetTab() call. The reason for the additional timestamp is
to avoid including the other IPC calls between WaitForInitialLoads and the
WaitForTabToBeRestored. That way if the focused tab is already loaded (as it
seems to load faster anyway), the difference between the two are a few ms.

If you're trying to measure the time it takes to finish loading a tab it
> would be better to have chrome track the times for you. session_restore
> could easily track this information, but you only need it during
> automation so it would be best to pass in some switch that some code to
> do the tracking.
>
>
I understand that it would be more accurate to measure this on the chrome
side, but from what I can tell the IPC overhead is on the order of
milliseconds, while the test itself takes half a second to a few seconds to
run. There's also IPC overhead from the renderer to the render host, so this
can't be much more.

It's a lot of work to add an additional command line flag and change the IPC
messages, and the test is mostly just meant to be a benchmark for future
improvements, so it doesn't need to be really accurate in my opinion.


> http://codereview.chromium.org/2559001/diff/1/2#newcode185
> chrome/browser/sessions/session_restore_uitest.cc:185:
> base::TimeTicks::Now() - start_focused_tab_test;
> indent by 4.
>
> http://codereview.chromium.org/2559001/diff/1/3
> File chrome/test/ui/ui_test.cc (right):
>
> http://codereview.chromium.org/2559001/diff/1/3#newcode266
> chrome/test/ui/ui_test.cc:266: initial_loads_time_ = TimeTicks::Now();
> Seems like chrome should be sending back times to you, otherwise they
> are never going to be true (IPC overhead).
>
>
If I were to do this, it wouldn't be a huge amount of code to change.
Basically, in browser_init.cc, CreateAutomationProvider can track when it
sends the "Hello" message in ConnectToChannel, then pass that in to
SetExpectedTabCount so that code can hold onto the timestamp. The annoying
part is the matter of adding a new IPC message to send this information to
the ui_tests.


>
> http://codereview.chromium.org/2559001/show
>

[sky, 2010-06-03 22:02:39]

On Thu, Jun 3, 2010 at 2:47 PM,  <huanr@chromium.org> wrote:
> 1. Make changes to comply the chrome code style:
> http://dev.chromium.org/developers/coding-style
>
> 2. I agree with Scott's comments on time measurement. It is better to send
> back
> the time from browser side. Originally I thought we could follow the current
> browser start up test which only measures the time on test automation side.
> But
> we have more variables to deal with in session restore case, and it is
> actually
> better if we also fix the current browser start up test. That may reduce the
> flakiness we observed on performance bot.
>
> 3. Scott just made some change that serializes tab loading. It will be
> interesting to run the test before and after his change.
>
> 4. We may consider using more interesting pages later, like pages in
> trunk/data/page_cycler.
>
> 5. It will be helpful to verify that we are still hitting disk cache code
> even
> for file URLs.
>
> If no one objects, I propose you fix all code style issues first and make
> initial check in. Then you can work on a separate CL to address other
> issues. I
> can apply for a provisional committer with initial check in so you can use
> try
> service.

It seems like quite a bit has to be changed before we're measuring
what we want. Additionally, won't the output have to be changed to
output something similar to our other perf tests so that it can be
displayed on a dashboard? I think we should wait before checking it
in.

  -Scott

[sky, 2010-06-03 22:06:42]

On Thu, Jun 3, 2010 at 3:01 PM, Patrick Horn <pathorn@chromium.org> wrote:
> Hi all,
> Comments inline.
>
> On Thu, Jun 3, 2010 at 9:10 AM, <sky@chromium.org> wrote:
>>
>> http://codereview.chromium.org/2559001/diff/1/2
>> File chrome/browser/sessions/session_restore_uitest.cc (right):
>>
>> http://codereview.chromium.org/2559001/diff/1/2#newcode84
>> chrome/browser/sessions/session_restore_uitest.cc:84: void
>> RunTabPerformanceTest(std::string testname,
>> const std::string&
>>
>> http://codereview.chromium.org/2559001/diff/1/2#newcode137
>> chrome/browser/sessions/session_restore_uitest.cc:137: void
>> SessionRestoreUITest::RunTabPerformanceTest(
>> Move this before the tests.
>>
>> http://codereview.chromium.org/2559001/diff/1/2#newcode139
>> chrome/browser/sessions/session_restore_uitest.cc:139: // assert
>> (num_wait_tabs > 0);
>> DCHECK_GT
>>
>> http://codereview.chromium.org/2559001/diff/1/2#newcode148
>> chrome/browser/sessions/session_restore_uitest.cc:148:
>> automation()->GetBrowserWindow(0));
>> indent by 4.
>>
>> http://codereview.chromium.org/2559001/diff/1/2#newcode152
>> chrome/browser/sessions/session_restore_uitest.cc:152:
>> browser_proxy->AppendTab(url2_.Resolve(std::string("?")+IntToString(i)));
>> space before/after the +
>>
>> http://codereview.chromium.org/2559001/diff/1/2#newcode162
>> chrome/browser/sessions/session_restore_uitest.cc:162:
>> SetProcessAffinityMask(GetCurrentProcess(), 0x1);
>> Does this change the affinity of the ui test process, or chrome?
>
> It does change the affinity of the ui test process, but the affinity
> propogates into child processes--hence why it also applies to all the
> renderers. I then change it back after the new browser is launched.
> I weighed this against changing base::LaunchApp in process_util_win.cc to
> take in an affinity mask, since it's the only function with access to the
> spawned HPROCESS. I didn't think it makes sense in general to have calls to
> affinity inside the chrome browser process, since this is just for a test
> case.
>>
>> http://codereview.chromium.org/2559001/diff/1/2#newcode182
>> chrome/browser/sessions/session_restore_uitest.cc:182:
>> ASSERT_TRUE(tab_proxy->WaitForTabToBeRestored(action_max_timeout_ms()));
>> I'm not sure what you're trying to measure, but by the time you ask
>> chrome for the tab proxy chrome may have already restored the tab.
>>
> My intent was to basically take the time until LOAD_STOP was reached in
> num_wait_tabs tabs, and also for the focused tab. I did change it to include
> the time for the GetTab() call. The reason for the additional timestamp is
> to avoid including the other IPC calls between WaitForInitialLoads and the
> WaitForTabToBeRestored. That way if the focused tab is already loaded (as it
> seems to load faster anyway), the difference between the two are a few ms.
>>
>> If you're trying to measure the time it takes to finish loading a tab it
>> would be better to have chrome track the times for you. session_restore
>> could easily track this information, but you only need it during
>> automation so it would be best to pass in some switch that some code to
>> do the tracking.
>>
>
> I understand that it would be more accurate to measure this on the chrome
> side, but from what I can tell the IPC overhead is on the order of
> milliseconds, while the test itself takes half a second to a few seconds to
> run. There's also IPC overhead from the renderer to the render host, so this
> can't be much more.
> It's a lot of work to add an additional command line flag and change the IPC
> messages, and the test is mostly just meant to be a benchmark for future
> improvements, so it doesn't need to be really accurate in my opinion.
>
>>
>> http://codereview.chromium.org/2559001/diff/1/2#newcode185
>> chrome/browser/sessions/session_restore_uitest.cc:185:
>> base::TimeTicks::Now() - start_focused_tab_test;
>> indent by 4.
>>
>> http://codereview.chromium.org/2559001/diff/1/3
>> File chrome/test/ui/ui_test.cc (right):
>>
>> http://codereview.chromium.org/2559001/diff/1/3#newcode266
>> chrome/test/ui/ui_test.cc:266: initial_loads_time_ = TimeTicks::Now();
>> Seems like chrome should be sending back times to you, otherwise they
>> are never going to be true (IPC overhead).
>
> If I were to do this, it wouldn't be a huge amount of code to change.
> Basically, in browser_init.cc, CreateAutomationProvider can track when it
> sends the "Hello" message in ConnectToChannel, then pass that in to
> SetExpectedTabCount so that code can hold onto the timestamp. The annoying
> part is the matter of adding a new IPC message to send this information to
> the ui_tests.
>
>>
>> http://codereview.chromium.org/2559001/show
>
>

If we're writing a benchmark we need to make sure there is as little
variance as possible. Additionally the delays you'll see will likely
vary depending upon the machine. The build bots usually are slower
machines, there may be more contention and so more overhead.

  -Scott

[Patrick Horn, 2010-06-03 23:04:38]

I forgot to click publish for my second patch. It mostly changes just the style
issues addressed in Paweł and sky's comments--I'd prefer to put the new IPC
messages into a separate code review.

-Patrick

[sky, 2010-06-03 23:36:29]

2010/06/03 23:04:38, Patrick Horn wrote:
> I forgot to click publish for my second patch. It mostly changes just the
style
> issues addressed in Paweł and sky's comments--I'd prefer to put the new IPC
> messages into a separate code review.

We should wait until it's at a point where it can be used for perf measuring. It
doesn't seem like it's there yet.

  -Scott

[Paweł Hajdan Jr., 2010-06-04 07:35:14]

http://codereview.chromium.org/2559001/diff/6001/7001
File chrome/browser/sessions/session_restore_uitest.cc (right):

http://codereview.chromium.org/2559001/diff/6001/7001#newcode84
chrome/browser/sessions/session_restore_uitest.cc:84: void
RunTabPerformanceTest(const std::string& testname,
Please add the comment. Especially what |num_wait_tabs| means.

http://codereview.chromium.org/2559001/diff/6001/7001#newcode117
chrome/browser/sessions/session_restore_uitest.cc:117: /* RunTabPerformanceTest
does a few things:
nit: This comment doesn't seem very useful, and we rather use the "//" style.

http://codereview.chromium.org/2559001/diff/6001/7001#newcode122
chrome/browser/sessions/session_restore_uitest.cc:122: // simulate a dual-core
hyperthreaded system.
nit: simulate -> Simulate.

http://codereview.chromium.org/2559001/diff/6001/7001#newcode126
chrome/browser/sessions/session_restore_uitest.cc:126: // otherwise window might
not be open in time.
nit: otherwise -> Otherwise. It would be nice to include the context in the
sentence. It's now not obvious whether it applies to the line above or the line
below.

[Patrick Horn, 2010-06-07 23:01:55]

This is a completely new patch set which measures times in InitialLoadObserver.

I also added a JSON message "GetInitialLoadTimes" to read these times, and
updated the session_restore_uitest to use it. This also allows us to measure
statistics about individual tabs, since it stores start and stop times for each
tab as a separate entry.

[Paweł Hajdan Jr., 2010-06-08 06:42:25]

http://codereview.chromium.org/2559001/diff/16001/17001
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2559001/diff/16001/17001#newcode2097
chrome/browser/automation/automation_provider.cc:2097: bool reply_return = true;
It seems that reply_return is always true. Could you just use "true" in place of
"reply_return" and remove the var declaration?

http://codereview.chromium.org/2559001/diff/16001/17001#newcode2099
chrome/browser/automation/automation_provider.cc:2099: // return_value owns
items.
nit: This comment seems to be unneeded. scoped_ptr itself means "I own it".

http://codereview.chromium.org/2559001/diff/16001/17003
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/2559001/diff/16001/17003#newcode41
chrome/browser/automation/automation_provider_observers.cc:41: init_time_ =
base::TimeTicks::Now();
nit: Could you do that instead in the initializer list?

http://codereview.chromium.org/2559001/diff/16001/17003#newcode81
chrome/browser/automation/automation_provider_observers.cc:81: DictionaryValue
*return_value = new DictionaryValue;
nit: should be DictionaryValue* return_value (note the star).

http://codereview.chromium.org/2559001/diff/16001/17004
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/2559001/diff/16001/17004#newcode39
chrome/browser/automation/automation_provider_observers.h:39: DictionaryValue
*GetTimingInformation() const;
nit: Should be DictionaryValue* GetTimingInformation (note the star).

http://codereview.chromium.org/2559001/diff/16001/17004#newcode42
chrome/browser/automation/automation_provider_observers.h:42: class TabTime {
Could you move the definition of TabTime to the .cc file, and only leave a

class TabTime;

line here?

http://codereview.chromium.org/2559001/diff/16001/17005
File chrome/browser/sessions/session_restore_uitest.cc (right):

http://codereview.chromium.org/2559001/diff/16001/17005#newcode126
chrome/browser/sessions/session_restore_uitest.cc:126: // simulate a dual-core
hyperthreaded system.
nit: simulate -> Simulate.

http://codereview.chromium.org/2559001/diff/16001/17005#newcode129
chrome/browser/sessions/session_restore_uitest.cc:129: DCHECK_GT(num_tabs, 0);
We don't want to crash the entire ui_tests. Could you rather use ASSERTs here?

http://codereview.chromium.org/2559001/diff/16001/17005#newcode173
chrome/browser/sessions/session_restore_uitest.cc:173: DictionaryValue
*values_dict;
nit: Move star to the left (applies to other places in this file too).

http://codereview.chromium.org/2559001/diff/16001/17005#newcode180
chrome/browser/sessions/session_restore_uitest.cc:180: //std::cout <<
json_response;
nit: You probably want to remove it.

http://codereview.chromium.org/2559001/diff/16001/17005#newcode208
chrome/browser/sessions/session_restore_uitest.cc:208: if (stop_ms >
max_stop_time) {
nit: Would it be cleaner to use std::max?

[sky, 2010-06-08 16:12:39]

http://codereview.chromium.org/2559001/diff/16001/17001
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2559001/diff/16001/17001#newcode2094
chrome/browser/automation/automation_provider.cc:2094: DictionaryValue* args,
It doesn't seem like args is used. Remove it if it's not needed.

http://codereview.chromium.org/2559001/diff/16001/17001#newcode2096
chrome/browser/automation/automation_provider.cc:2096: std::string json_return =
"{}";
Does json_return really need to be initialized? DOesn't Write do that for you?

http://codereview.chromium.org/2559001/diff/16001/17002
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/2559001/diff/16001/17002#newcode389
chrome/browser/automation/automation_provider.h:389: // Uses the JSON interface
for input/output.
Document the key/value pairs used in the dictionary some where.

http://codereview.chromium.org/2559001/diff/16001/17003
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/2559001/diff/16001/17003#newcode75
chrome/browser/automation/automation_provider_observers.cc:75: base::TimeDelta
delta_stop = it->second.stop_time() - init_time_;
It's possible the stop_time was was never set. Do you want to indicate this some
how?

http://codereview.chromium.org/2559001/diff/16001/17004
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/2559001/diff/16001/17004#newcode39
chrome/browser/automation/automation_provider_observers.h:39: DictionaryValue
*GetTimingInformation() const;
Document ownership returns to caller.

http://codereview.chromium.org/2559001/diff/16001/17004#newcode45
chrome/browser/automation/automation_provider_observers.h:45: :
load_start_time_(started) {
indent by 4

http://codereview.chromium.org/2559001/diff/16001/17004#newcode56
chrome/browser/automation/automation_provider_observers.h:56: private:
should match indentation of public

http://codereview.chromium.org/2559001/diff/16001/17005
File chrome/browser/sessions/session_restore_uitest.cc (right):

http://codereview.chromium.org/2559001/diff/16001/17005#newcode123
chrome/browser/sessions/session_restore_uitest.cc:123: void
SessionRestoreUITest::RunTabPerformanceTest(
This code needs to be part of the other performance tests, not session restore
ui test. You may want to refactor some of the code for better sharing.

[rvargas (doing something else), 2010-06-09 23:05:13]

http://codereview.chromium.org/2559001/diff/27001/28005
File chrome/browser/sessions/session_restore_uitest.cc (right):

http://codereview.chromium.org/2559001/diff/27001/28005#newcode128
chrome/browser/sessions/session_restore_uitest.cc:128: const unsigned int
kSessionRestoreAffinity = 0x000f;
Drive by: Isn't four CPUs a little high? If we are trying to test something
closer to what an average user has, two would be better.

[Patrick Horn, 2010-06-09 23:44:45]

I've removed the test for now, since I intend to merge it with the new test in
startup_test.cc after my cl 2753001 lands.

My plan is to move the call to "GetInitialLoadTimes" to be part of
RunStartupTest() so that it can theoretically be used for all startup perf
tests.  This also has the benefit of letting us take the average of several test
runs and having less duplicate code.

http://codereview.chromium.org/2559001/diff/16001/17001
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2559001/diff/16001/17001#newcode2094
chrome/browser/automation/automation_provider.cc:2094: DictionaryValue* args,
On 2010/06/08 16:12:39, sky wrote:
> It doesn't seem like args is used. Remove it if it's not needed.

I can't remove the arguments because it needs to be of type JsonHandler to be
used with SendJSONRequest. But I removed the names to avoid "unused variable"
warnings.

http://codereview.chromium.org/2559001/diff/16001/17001#newcode2096
chrome/browser/automation/automation_provider.cc:2096: std::string json_return =
"{}";
On 2010/06/08 16:12:39, sky wrote:
> Does json_return really need to be initialized? DOesn't Write do that for you?

Done.

http://codereview.chromium.org/2559001/diff/16001/17001#newcode2099
chrome/browser/automation/automation_provider.cc:2099: // return_value owns
items.
On 2010/06/08 06:42:25, Paweł Hajdan Jr. wrote:
> nit: This comment seems to be unneeded. scoped_ptr itself means "I own it".

Done.

http://codereview.chromium.org/2559001/diff/16001/17002
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/2559001/diff/16001/17002#newcode389
chrome/browser/automation/automation_provider.h:389: // Uses the JSON interface
for input/output.
On 2010/06/08 16:12:39, sky wrote:
> Document the key/value pairs used in the dictionary some where.

Done.

http://codereview.chromium.org/2559001/diff/16001/17003
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/2559001/diff/16001/17003#newcode41
chrome/browser/automation/automation_provider_observers.cc:41: init_time_ =
base::TimeTicks::Now();
On 2010/06/08 06:42:25, Paweł Hajdan Jr. wrote:
> nit: Could you do that instead in the initializer list?

Done.

http://codereview.chromium.org/2559001/diff/16001/17003#newcode75
chrome/browser/automation/automation_provider_observers.cc:75: base::TimeDelta
delta_stop = it->second.stop_time() - init_time_;
On 2010/06/08 16:12:39, sky wrote:
> It's possible the stop_time was was never set. Do you want to indicate this
some
> how?

Done.

http://codereview.chromium.org/2559001/diff/16001/17003#newcode81
chrome/browser/automation/automation_provider_observers.cc:81: DictionaryValue
*return_value = new DictionaryValue;
On 2010/06/08 06:42:25, Paweł Hajdan Jr. wrote:
> nit: should be DictionaryValue* return_value (note the star).

Done.

http://codereview.chromium.org/2559001/diff/16001/17004
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/2559001/diff/16001/17004#newcode39
chrome/browser/automation/automation_provider_observers.h:39: DictionaryValue
*GetTimingInformation() const;
On 2010/06/08 06:42:25, Paweł Hajdan Jr. wrote:
> nit: Should be DictionaryValue* GetTimingInformation (note the star).

Done.

http://codereview.chromium.org/2559001/diff/16001/17004#newcode39
chrome/browser/automation/automation_provider_observers.h:39: DictionaryValue
*GetTimingInformation() const;
On 2010/06/08 16:12:39, sky wrote:
> Document ownership returns to caller.

Done.

http://codereview.chromium.org/2559001/diff/16001/17004#newcode42
chrome/browser/automation/automation_provider_observers.h:42: class TabTime {
On 2010/06/08 06:42:25, Paweł Hajdan Jr. wrote:
> Could you move the definition of TabTime to the .cc file, and only leave a
> 
> class TabTime;
> 
> line here?

Done.

http://codereview.chromium.org/2559001/diff/16001/17004#newcode45
chrome/browser/automation/automation_provider_observers.h:45: :
load_start_time_(started) {
On 2010/06/08 16:12:39, sky wrote:
> indent by 4

Done.

http://codereview.chromium.org/2559001/diff/27001/28005
File chrome/browser/sessions/session_restore_uitest.cc (right):

http://codereview.chromium.org/2559001/diff/27001/28005#newcode128
chrome/browser/sessions/session_restore_uitest.cc:128: const unsigned int
kSessionRestoreAffinity = 0x000f;
On 2010/06/09 23:05:13, rvargas wrote:
> Drive by: Isn't four CPUs a little high? If we are trying to test something
> closer to what an average user has, two would be better.

I agree--My CPU happens to be hyperthreaded, so I was just testing with that,
but most multicore processors aren't. For my next update I'm going to remove
this from the patch since it doesn't have anything specific to do with this one
test.

I'm considering moving the affinity change function into part of process_util
(maybe with RAII logic to always restore the correct affinity), so that it would
be easier to test other ui tests with fewer processors.

[sky, 2010-06-10 15:38:40]

http://codereview.chromium.org/2559001/diff/34001/28006
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2559001/diff/34001/28006#newcode2097
chrome/browser/automation/automation_provider.cc:2097:
initial_load_observer_->GetTimingInformation());
indent by 4.

http://codereview.chromium.org/2559001/diff/34001/28009
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/2559001/diff/34001/28009#newcode39
chrome/browser/automation/automation_provider_observers.h:39: // Caller owns the
return value and is responsible for deleting it.
You should either document what's in the dictionary, or see see blah blah for
details of dictionary.

[Patrick Horn, 2010-06-10 20:06:07]

http://codereview.chromium.org/2559001/diff/34001/28006
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2559001/diff/34001/28006#newcode2097
chrome/browser/automation/automation_provider.cc:2097:
initial_load_observer_->GetTimingInformation());
On 2010/06/10 15:38:41, sky wrote:
> indent by 4.

Done.

http://codereview.chromium.org/2559001/diff/34001/28009
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/2559001/diff/34001/28009#newcode39
chrome/browser/automation/automation_provider_observers.h:39: // Caller owns the
return value and is responsible for deleting it.
On 2010/06/10 15:38:41, sky wrote:
> You should either document what's in the dictionary, or see see blah blah for
> details of dictionary.

Done.

[sky, 2010-06-10 22:24:14]

LGTM

[huanr, 2010-06-11 01:12:35]

LGTM

[Patrick Horn, 2010-06-16 01:19:31]

My commit (49862) got reverted today (49870) due to a mac buildbot link error.

(Paraphrased from IRC discussion) The error was a mmap error on mac. Looks to
have happened the previous day at 5pm as well, so it may have been a cron job.
Also possible is that new functions pushed the mac over the memory limit.

Here's the failed build:
http://build.chromium.org/buildbot/waterfall/builders/Chromium%20Mac%20Builde...

I'm hoping to recommit it tomorrow.