Telemetry on android: improves RawDisplayFrameRateMeasurement.

Telemetry on android: improves RawDisplayFrameRateMeasurement.

A few changes here:
- move up from "scrolling_action" to "smoothness_benchmark" (which knows about results).
- changes surface_stats_collector itself to provide but not print the results.
- creates a Telemetry "platform.RawDisplayResult" abstraction so that
android_platform_backend.py can use that, and the rest of Telemetry doesn't know about
android-specific details.
- while at it, also ensure that DidRunAction is in a finally block after RunAction to ensure
proper cleanup (if needed).

BUG=170894
TEST=


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

[bulach, 2013-03-08 14:22:11]

sort of a follow up to sami's fix at
https://chromiumcodereview.appspot.com/12432006, this should help better results
per url on the Telemetry tests.. ptal

[Sami, 2013-03-08 18:22:20]

lgtm, I think this makes a lot of sense. Just some nits below.

https://codereview.chromium.org/12499006/diff/1/build/android/pylib/surface_s...
File build/android/pylib/surface_stats_collector.py (right):

https://codereview.chromium.org/12499006/diff/1/build/android/pylib/surface_s...
build/android/pylib/surface_stats_collector.py:40: self._print_perf_results =
True
Is anyone using this in non-print-mode or should we just get rid of it?

https://codereview.chromium.org/12499006/diff/1/tools/perf/perf_tools/smoothn...
File tools/perf/perf_tools/smoothness_benchmark.py (right):

https://codereview.chromium.org/12499006/diff/1/tools/perf/perf_tools/smoothn...
tools/perf/perf_tools/smoothness_benchmark.py:209: for r in
tab.browser.platform.GetRawDisplayFrameRateMeasurement():
Nit: might be more idiomatic to do

   for name, unit, value in ...

https://codereview.chromium.org/12499006/diff/1/tools/telemetry/telemetry/cor...
File tools/telemetry/telemetry/core/chrome/android_platform_backend.py (right):

https://codereview.chromium.org/12499006/diff/1/tools/telemetry/telemetry/cor...
tools/telemetry/telemetry/core/chrome/android_platform_backend.py:51: for r in
self._surface_stats_collector.GetResults():
Ditto

https://codereview.chromium.org/12499006/diff/1/tools/telemetry/telemetry/cor...
File tools/telemetry/telemetry/core/chrome/platform.py (right):

https://codereview.chromium.org/12499006/diff/1/tools/telemetry/telemetry/cor...
tools/telemetry/telemetry/core/chrome/platform.py:27: class
RawDisplayResult(object):
Maybe RawDisplayFrameRateResult to reduce confusion?

[Sami, 2013-03-08 18:23:58]

https://codereview.chromium.org/12499006/diff/1/tools/perf/perf_tools/smoothn...
File tools/perf/perf_tools/smoothness_benchmark.py (right):

https://codereview.chromium.org/12499006/diff/1/tools/perf/perf_tools/smoothn...
tools/perf/perf_tools/smoothness_benchmark.py:209: for r in
tab.browser.platform.GetRawDisplayFrameRateMeasurement():
On 2013/03/08 18:22:20, Sami wrote:
> Nit: might be more idiomatic to do
> 
>    for name, unit, value in ...

Ah, just realized we're not getting back a tuple but an actual object.
Nevermind.

[bulach, 2013-03-11 10:18:01]

thanks sami! all addressed, another look please?

https://codereview.chromium.org/12499006/diff/1/build/android/pylib/surface_s...
File build/android/pylib/surface_stats_collector.py (right):

https://codereview.chromium.org/12499006/diff/1/build/android/pylib/surface_s...
build/android/pylib/surface_stats_collector.py:40: self._print_perf_results =
True
On 2013/03/08 18:22:20, Sami wrote:
> Is anyone using this in non-print-mode or should we just get rid of it?

N-sided patch... :-/
Telemetry with this patch no longer relies on the print-mode, but downstream
still does..

https://codereview.chromium.org/12499006/diff/1/tools/perf/perf_tools/smoothn...
File tools/perf/perf_tools/smoothness_benchmark.py (right):

https://codereview.chromium.org/12499006/diff/1/tools/perf/perf_tools/smoothn...
tools/perf/perf_tools/smoothness_benchmark.py:209: for r in
tab.browser.platform.GetRawDisplayFrameRateMeasurement():
On 2013/03/08 18:22:20, Sami wrote:
> Nit: might be more idiomatic to do
> 
>    for name, unit, value in ...

Done.

https://codereview.chromium.org/12499006/diff/1/tools/telemetry/telemetry/cor...
File tools/telemetry/telemetry/core/chrome/android_platform_backend.py (right):

https://codereview.chromium.org/12499006/diff/1/tools/telemetry/telemetry/cor...
tools/telemetry/telemetry/core/chrome/android_platform_backend.py:51: for r in
self._surface_stats_collector.GetResults():
On 2013/03/08 18:22:20, Sami wrote:
> Ditto

Done.

https://codereview.chromium.org/12499006/diff/1/tools/telemetry/telemetry/cor...
File tools/telemetry/telemetry/core/chrome/platform.py (right):

https://codereview.chromium.org/12499006/diff/1/tools/telemetry/telemetry/cor...
tools/telemetry/telemetry/core/chrome/platform.py:27: class
RawDisplayResult(object):
On 2013/03/08 18:22:20, Sami wrote:
> Maybe RawDisplayFrameRateResult to reduce confusion?

