Refactor/rewrite scroll.js, reporting results as renderingStats objects.

chrome/test/data/scroll/scroll.js

Index: chrome/test/data/scroll/scroll.js
diff --git a/chrome/test/data/scroll/scroll.js b/chrome/test/data/scroll/scroll.js
index 49d2d445b4d0a75016caf5b27528f8f49c464036..f0096345c594db1a655e52f57f8c9ddc019128c3 100644
--- a/chrome/test/data/scroll/scroll.js
+++ b/chrome/test/data/scroll/scroll.js
@@ -3,119 +3,199 @@
 // found in the LICENSE file.
 
 // Inject this script on any page to measure framerate as the page is scrolled
-// twice from top to bottom.
+// from top to bottom.
 //
 // USAGE:
-//   1. To start the scrolling, invoke __scroll_test().
-//   2. Wait for __scrolling_complete to be true
-//   3. Read __frame_times array.
-
-
-// Function that sets a callback called when the next frame is rendered.
-var __set_frame_callback;
-
-// Function that scrolls the page.
-var __scroll_by;
-
-// Element that can be scrolled. Must provide scrollTop property.
-var __scrollable_element;
-
-// Amount of scrolling at each frame.
-var __scroll_delta = 100;
-
-// Number of scrolls to perform.
-var __num_scrolls = 2;
+//   1. Define a callback that takes the results array as a parameter.
+//   2. To start the test, instantiate a new ScrollTest().
+//   3a. When the test is complete, the callback will be called.
+//   3b. If no callback is specified, the results will be sent in
+//       JSON format through window.domAutomationController.
+//   3c. If there is no domAC, the results are sent to the console.
+
+window.performance = window.performance || {};

[nduca, 2012/08/14 00:24:34]

Its probably bad that we're mutating the window object. We should probably be
defining a local variable instead.

[dtu, 2012/08/16 02:31:10]

Done.

+performance.now = (function() {
+  return performance.now       ||
+         performance.mozNow    ||
+         performance.msNow     ||
+         performance.oNow      ||
+         performance.webkitNow ||
+         function() { return new Date().getTime(); };
+})();
+
+window.requestAnimationFrame = (function(){

[nduca, 2012/08/14 00:24:34]

same comment about mutating

[dtu, 2012/08/16 02:31:10]

Done.

+  return window.requestAnimationFrame       ||
+         window.webkitRequestAnimationFrame ||
+         window.mozRequestAnimationFrame    ||
+         window.oRequestAnimationFrame      ||
+         window.msRequestAnimationFrame     ||
+         function(callback) {
+           window.setTimeout(callback, 1000 / 60);
+         };
+})();
+
+var __isGmailTest = false;
+
+function BenchmarkingScrollStats() {

[nduca, 2012/08/14 00:24:34]

GpuBenchmarkingScrollStats

[dtu, 2012/08/16 02:31:10]

Done.

+  this.results = [];
+}
 
-// Current scroll position.
-var __ypos;
+BenchmarkingScrollStats.prototype.getRenderingStats = function() {

[nduca, 2012/08/14 00:24:34]

doesn't seem like it needs to be a member function...

[dtu, 2012/08/16 02:31:10]

Is it better in JavaScript to put it outside?
RafScrollStats.getDroppedFrameCount() also doesn't need to be a member, but it
doesn't really do anything useful outside of the class.

+  var stats = chrome.gpuBenchmarking.renderingStats();
+  stats.totalTimeInSeconds = window.performance.now() / 1000;

[nduca, 2012/08/14 00:24:34]

timestampWhenObtained?

[dtu, 2012/08/16 02:31:10]

As is, it is more consistent with totalPaintTimeInSeconds, etc; and the name
makes more sense after it's been diffed.

+  return stats;
+};
 
-// Time of previous scroll callback execution.
-var __start_time = 0;
+BenchmarkingScrollStats.prototype.getStatsDiff = function(old_stats) {
+  var stats = this.getRenderingStats();
+  for (var key in stats)
+    stats[key] -= old_stats[key];
+  return stats;
+};
 
-// True when all scrolling has completed.
-var __scrolling_complete = false;
+BenchmarkingScrollStats.prototype.startScroll = function(timestamp) {
+  this.initialStats = this.getRenderingStats();
+};
 
-// Array of frame times for each scroll in __num_scrolls.
-var __frame_times = [[]];
+BenchmarkingScrollStats.prototype.endStep = function(timestamp) {}
 
