Re-subimission of https://codereview.chromium.org/1041213003/

Re-subimission of https://codereview.chromium.org/1041213003/

1. Use window.performance.timing instead of statsCollectionController to get page loading time.
2. Fix waiting condition to avoid racing condition

BUG=472603, 467964
Committed: https://crrev.com/ad8d3f3d9182bb82f89c5d29e0c455d8deb8c15a
Cr-Commit-Position: refs/heads/master@{#324118}

[nednguyen, 2015-04-01 17:21:48]

nednguyen@google.com changed reviewers:
+ nednguyen@google.com

[nednguyen, 2015-04-01 17:21:49]

This time, I think we should separate the wait fix from the 
['request_start_ms', 'load_end_ms'] refactoring. Can you do the refactoring in
the next patch and keep this one focus on adding the wait for load only?

[cylee1, 2015-04-01 17:55:57]

On 2015/04/01 17:21:49, nednguyen wrote:
> This time, I think we should separate the wait fix from the 
> ['request_start_ms', 'load_end_ms'] refactoring. Can you do the refactoring in
> the next patch and keep this one focus on adding the wait for load only?

Hi Ned,
  I would like to do so, but as I stated before the problem is I don't know an
easy way to check whether the page is fully loaded.
  The original approach checks document.readyState which ensures document ready.
But that's not equal to window loaded event.
  I can't use window.onload either because it requires registering an event
handler like
    window.onload = function{ /* set some variable to true */ }
  *before* start loading the page (can I ?)
 
  So I can only try to check  
        t.WaitForJavaScriptExpression(
            'statsCollectionController.tabLoadTiming()["load_duration_ms"] > 0')
  But the problem is statsCollectionController.tabLoad() seems not always exist
because there's guard like
     if 'load_start_ms' not in result or 'load_duration_ms' not in result:
          raise Exception("Outdated Chrome version, "
              "statsCollectionController.tabLoadTiming() not present")
  in the original code.

  As a result if statsCollectionController.tabLoad() doesn't exist, it will
returns some error or timeout.
  I'm not sure if I'll break some existing performance test on some platforms
this way.

  Ideas?

[cylee1, 2015-04-03 21:22:10]

On 2015/04/01 17:55:57, cylee1 wrote:
> On 2015/04/01 17:21:49, nednguyen wrote:
> > This time, I think we should separate the wait fix from the 
> > ['request_start_ms', 'load_end_ms'] refactoring. Can you do the refactoring
in
> > the next patch and keep this one focus on adding the wait for load only?
> 
> Hi Ned,
>   I would like to do so, but as I stated before the problem is I don't know an
> easy way to check whether the page is fully loaded.
>   The original approach checks document.readyState which ensures document
ready.
> But that's not equal to window loaded event.
>   I can't use window.onload either because it requires registering an event
> handler like
>     window.onload = function{ /* set some variable to true */ }
>   *before* start loading the page (can I ?)
>  
>   So I can only try to check  
>         t.WaitForJavaScriptExpression(
>             'statsCollectionController.tabLoadTiming()["load_duration_ms"] >
0')
>   But the problem is statsCollectionController.tabLoad() seems not always
exist
> because there's guard like
>      if 'load_start_ms' not in result or 'load_duration_ms' not in result:
>           raise Exception("Outdated Chrome version, "
>               "statsCollectionController.tabLoadTiming() not present")
>   in the original code.
> 
>   As a result if statsCollectionController.tabLoad() doesn't exist, it will
> returns some error or timeout.
>   I'm not sure if I'll break some existing performance test on some platforms
> this way.
> 
>   Ideas?

I guess I didn't make it clear.
I mean the API "statsCollectionController.tabLoad" doesn't seem to exists in
older version of chrome (or platform ?)
So I can't  simply call 
  t.WaitForJavaScriptExpression(statsCollectionController.tabLoad[...])
because it may fail. 
My solution is to wait for window.performance.timing[...]. I don't see how can I
separate the fix into two CLs.

[nednguyen, 2015-04-03 21:22:15]

On 2015/04/01 17:55:57, cylee1 wrote:
> On 2015/04/01 17:21:49, nednguyen wrote:
> > This time, I think we should separate the wait fix from the 
> > ['request_start_ms', 'load_end_ms'] refactoring. Can you do the refactoring
in
> > the next patch and keep this one focus on adding the wait for load only?
> 
> Hi Ned,
>   I would like to do so, but as I stated before the problem is I don't know an
> easy way to check whether the page is fully loaded.
>   The original approach checks document.readyState which ensures document
ready.
> But that's not equal to window loaded event.
>   I can't use window.onload either because it requires registering an event
> handler like
>     window.onload = function{ /* set some variable to true */ }
>   *before* start loading the page (can I ?)
>  
>   So I can only try to check  
>         t.WaitForJavaScriptExpression(
>             'statsCollectionController.tabLoadTiming()["load_duration_ms"] >
0')
>   But the problem is statsCollectionController.tabLoad() seems not always
exist
> because there's guard like
>      if 'load_start_ms' not in result or 'load_duration_ms' not in result:
>           raise Exception("Outdated Chrome version, "
>               "statsCollectionController.tabLoadTiming() not present")
>   in the original code.
> 
>   As a result if statsCollectionController.tabLoad() doesn't exist, it will
> returns some error or timeout.
>   I'm not sure if I'll break some existing performance test on some platforms
> this way.
> 
>   Ideas?


Can you run benchmarks that use this metrics through perf trybot:
http://www.chromium.org/developers/performance-try-bots to make sure things are
green before we land this.

[nednguyen, 2015-04-03 21:24:59]

https://codereview.chromium.org/1056533003/diff/1/tools/perf/metrics/startup_...
File tools/perf/metrics/startup_metric.py (right):

https://codereview.chromium.org/1056533003/diff/1/tools/perf/metrics/startup_...
tools/perf/metrics/startup_metric.py:73:
tab_load_times.append(TabLoadTime(request_start, load_event_end))
Why do we need to use a list here? Seems like this list always have length=1 and
we only access tab_load_times[0]

[cylee1, 2015-04-03 21:26:57]

https://codereview.chromium.org/1056533003/diff/1/tools/perf/metrics/startup_...
File tools/perf/metrics/startup_metric.py (right):

https://codereview.chromium.org/1056533003/diff/1/tools/perf/metrics/startup_...
tools/perf/metrics/startup_metric.py:73:
tab_load_times.append(TabLoadTime(request_start, load_event_end))
On 2015/04/03 21:24:59, nednguyen wrote:
> Why do we need to use a list here? Seems like this list always have length=1
and
> we only access tab_load_times[0]

I dont' know. It's how it was originally.
Do you suggest a refactoring in this CL?

[nednguyen, 2015-04-03 21:28:36]

lgtm

Please wait for the trybot results to be green before you land this.

https://codereview.chromium.org/1056533003/diff/1/tools/perf/metrics/startup_...
File tools/perf/metrics/startup_metric.py (right):

https://codereview.chromium.org/1056533003/diff/1/tools/perf/metrics/startup_...
tools/perf/metrics/startup_metric.py:73:
tab_load_times.append(TabLoadTime(request_start, load_event_end))
On 2015/04/03 21:26:57, cylee1 wrote:
> On 2015/04/03 21:24:59, nednguyen wrote:
> > Why do we need to use a list here? Seems like this list always have length=1
> and
> > we only access tab_load_times[0]
> 
> I dont' know. It's how it was originally.
> Do you suggest a refactoring in this CL? 

I prefer refactoring work to be done in a different patch :-)

[cylee1, 2015-04-03 21:32:43]

On 2015/04/03 21:28:36, nednguyen wrote:
> lgtm
> 
> Please wait for the trybot results to be green before you land this.
> 
>
https://codereview.chromium.org/1056533003/diff/1/tools/perf/metrics/startup_...
> File tools/perf/metrics/startup_metric.py (right):
> 
>
https://codereview.chromium.org/1056533003/diff/1/tools/perf/metrics/startup_...
> tools/perf/metrics/startup_metric.py:73:
> tab_load_times.append(TabLoadTime(request_start, load_event_end))
> On 2015/04/03 21:26:57, cylee1 wrote:
> > On 2015/04/03 21:24:59, nednguyen wrote:
> > > Why do we need to use a list here? Seems like this list always have
length=1
> > and
> > > we only access tab_load_times[0]
> > 
> > I dont' know. It's how it was originally.
> > Do you suggest a refactoring in this CL? 
> 
> I prefer refactoring work to be done in a different patch :-)

Thank you !
Actually I manually selected some perf trybots in the trybot list and I'm not
sure if they're supposed to work
The error seems to be a patching error 
http://build.chromium.org/p/client.skia/builders/Perf-Win7-MSVC-ShuttleA-GPU-...
and not related to my actually change.

How do you usually run (perf) trybots ?

[nednguyen, 2015-04-03 21:34:45]

On 2015/04/03 21:32:43, cylee1 wrote:
> On 2015/04/03 21:28:36, nednguyen wrote:
> > lgtm
> > 
> > Please wait for the trybot results to be green before you land this.
> > 
> >
>
https://codereview.chromium.org/1056533003/diff/1/tools/perf/metrics/startup_...
> > File tools/perf/metrics/startup_metric.py (right):
> > 
> >
>
https://codereview.chromium.org/1056533003/diff/1/tools/perf/metrics/startup_...
> > tools/perf/metrics/startup_metric.py:73:
> > tab_load_times.append(TabLoadTime(request_start, load_event_end))
> > On 2015/04/03 21:26:57, cylee1 wrote:
> > > On 2015/04/03 21:24:59, nednguyen wrote:
> > > > Why do we need to use a list here? Seems like this list always have
> length=1
> > > and
> > > > we only access tab_load_times[0]
> > > 
> > > I dont' know. It's how it was originally.
> > > Do you suggest a refactoring in this CL? 
> > 
> > I prefer refactoring work to be done in a different patch :-)
> 
> Thank you !
> Actually I manually selected some perf trybots in the trybot list and I'm not
> sure if they're supposed to work
> The error seems to be a patching error 
>
http://build.chromium.org/p/client.skia/builders/Perf-Win7-MSVC-ShuttleA-GPU-...
> and not related to my actually change.
> 
> How do you usually run (perf) trybots ?

