Bug 1227477 - Polish the way the timeline time graduations are calculated; r=pbro
authorNicolas Chevobbe <chevobbe.nicolas@gmail.com>
Fri, 15 Jan 2016 22:45:05 +0100
changeset 282175 0ae6c7e277af
parent 282174 103a122789a3
child 282282 df5126ec7873
push id17290
push userpbrosset@mozilla.com
push dateFri, 29 Jan 2016 10:18:30 +0000
treeherderfx-team@0ae6c7e277af [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbro
bugs1227477
milestone47.0a1
Bug 1227477 - Polish the way the timeline time graduations are calculated; r=pbro
devtools/client/animationinspector/components/animation-timeline.js
devtools/client/animationinspector/test/browser_animation_timeline_header.js
devtools/client/animationinspector/test/unit/test_findOptimalTimeInterval.js
devtools/client/animationinspector/utils.js
--- a/devtools/client/animationinspector/components/animation-timeline.js
+++ b/devtools/client/animationinspector/components/animation-timeline.js
@@ -405,25 +405,30 @@ AnimationsTimeline.prototype = {
   onAnimationStateChanged: function() {
     // For now, simply re-render the component. The animation front's state has
     // already been updated.
     this.render(this.animations);
   },
 
   drawHeaderAndBackground: function() {
     let width = this.timeHeaderEl.offsetWidth;
-    let scale = width / (TimeScale.maxEndTime - TimeScale.minStartTime);
+    let animationDuration = TimeScale.maxEndTime - TimeScale.minStartTime;
+    let minTimeInterval = TIME_GRADUATION_MIN_SPACING * animationDuration / width;
+    let intervalLength = findOptimalTimeInterval(minTimeInterval);
+    let intervalWidth = intervalLength * width / animationDuration;
+
     drawGraphElementBackground(this.win.document, "time-graduations",
-                               width, scale);
+                               width, intervalWidth);
 
     // And the time graduation header.
     this.timeHeaderEl.innerHTML = "";
-    let interval = findOptimalTimeInterval(scale, TIME_GRADUATION_MIN_SPACING);
-    for (let i = 0; i < width; i += interval) {
-      let pos = 100 * i / width;
+
+    for (let i = 0; i <= width / intervalWidth; i++) {
+      let pos = 100 * i * intervalWidth / width;
+
       createNode({
         parent: this.timeHeaderEl,
         nodeType: "span",
         attributes: {
           "class": "time-tick",
           "style": `left:${pos}%`
         },
         textContent: TimeScale.formatTime(TimeScale.distanceToRelativeTime(pos))
--- a/devtools/client/animationinspector/test/browser_animation_timeline_header.js
+++ b/devtools/client/animationinspector/test/browser_animation_timeline_header.js
@@ -19,33 +19,36 @@ add_task(function*() {
   yield addTab(TEST_URL_ROOT + "doc_simple_animation.html");
   let {panel} = yield openAnimationInspector();
 
   let timeline = panel.animationsTimelineComponent;
   let headerEl = timeline.timeHeaderEl;
 
   info("Find out how many time graduations should there be");
   let width = headerEl.offsetWidth;
-  let scale = width / (TimeScale.maxEndTime - TimeScale.minStartTime);
+
+  let animationDuration = TimeScale.maxEndTime - TimeScale.minStartTime;
+  let minTimeInterval = TIME_GRADUATION_MIN_SPACING * animationDuration / width;
+
   // Note that findOptimalTimeInterval is tested separately in xpcshell test
   // test_findOptimalTimeInterval.js, so we assume that it works here.
-  let interval = findOptimalTimeInterval(scale, TIME_GRADUATION_MIN_SPACING);
-  let nb = Math.ceil(width / interval);
+  let interval = findOptimalTimeInterval(minTimeInterval);
+  let nb = Math.ceil(animationDuration / interval);
 
   is(headerEl.querySelectorAll(".time-tick").length, nb,
      "The expected number of time ticks were found");
 
   info("Make sure graduations are evenly distributed and show the right times");
   [...headerEl.querySelectorAll(".time-tick")].forEach((tick, i) => {
     let left = parseFloat(tick.style.left);
-    let expectedPos = i * interval * 100 / width;
+    let expectedPos = i * interval * 100 / animationDuration;
     is(Math.round(left), Math.round(expectedPos),
-      "Graduation " + i + " is positioned correctly");
+      `Graduation ${i} is positioned correctly`);
 
     // Note that the distancetoRelativeTime and formatTime functions are tested
     // separately in xpcshell test test_timeScale.js, so we assume that they
     // work here.
     let formattedTime = TimeScale.formatTime(
       TimeScale.distanceToRelativeTime(expectedPos, width));
     is(tick.textContent, formattedTime,
-      "Graduation " + i + " has the right text content");
+      `Graduation ${i} has the right text content`);
   });
 });
--- a/devtools/client/animationinspector/test/unit/test_findOptimalTimeInterval.js
+++ b/devtools/client/animationinspector/test/unit/test_findOptimalTimeInterval.js
@@ -9,75 +9,73 @@
 var Cu = Components.utils;
 const {require} = Cu.import("resource://devtools/shared/Loader.jsm", {});
 const {findOptimalTimeInterval} = require("devtools/client/animationinspector/utils");
 
 // This test array contains objects that are used to test the
 // findOptimalTimeInterval function. Each object should have the following
 // properties:
 // - desc: an optional string that will be printed out
-// - timeScale: a number that represents how many pixels is 1ms
-// - minSpacing: an optional number that represents the minim space between 2
-//   time graduations
+// - minTimeInterval: a number that represents the minimum time in ms
+//   that should be displayed in one interval
 // - expectedInterval: a number that you expect the findOptimalTimeInterval
 //   function to return as a result.
 //   Optionally you can pass a string where `interval` is the calculated
 //   interval, this string will be eval'd and tested to be truthy.
 const TEST_DATA = [{
-  desc: "With 1px being 1ms and no minSpacing, expect the interval to be the " +
-        "interval multiple",
-  timeScale: 1,
-  minSpacing: undefined,
+  desc: "With no minTimeInterval, expect the interval to be 0",
+  minTimeInterval: null,
+  expectedInterval: 0
+}, {
+  desc: "With a minTimeInterval of 0 ms, expect the interval to be 0",
+  minTimeInterval: 0,
+  expectedInterval: 0
+}, {
+  desc: "With a minInterval of 1ms, expect the interval to be the 1ms too",
+  minTimeInterval: 1,
+  expectedInterval: 1
+}, {
+  desc: "With a very small minTimeInterval, expect the interval to be 1ms",
+  minTimeInterval: 1e-31,
+  expectedInterval: 1
+}, {
+  desc: "With a minInterval of 2.5ms, expect the interval to be 2.5ms too",
+  minTimeInterval: 2.5,
+  expectedInterval: 2.5
+}, {
+  desc: "With a minInterval of 5ms, expect the interval to be 5ms too",
+  minTimeInterval: 5,
+  expectedInterval: 5
+}, {
+  desc: "With a minInterval of 7ms, expect the interval to be the next " +
+        "multiple of 5",
+  minTimeInterval: 7,
+  expectedInterval: 10
+}, {
+  minTimeInterval: 20,
   expectedInterval: 25
 }, {
-  desc: "With 1px being 1ms and a custom minSpacing being a multiple of 25 " +
-        "expect the interval to be the custom min spacing",
-  timeScale: 1,
-  minSpacing: 50,
-  expectedInterval: 50
-}, {
-  desc: "With 1px being 1ms and a custom minSpacing not being multiple of 25 " +
-        "expect the interval to be the next multiple of 10",
-  timeScale: 1,
-  minSpacing: 26,
+  minTimeInterval: 33,
   expectedInterval: 50
 }, {
-  desc: "If 1ms corresponds to a distance that is greater than the min " +
-        "spacing then, expect the interval to be this distance",
-  timeScale: 20,
-  minSpacing: undefined,
-  expectedInterval: 20
-}, {
-  desc: "If 1ms corresponds to a distance that is greater than the min " +
-        "spacing then, expect the interval to be this distance, even if it " +
-        "isn't a multiple of 25",
-  timeScale: 33,
-  minSpacing: undefined,
-  expectedInterval: 33
+  minTimeInterval: 987,
+  expectedInterval: 1000
 }, {
-  desc: "If 1ms is a very small distance, then expect this distance to be " +
-        "multiplied by 25, 50, 100, 200, etc... until it goes over the min " +
-        "spacing",
-  timeScale: 0.001,
-  minSpacing: undefined,
-  expectedInterval: 12.8
+  minTimeInterval: 1234,
+  expectedInterval: 2500
 }, {
-  desc: "If the time scale is such that we need to iterate more than the " +
-        "maximum allowed number of iterations, then expect an interval lower " +
-        "than the minimum one",
-  timeScale: 1e-31,
-  minSpacing: undefined,
-  expectedInterval: "interval < 10"
+  minTimeInterval: 9800,
+  expectedInterval: 10000
 }];
 
 function run_test() {
-  for (let {timeScale, desc, minSpacing, expectedInterval} of TEST_DATA) {
-    do_print("Testing timeScale: " + timeScale + " and minSpacing: " +
-              minSpacing + ". Expecting " + expectedInterval + ".");
+  for (let {minTimeInterval, desc, expectedInterval} of TEST_DATA) {
+    do_print(`Testing minTimeInterval: ${minTimeInterval}.
+              Expecting ${expectedInterval}.`);
 
-    let interval = findOptimalTimeInterval(timeScale, minSpacing);
+    let interval = findOptimalTimeInterval(minTimeInterval);
     if (typeof expectedInterval == "string") {
       ok(eval(expectedInterval), desc);
     } else {
       equal(interval, expectedInterval, desc);
     }
   }
 }
--- a/devtools/client/animationinspector/utils.js
+++ b/devtools/client/animationinspector/utils.js
@@ -13,27 +13,25 @@ var {loader} = Cu.import("resource://dev
 loader.lazyRequireGetter(this, "EventEmitter",
                                "devtools/shared/event-emitter");
 
 const STRINGS_URI = "chrome://devtools/locale/animationinspector.properties";
 const L10N = new ViewHelpers.L10N(STRINGS_URI);
 // How many times, maximum, can we loop before we find the optimal time
 // interval in the timeline graph.
 const OPTIMAL_TIME_INTERVAL_MAX_ITERS = 100;
-// Background time graduations should be multiple of this number of millis.
-const TIME_INTERVAL_MULTIPLE = 25;
-const TIME_INTERVAL_SCALES = 3;
-// The default minimum spacing between time graduations in px.
-const TIME_GRADUATION_MIN_SPACING = 10;
+// Time graduations should be multiple of one of these number.
+const OPTIMAL_TIME_INTERVAL_MULTIPLES = [1, 2.5, 5];
+
 // RGB color for the time interval background.
 const TIME_INTERVAL_COLOR = [128, 136, 144];
 // byte
-const TIME_INTERVAL_OPACITY_MIN = 32;
+const TIME_INTERVAL_OPACITY_MIN = 64;
 // byte
-const TIME_INTERVAL_OPACITY_ADD = 32;
+const TIME_INTERVAL_OPACITY_MAX = 96;
 
 const MILLIS_TIME_FORMAT_MAX_DURATION = 4000;
 
 /**
  * DOM node creation helper function.
  * @param {Object} Options to customize the node to be created.
  * - nodeType {String} Optional, defaults to "div",
  * - attributes {Object} Optional attributes object like
@@ -67,19 +65,19 @@ exports.createNode = createNode;
 
 /**
  * Given a data-scale, draw the background for a graph (vertical lines) into a
  * canvas and set that canvas as an image-element with an ID that can be used
  * from CSS.
  * @param {Document} document The document where the image-element should be set.
  * @param {String} id The ID for the image-element.
  * @param {Number} graphWidth The width of the graph.
- * @param {Number} timeScale How many px is 1ms in the graph.
+ * @param {Number} intervalWidth The width of one interval
  */
-function drawGraphElementBackground(document, id, graphWidth, timeScale) {
+function drawGraphElementBackground(document, id, graphWidth, intervalWidth) {
   let canvas = document.createElement("canvas");
   let ctx = canvas.getContext("2d");
 
   // Set the canvas width (as requested) and height (1px, repeated along the Y
   // axis).
   canvas.width = graphWidth;
   canvas.height = 1;
 
@@ -88,64 +86,65 @@ function drawGraphElementBackground(docu
   let pixelArray = imageData.data;
 
   let buf = new ArrayBuffer(pixelArray.length);
   let view8bit = new Uint8ClampedArray(buf);
   let view32bit = new Uint32Array(buf);
 
   // Build new millisecond tick lines...
   let [r, g, b] = TIME_INTERVAL_COLOR;
-  let alphaComponent = TIME_INTERVAL_OPACITY_MIN;
-  let interval = findOptimalTimeInterval(timeScale);
+  let opacities = [TIME_INTERVAL_OPACITY_MAX, TIME_INTERVAL_OPACITY_MIN];
 
-  // Insert one pixel for each division on each scale.
-  for (let i = 1; i <= TIME_INTERVAL_SCALES; i++) {
-    let increment = interval * Math.pow(2, i);
-    for (let x = 0; x < canvas.width; x += increment) {
-      let position = x | 0;
-      view32bit[position] = (alphaComponent << 24) | (b << 16) | (g << 8) | r;
+  // Insert one tick line on each interval
+  for (let i = 0; i <= graphWidth / intervalWidth; i++) {
+    let x = i * intervalWidth;
+    // Ensure the last line is drawn on canvas
+    if (x >= graphWidth) {
+      x = graphWidth - 0.5;
     }
-    alphaComponent += TIME_INTERVAL_OPACITY_ADD;
+    let position = x | 0;
+    let alphaComponent = opacities[i % opacities.length];
+
+    view32bit[position] = (alphaComponent << 24) | (b << 16) | (g << 8) | r;
   }
 
   // Flush the image data and cache the waterfall background.
   pixelArray.set(view8bit);
   ctx.putImageData(imageData, 0, 0);
   document.mozSetImageElement(id, canvas);
 }
 
 exports.drawGraphElementBackground = drawGraphElementBackground;
 
 /**
  * Find the optimal interval between time graduations in the animation timeline
- * graph based on a time scale and a minimum spacing.
- * @param {Number} timeScale How many px is 1ms in the graph.
- * @param {Number} minSpacing The minimum spacing between 2 graduations,
- * defaults to TIME_GRADUATION_MIN_SPACING.
- * @return {Number} The optimal interval, in pixels.
+ * graph based on a minimum time interval
+ * @param {Number} minTimeInterval Minimum time in ms in one interval
+ * @return {Number} The optimal interval time in ms
  */
-function findOptimalTimeInterval(timeScale,
-                                 minSpacing=TIME_GRADUATION_MIN_SPACING) {
-  let timingStep = TIME_INTERVAL_MULTIPLE;
+function findOptimalTimeInterval(minTimeInterval) {
   let numIters = 0;
+  let multiplier = 1;
 
-  if (timeScale > minSpacing) {
-    return timeScale;
+  if (!minTimeInterval) {
+    return 0;
   }
 
+  let interval;
   while (true) {
-    let scaledStep = timeScale * timingStep;
-    if (++numIters > OPTIMAL_TIME_INTERVAL_MAX_ITERS) {
-      return scaledStep;
+    for (let i = 0; i < OPTIMAL_TIME_INTERVAL_MULTIPLES.length; i++) {
+      interval = OPTIMAL_TIME_INTERVAL_MULTIPLES[i] * multiplier;
+      if (minTimeInterval <= interval) {
+        return interval;
+      }
     }
-    if (scaledStep < minSpacing) {
-      timingStep *= 2;
-      continue;
+    if (++numIters > OPTIMAL_TIME_INTERVAL_MAX_ITERS) {
+      return interval;
     }
-    return scaledStep;
+    multiplier *= 10;
   }
 }
 
 exports.findOptimalTimeInterval = findOptimalTimeInterval;
 
 /**
  * Format a timestamp (in ms) as a mm:ss.mmm string.
  * @param {Number} time