-// Set this to true when scrolling in Gmail.
-var __is_gmail_test = false;
+BenchmarkingScrollStats.prototype.endScroll = function() {
+  this.results.push(this.getStatsDiff(this.initialStats));

[nduca, 2012/08/14 00:24:34]

do we need to support more than one results?

[dtu, 2012/08/16 02:31:10]

Done.

+};
 
+BenchmarkingScrollStats.prototype.getResults = function() {
+  return this.results;
+};
 
-// Initializes the platform-independent frame callback scheduler function.
-function __init_set_frame_callback() {
-  __set_frame_callback = window.requestAnimationFrame ||
-                         window.mozRequestAnimationFrame ||
-                         window.msRequestAnimationFrame ||
-                         window.oRequestAnimationFrame ||
-                         window.webkitRequestAnimationFrame;
+function RAFScrollStats() {

[nduca, 2012/08/14 00:24:34]

this class should set up its own raf loop that records frame times.

[dtu, 2012/08/16 02:31:10]

Done. Huh, hadn't even occurred to me. That makes the interface cleaner.

+  this.results = [];
 }
 
-
-// Initializes the most realistic scrolling method.
-function __init_scroll_by() {
-  if (__is_gmail_test) {
-    __scroll_by = function(x, y) {
-      __scrollable_element.scrollByLines(1);
-    };
-  } else if (window.chrome && window.chrome.benchmarking &&
-             window.chrome.benchmarking.smoothScrollBy) {
-    __scroll_by = window.chrome.benchmarking.smoothScrollBy;
-  } else {
-    __scroll_by = window.scrollBy;
+RAFScrollStats.prototype.getDroppedFrameCount = function(frameTimes) {
+  var droppedFrameCount = 0;
+  for (var i = 1; i < frameTimes.length; i++) {
+    var frameTime = frameTimes[i] - frameTimes[i-1];
+    if (frameTime > 1000 / 55)
+      droppedFrameCount++;
   }
-}
-
-
-// Scrolls page down and reschedules itself until it hits the bottom.
-// Collects stats along the way.
-function __do_scroll(now_time) {
-  __scroll_by(0, __scroll_delta);
-  __set_frame_callback(function(now_time) {
-    if (__start_time) {
-      if (__scrollable_element.scrollTop > __ypos) {
-        // Scroll in progress, push a frame.
-        __frame_times[__frame_times.length-1].push(now_time - __start_time);
-      } else {
-        // Scroll complete, either scroll again or finish.
-        if (__frame_times.length < __num_scrolls) {
-          __scrollable_element.scrollTop = 0;
-          __frame_times.push([]);
-        } else {
-          console.log('frame_times', '' + __frame_times);
-          __scrolling_complete = true;
-          return;
-        }
-      }
-    }
-    __ypos = __scrollable_element.scrollTop;
-    __start_time = now_time;
-    __do_scroll();
-  });
-}
-
-
-function __start_scroll(scrollable_element) {
-  __scrollable_element = scrollable_element;
-  __init_scroll_by();
-  __set_frame_callback(__do_scroll);
-}
-
-
-// Performs the scroll test.
-function __scroll_test() {
-  __init_set_frame_callback();
-  if (__is_gmail_test) {
-    gmonkey.load("2.0", function(api) {
-        __start_scroll(api.getScrollableElement());
+  return droppedFrameCount;
+};
+
+RAFScrollStats.prototype.startScroll = function(timestamp) {
+  this.frameTimes = [timestamp];
+};
+
+RAFScrollStats.prototype.endStep = function(timestamp) {
+  this.frameTimes.push(timestamp);
+};
+
+RAFScrollStats.prototype.endScroll = function() {
+  var scrollResult = {};
+  scrollResult.numAnimationFrames = this.frameTimes.length - 1;
+  scrollResult.droppedFrameCount = this.getDroppedFrameCount(this.frameTimes);
+  scrollResult.totalTimeInSeconds =
+      (this.frameTimes[this.frameTimes.length - 1] - this.frameTimes[0]) / 1000;
+  this.results.push(scrollResult);
+};
+
+RAFScrollStats.prototype.getResults = function() {
+  return this.results;
+};
+
+// In the terminology of this class, a "step" is when the page
+// is being scrolled. processStep is run between the steps.
+// i.e. startScroll -> startStep -> scroll -> endStep ->
+//      processStep -> startStep -> scroll -> endStep -> endScroll
+function ScrollTest(callback) {
+  var self = this;
+
+  this.TOTAL_ITERATIONS = 2;
+  this.SCROLL_DELTA = 100;
+
+  this.callback = callback;
+  this.iteration = 0;
+
+  if (window.chrome && chrome.gpuBenchmarking)
+    this.scrollStats = new BenchmarkingScrollStats();

[nduca, 2012/08/14 00:24:34]

seems like this should be a private var, and just have a getter on this class
that plumbs to this.scrollStats_.results

[dtu, 2012/08/16 02:31:10]

Not sure what you're saying. A getter is not needed because the class returns
the result in a callback. The var is not used outside this class, so it could
just be named with an underscore.

+  else
+    this.scrollStats = new RAFScrollStats;

[nduca, 2012/08/14 00:24:34]

any reason for not ()?

[dtu, 2012/08/16 02:31:10]

Done.

+
+  if (__isGmailTest) {

[nduca, 2012/08/14 00:24:34]

feels like this could be a construction time variable?

[dtu, 2012/08/16 02:31:10]

Done.

+    gmonkey.load('2.0', function(api) {
+        self.start(api.getScrollableElement());
     });
   } else {
-    if (window.performance.timing.loadEventStart) {
-      __start_scroll(document.body);  // Page already loaded.
-    } else {
-      window.addEventListener('load', function() {
-        __start_scroll(document.body);  // Page hasn't loaded yet, schedule.
-      });
-    }
+    if (document.readyState == 'complete')
+      this.start();
+    else
+      window.addEventListener('load', function() { self.start(); });
   }
 }
+
+ScrollTest.prototype.scroll = function(x, y) {
+  if (__isGmailTest)
+    this.element.scrollByLines(1);
+  else
+    window.scrollBy(x, y);
+};
+
+ScrollTest.prototype.start = function(element) {
+  this.element = element || document.body;
+  window.requestAnimationFrame(this.startScroll.bind(this));
+};
+
+ScrollTest.prototype.startScroll = function(timestamp) {
+  this.element.scrollTop = 0;
+  this.scrollStats.startScroll(timestamp);
+  this.startStep();
+};
+
+ScrollTest.prototype.startStep = function() {
+  this.scroll(0, this.SCROLL_DELTA);
+  window.requestAnimationFrame(this.processStep.bind(this));
+};
+
+ScrollTest.prototype.endStep = function(timestamp) {
+  this.scrollStats.endStep(timestamp);

[nduca, 2012/08/14 00:24:34]

remove this

[dtu, 2012/08/16 02:31:10]

Done.

+};
+
+ScrollTest.prototype.endScroll = function() {
+  this.scrollStats.endScroll();
+  this.iteration++;
+};
+
+ScrollTest.prototype.sendResults = function() {
+  var results = this.scrollStats.getResults();
+  if (this.callback)
+    this.callback(results);
+  else if (window.domAutomationController)

[nduca, 2012/08/14 00:24:34]

Does anything still use this send case? I wonder if we can modify the python
code to hook in toto the scroll test instead?

[dtu, 2012/08/16 02:31:10]

Done. On the Python side that looks like:
__ScrollTest(function(results) {
  window.domAutomationController.send(JSON.stringify(results));
});

+    window.domAutomationController.send(JSON.stringify(results));
+  else
+    console.log(results);
+};
+
+ScrollTest.prototype.processStep = function(timestamp) {
+  this.endStep(timestamp);

[nduca, 2012/08/14 00:24:34]

seems like we should remove this. RAF loop that drives the stats should be
independent of the raf loop that drives scrolling.

[dtu, 2012/08/16 02:31:10]

Done.

+
+  // clientHeight is "special" for the body element.
+  if (this.element == document.body)
+    var clientHeight = window.innerHeight;
+  else
+    var clientHeight = this.element.clientHeight;
+
+  var isScrollComplete =
+      this.element.scrollTop + clientHeight >= this.element.scrollHeight;
+  if (isScrollComplete) {
+    this.endScroll();
+
+    var isTestComplete = this.iteration >= this.TOTAL_ITERATIONS;
+    if (isTestComplete)
+      this.sendResults();
+    else
+      window.requestAnimationFrame(this.startScroll.bind(this));
+
+    return;
+  }
+
+  this.startStep();
+};