You can find the instructions in
http://www.chromium.org/developers/performance-try-bots.

[nednguyen, 2015-04-03 22:07:09]

https://codereview.chromium.org/1056533003/diff/1/tools/perf/metrics/startup_...
File tools/perf/metrics/startup_metric.py (right):

https://codereview.chromium.org/1056533003/diff/1/tools/perf/metrics/startup_...
tools/perf/metrics/startup_metric.py:89: foreground_tab_stats =
tab_load_times[0]
If the TimeoutExceptions is thrown above, wouldn't this make tab_load_times
empty and creates a KeyError? Should we fail more gracefully here?

[cylee1, 2015-04-07 09:37:50]

Hi Ned,
  I followed the trybot instruction to run trybot as:
tools/perf/run_benchmark  --browser=trybot-all  startup.cold.blank_page

https://codereview.chromium.org/1063043002

However there are puzzling patch errors:
Failed to apply patch for tools/run-perf-test.cfg:

Ideas?

https://codereview.chromium.org/1056533003/diff/1/tools/perf/metrics/startup_...
File tools/perf/metrics/startup_metric.py (right):

https://codereview.chromium.org/1056533003/diff/1/tools/perf/metrics/startup_...
tools/perf/metrics/startup_metric.py:89: foreground_tab_stats =
tab_load_times[0]
On 2015/04/03 22:07:08, nednguyen wrote:
> If the TimeoutExceptions is thrown above, wouldn't this make tab_load_times
> empty and creates a KeyError? Should we fail more gracefully here?

