[Android] Don't wait for background tabs

[Android] Don't wait for background tabs

The session restore tests were timing out on Android. This was because
they were waiting for the background tabs to be loaded, which they
would not be in normal startup.

The only reason the test worked at all is that when devtools accessed
the background tabs this forced them to be loaded.

This changes the test to only wait for, and to only measure, the
foreground tab.

NOTRY=true
BUG=343168, 343900
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251364

[tonyg, 2014-02-12 22:25:57]

https://codereview.chromium.org/161473002/diff/1/tools/perf/measurements/sess...
File tools/perf/measurements/session_restore.py (right):

https://codereview.chromium.org/161473002/diff/1/tools/perf/measurements/sess...
tools/perf/measurements/session_restore.py:54: if 'android' in
tab.browser.browser_type:
I don't like defining the metric differently by platform. I think the metric
should just be Speed Index measured from launching the browser until foreground
tab visually complete.

If that is too much to bite off immediately, then I recommend we do the proposed
android behavior for all platforms.

[jeremy, 2014-02-13 09:40:14]

Agree with Tony that we should just have the Android behavior on all platforms.

https://codereview.chromium.org/161473002/diff/1/tools/perf/measurements/sess...
File tools/perf/measurements/session_restore.py (right):

https://codereview.chromium.org/161473002/diff/1/tools/perf/measurements/sess...
tools/perf/measurements/session_restore.py:72: # TODO(jeremy): Measure time to
load - first, last and frontmost tab here.
Can you leave this comment in when you make the change.

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

https://codereview.chromium.org/161473002/diff/1/tools/perf/metrics/startup_m...
tools/perf/metrics/startup_metric.py:83: # is tab[0]
Can you stick in a comment explaining in detail the Android behavior and that
we're doing the same thing on all platforms so we get the same measurement on
all of them.

[aberent, 2014-02-13 11:37:42]

https://codereview.chromium.org/161473002/diff/1/tools/perf/measurements/sess...
File tools/perf/measurements/session_restore.py (right):

https://codereview.chromium.org/161473002/diff/1/tools/perf/measurements/sess...
tools/perf/measurements/session_restore.py:54: if 'android' in
tab.browser.browser_type:
On 2014/02/12 22:25:57, tonyg wrote:
> I don't like defining the metric differently by platform. I think the metric
> should just be Speed Index measured from launching the browser until
foreground
> tab visually complete.
> 
> If that is too much to bite off immediately, then I recommend we do the
proposed
> android behavior for all platforms.

Switched to Android behaviour in all cases. Switching to speed index looks more
difficult, since it depends on polling the tab to decide whether there is more
data coming, and in this case we want to start timing before the tab (or Chrome)
exists. As such I am not doing that now.

https://codereview.chromium.org/161473002/diff/1/tools/perf/measurements/sess...
tools/perf/measurements/session_restore.py:72: # TODO(jeremy): Measure time to
load - first, last and frontmost tab here.
On 2014/02/13 09:40:15, jeremy wrote:
> Can you leave this comment in when you make the change.

Done.

[jeremy, 2014-02-13 13:35:26]

https://codereview.chromium.org/161473002/diff/120001/tools/perf/measurements...
File tools/perf/measurements/session_restore.py (right):

https://codereview.chromium.org/161473002/diff/120001/tools/perf/measurements...
tools/perf/measurements/session_restore.py:59: t = tab.browser.tabs[0]
Looks like we have no way of checking that this is in fact the frontmost tab
which kind of scares me :/

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

https://codereview.chromium.org/161473002/diff/120001/tools/perf/metrics/star...
tools/perf/metrics/startup_metric.py:44: # pylint: disable=W0101
nit:Why change the indent here?

https://codereview.chromium.org/161473002/diff/120001/tools/perf/metrics/star...
tools/perf/metrics/startup_metric.py:68: is_foreground_tab =
t.EvaluateJavaScript('!document.hidden')
You can remove this since we don't use it

[aberent, 2014-02-13 15:59:29]

https://codereview.chromium.org/161473002/diff/120001/tools/perf/measurements...
File tools/perf/measurements/session_restore.py (right):

https://codereview.chromium.org/161473002/diff/120001/tools/perf/measurements...
tools/perf/measurements/session_restore.py:59: t = tab.browser.tabs[0]
On 2014/02/13 13:35:26, jeremy wrote:
> Looks like we have no way of checking that this is in fact the frontmost tab
> which kind of scares me :/

Fixed, I think (just testing) although it only works through luck on Android
(due to a bug). See the next patch.

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

https://codereview.chromium.org/161473002/diff/120001/tools/perf/metrics/star...
tools/perf/metrics/startup_metric.py:44: # pylint: disable=W0101
On 2014/02/13 13:35:26, jeremy wrote:
> nit:Why change the indent here?

Accidental. Will check whether we still need this at all before fixing.

https://codereview.chromium.org/161473002/diff/120001/tools/perf/metrics/star...
tools/perf/metrics/startup_metric.py:68: is_foreground_tab =
t.EvaluateJavaScript('!document.hidden')
On 2014/02/13 13:35:26, jeremy wrote:
> You can remove this since we don't use it

Done.

[tonyg, 2014-02-14 16:05:41]

lgtm

https://codereview.chromium.org/161473002/diff/190001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/browser.py (right):

https://codereview.chromium.org/161473002/diff/190001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/browser.py:397: def foreground_tab(self):
Let's move this up to appear right below def tabs(self). Should help folks find
it easier.

Also, how would you feel about writing a unittest for this?

https://codereview.chromium.org/161473002/diff/190001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/browser.py:398: for i in xrange(len(self._tabs)):
Why do we need i?

Can't this be 

for tab in self.tabs:
  if tab.Evaluate...

https://codereview.chromium.org/161473002/diff/190001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/browser.py:400: # This only works through luck on
Android, due to crbug.com/322544
Man, we really need to fix the visibility api on android :/

https://codereview.chromium.org/161473002/diff/190001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/browser.py:405: return self._tabs[i]
Please raise an exception if we make it through the loop without returning.

[aberent, 2014-02-14 16:43:36]

New patch set, which I will land, follows once I have retested these changes.

https://codereview.chromium.org/161473002/diff/190001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/browser.py (right):

https://codereview.chromium.org/161473002/diff/190001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/browser.py:397: def foreground_tab(self):
On 2014/02/14 16:05:42, tonyg wrote:
> Let's move this up to appear right below def tabs(self). Should help folks
find
> it easier.

Done.
> 
> Also, how would you feel about writing a unittest for this?
Will do as a separate CL, want to get this landed and have a hard personal
deadline at 5:30 GMT today.

https://codereview.chromium.org/161473002/diff/190001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/browser.py:398: for i in xrange(len(self._tabs)):
On 2014/02/14 16:05:42, tonyg wrote:
> Why do we need i?
> 
> Can't this be 
> 
> for tab in self.tabs:
>   if tab.Evaluate...

Copied from original code after I tried 'for tab in self.tabs' and found I was
getting strings instead of tab objects back. I think the reason is to do with
the definitions of __getitem__ and __iter__ in tab_list_backend, but I haven't
fully investigated.

https://codereview.chromium.org/161473002/diff/190001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/browser.py:405: return self._tabs[i]
On 2014/02/14 16:05:42, tonyg wrote:
> Please raise an exception if we make it through the loop without returning.

Done.

[aberent, 2014-02-14 17:26:49]

The CQ bit was checked by aberent@chromium.org

[commit-bot: I haz the power, 2014-02-14 17:27:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/161473002/260001

[commit-bot: I haz the power, 2014-02-14 17:34:16]

Change committed as 251364