Bug 1227477 - Polish the way the timeline time graduations are calculated; r=pbro

devtools/client/animationinspector/components/animation-timeline.js

@@ -410,15 +410,20 @@ AnimationsTimeline.prototype = {
 
   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",

devtools/client/animationinspector/test/browser_animation_timeline_header.js

@@ -24,11 +24,14 @@ add_task(function*() {
 
   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");
@@ -36,9 +39,9 @@ add_task(function*() {
   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
@@ -46,6 +49,6 @@ add_task(function*() {
     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`);
   });
 });

devtools/client/animationinspector/test/unit/test_findOptimalTimeInterval.js

@@ -14,66 +14,64 @@ const {findOptimalTimeInterval} = require("devtools/client/animationinspector/ut
 // 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,
+  minTimeInterval: 33,
   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,
-  expectedInterval: 50
+  minTimeInterval: 987,
+  expectedInterval: 1000
 }, {
-  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
+  minTimeInterval: 1234,
+  expectedInterval: 2500
 }, {
-  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
-}, {
-  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
-}, {
-  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 {

devtools/client/animationinspector/utils.js

@@ -18,17 +18,15 @@ 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;
 
@@ -72,9 +70,9 @@ exports.createNode = createNode;
  * @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");
 
@@ -93,17 +91,19 @@ function drawGraphElementBackground(document, id, graphWidth, timeScale) {
 
   // 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.
@@ -116,31 +116,30 @@ 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;
+    for (let i = 0; i < OPTIMAL_TIME_INTERVAL_MULTIPLES.length; i++) {
+      interval = OPTIMAL_TIME_INTERVAL_MULTIPLES[i] * multiplier;
+      if (minTimeInterval <= interval) {
+        return interval;
+      }
+    }
     if (++numIters > OPTIMAL_TIME_INTERVAL_MAX_ITERS) {
-      return scaledStep;
+      return interval;
     }
-    if (scaledStep < minSpacing) {
-      timingStep *= 2;
-      continue;
-    }
-    return scaledStep;
+    multiplier *= 10;
   }
 }