True, it was a legacy issue. 
Did a tiny refactoring to resolve it.

https://codereview.chromium.org/1056533003/diff/20001/tools/perf/metrics/star...
File tools/perf/metrics/startup_metric.py (right):

https://codereview.chromium.org/1056533003/diff/20001/tools/perf/metrics/star...
tools/perf/metrics/startup_metric.py:19: DEFAULT_LOADING_TIMEOUT = 90
Default timeout in the origianl code
  t.WaitForDocumentReadyStateToBeComplete()
is 90 seconds.
So it's basically copy the settings here.

[nednguyen, 2015-04-07 14:54:31]

On 2015/04/07 09:37:50, cylee1 wrote:
> Hi Ned,
>   I followed the trybot instruction to run trybot as:
> tools/perf/run_benchmark  --browser=trybot-all  startup.cold.blank_page
> 
> https://codereview.chromium.org/1063043002
> 
> However there are puzzling patch errors:
> Failed to apply patch for tools/run-perf-test.cfg:
> 
> Ideas?

I have no idea. Maybe you need to rebase?

> 
>
https://codereview.chromium.org/1056533003/diff/1/tools/perf/metrics/startup_...
> File tools/perf/metrics/startup_metric.py (right):
> 
>
https://codereview.chromium.org/1056533003/diff/1/tools/perf/metrics/startup_...
> tools/perf/metrics/startup_metric.py:89: foreground_tab_stats =
> tab_load_times[0]
> On 2015/04/03 22:07:08, nednguyen wrote:
> > If the TimeoutExceptions is thrown above, wouldn't this make tab_load_times
> > empty and creates a KeyError? Should we fail more gracefully here?
> 
> True, it was a legacy issue. 
> Did a tiny refactoring to resolve it.
> 
>
https://codereview.chromium.org/1056533003/diff/20001/tools/perf/metrics/star...
> File tools/perf/metrics/startup_metric.py (right):
> 
>
https://codereview.chromium.org/1056533003/diff/20001/tools/perf/metrics/star...
> tools/perf/metrics/startup_metric.py:19: DEFAULT_LOADING_TIMEOUT = 90
> Default timeout in the origianl code
>   t.WaitForDocumentReadyStateToBeComplete()
> is 90 seconds.
> So it's basically copy the settings here.