yeah, good point! 
changed to RawDisplayFrameRateMeasurement and the following to plural,
GetRawDisplayFrameRateMeasurements as it returns a list of these objects, so now
everything is about "RawDisplayFrameRateMeasurement"...

[Sami, 2013-03-11 10:36:07]

On 2013/03/11 10:18:01, bulach wrote:
> N-sided patch... :-/
> Telemetry with this patch no longer relies on the print-mode, but downstream
> still does..

Ah, right. Let's try to clean that up once this lands.
 
> yeah, good point! 
> changed to RawDisplayFrameRateMeasurement and the following to plural,
> GetRawDisplayFrameRateMeasurements as it returns a list of these objects, so
now
> everything is about "RawDisplayFrameRateMeasurement"...

Thanks, much clearer, lgtm.

[bulach, 2013-03-12 13:01:41]

+marja

[nduca, 2013-03-13 05:47:54]

lgtm this is so much nicer, thank you

https://codereview.chromium.org/12499006/diff/7001/build/android/pylib/surfac...
File build/android/pylib/surface_stats_collector.py (right):

https://codereview.chromium.org/12499006/diff/7001/build/android/pylib/surfac...
build/android/pylib/surface_stats_collector.py:67: def
SuppressPrintingResults(self):
is there any time when this isn't done? Can we just remove this? And/or just
make the _PrintPerfResults a public method for the limited case where its
needed?

[Sami, 2013-03-13 10:27:38]

On 2013/03/13 05:47:54, nduca wrote:
> is there any time when this isn't done? Can we just remove this? And/or just
> make the _PrintPerfResults a public method for the limited case where its
> needed?

See comment #4 -- we'll get rid of that as soon as the downstream dependency
goes away.

[marja, 2013-03-13 13:16:31]

Removing myself from the reviewer list, since nduca@ already reviewed.

[bulach, 2013-03-13 13:46:40]

thanks all! I'll land this, as soon as it rolls downstream, I'll fix the API
usage on the other side, then I'll come back to clean it up..

[commit-bot: I haz the power, 2013-03-13 13:47:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/12499006/7001

[commit-bot: I haz the power, 2013-03-13 13:47:41]

Failed to apply patch for tools/telemetry/telemetry/page/scrolling_action.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file tools/telemetry/telemetry/page/scrolling_action.py
  Hunk #1 FAILED at 25.
  1 out of 1 hunk FAILED -- saving rejects to file
tools/telemetry/telemetry/page/scrolling_action.py.rej

Patch:       tools/telemetry/telemetry/page/scrolling_action.py
Index: tools/telemetry/telemetry/page/scrolling_action.py
diff --git a/tools/telemetry/telemetry/page/scrolling_action.py
b/tools/telemetry/telemetry/page/scrolling_action.py
index
5b01f178fbcbdde51824f8c37db402614d525670..6e1d293c7b963fbdda8ab5a3554f54cd3bb764f3
100644
--- a/tools/telemetry/telemetry/page/scrolling_action.py
+++ b/tools/telemetry/telemetry/page/scrolling_action.py
@@ -25,29 +25,23 @@ class ScrollingAction(page_action.PageAction):
      """)
 
   def RunAction(self, page, tab, previous_action):
-    try:
-      if tab.browser.platform.IsRawDisplayFrameRateSupported():
-        tab.browser.platform.StartRawDisplayFrameRateMeasurement('')
-      # scrollable_element_function is a function that passes the scrollable
-      # element on the page to a callback. For example:
-      #   function (callback) {
-      #     callback(document.getElementById('foo'));
-      #   }
-      if hasattr(self, 'scrollable_element_function'):
-        tab.ExecuteJavaScript("""
-            (%s)(function(element) {
-              window.__scrollingAction.start(element);
-            });""" % (self.scrollable_element_function))
-      else:
-        tab.ExecuteJavaScript(
-          'window.__scrollingAction.start(document.body);')
-
-      # Poll for scroll benchmark completion.
-      util.WaitFor(lambda: tab.EvaluateJavaScript(
-          'window.__scrollingActionDone'), 60)
-    finally:
-      if tab.browser.platform.IsRawDisplayFrameRateSupported():
-        tab.browser.platform.StopRawDisplayFrameRateMeasurement()
+    # scrollable_element_function is a function that passes the scrollable
+    # element on the page to a callback. For example:
+    #   function (callback) {
+    #     callback(document.getElementById('foo'));
+    #   }
+    if hasattr(self, 'scrollable_element_function'):
+      tab.ExecuteJavaScript("""
+          (%s)(function(element) {
+            window.__scrollingAction.start(element);
+          });""" % (self.scrollable_element_function))
+    else:
+      tab.ExecuteJavaScript(
+        'window.__scrollingAction.start(document.body);')
+
+    # Poll for scroll benchmark completion.
+    util.WaitFor(lambda: tab.EvaluateJavaScript(
+        'window.__scrollingActionDone'), 60)
 
   def CanBeBound(self):
     return True

[commit-bot: I haz the power, 2013-03-13 18:51:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/12499006/25001

[commit-bot: I haz the power, 2013-03-13 23:23:25]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-03-14 09:52:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/12499006/25001

[commit-bot: I haz the power, 2013-03-14 09:57:51]

Change committed as 188037