[nednguyen, 2015-04-07 14:55:10]

nednguyen@google.com changed reviewers:
+ aiolos@chromium.org, dtu@chromium.org

[nednguyen, 2015-04-07 14:55:10]

+Dave, Kari for help with the trybot

[cylee1, 2015-04-07 19:58:34]

On 2015/04/07 14:55:10, nednguyen wrote:
> +Dave, Kari for help with the trybot

Hi Ned,
  Thanks for the remainder, I rebased my branch and now it passed on some
platforms (e.g. mac/android). The failure on windows doesn't look like caused by
this patch. Details here  https://codereview.chromium.org/1059193003
  Could I submit it now?

[nednguyen, 2015-04-07 20:13:44]

On 2015/04/07 19:58:34, cylee1 wrote:
> On 2015/04/07 14:55:10, nednguyen wrote:
> > +Dave, Kari for help with the trybot
> 
> Hi Ned,
>   Thanks for the remainder, I rebased my branch and now it passed on some
> platforms (e.g. mac/android). The failure on windows doesn't look like caused
by
> this patch. Details here  https://codereview.chromium.org/1059193003
>   Could I submit it now?

Yes, lgtm again.

[cylee1, 2015-04-07 20:14:42]

The CQ bit was checked by cylee@chromium.org

[commit-bot: I haz the power, 2015-04-07 20:15:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1056533003/20001

[commit-bot: I haz the power, 2015-04-07 21:07:34]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-07 21:08:31]

Patchset 2 (id:??) landed as
https://crrev.com/ad8d3f3d9182bb82f89c5d29e0c455d8deb8c15a
Cr-Commit-Position: refs/heads/master@{#324118}

[nednguyen, 2015-04-09 15:31:03]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1072053002/ by nednguyen@google.com.

The reason for reverting is: Speculative revert
(https://code.google.com/p/chromium/issues/detail?id=474977).