Revision to CPM UI using Flot and JQuery

chrome/browser/resources/performance_monitor/chart.js

 /* Copyright (c) 2012 The Chromium Authors. All rights reserved.
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file. */
 
 'use strict';
 
-google.load('visualization', '1', {packages: ['corechart']});
-
-function $(criterion) {
+function querySelect(criterion) {

[Dan Beam, 2012/07/02 21:33:10]

put this inside controller if it's only used in there to avoid leaking globally

[clintstaley, 2012/07/02 23:05:29]

It loses all its brevity, though, if I do that.  I instead tried your earlier
suggestion of relying on the JQuery $ and using $(criterion)[0].  Works
perfectly, and is concise.  OK to go with that?

[Dan Beam, 2012/07/03 01:23:46]

I'm fine with using jQuery as a selector engine (just passing though to
document.querySelector anyways), otherwise you're including it relatively
needlessly...

   return document.querySelector(criterion);
 }
 
+
 var controller = new function() {

[Dan Beam, 2012/07/02 21:33:10]

why are you using

  new function() {};

here?

[clintstaley, 2012/07/02 23:05:29]

[amusement]  Because I had a more conventional pattern, but aa@ likes the
one-shot constructor syntax when you're making only one object.  I get the
impression that object construction styles are a matter of some debate in this
language.

[Dan Beam, 2012/07/03 01:23:46]

Generally we do stuff like:

  cr.define('namespace', function() {
    var localStaticData = 1;

    function privateStatic() {
      localStaticData += 1;
    }

    function publicStatic() {
      return localStaticData;
    }

    /** @constructor */
    function class() {
      var inst = new class();
      inst.__proto__ = class.prototype;
      return inst;
    }

    class.prototype = {
      __proto__: OptionalSuperClass,

      publicMethod: function() {
        this.doSomethingSneaky_();
      },

      /** @protected */
      protectedMethod: function() {
        // Somebody could change me.
      },

      /** @private */
      privateMethod_: function() {
        this.doSomethingSneakier_();
      },

      /** @inheritDoc */
      inherittedMethod: function() {
        OptionalSuperClass.prototype.overriddenMethod.call(this);
        this.doSomethingForMe_();
      },

      /** @override */
      overriddenMethod: function() {
        this.ignoreSuperClassesMethod_();
      },
    };

    return {
      class: class,  // Published to namespace.class.
      publicStatic: publicStatic,  // Published to namespace.publicStatic.
    };
  });

and then

  var instance = new namespace.class();
  instance.publicMethod();

and

  namespace.publicStatic();

There are other things you can do we've set up like singletons (using
addSingletonGetter,
http://code.google.com/p/chromium/source/search?q=cr%5C.addsingletongetter) and
other stuff like static classes
(http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/resources/shar...)

I don't really think construction styles are a matter of debate if you're
comfortable with the language (not sure much of Chrome team is).

[clintstaley, 2012/07/04 00:01:03]

Perhaps you're right.  I lack the depth to comment other than to note that I've
heard varying opinions on how best to set up classes.  

The example you gave here is very informative, and I'll revise my code to
conform (though I hope it's ok to defer the actual cr.define installation until
we have this wired into the full webui).  If I understand correctly, this
approach holds static state in a closure of the class-creating function, which
makes it implicitly private. (?)  And one prefers putting instance methods into
a __proto__ to avoid repeated references to them in every object (reminiscent of
having one vtable object per class in C++?)

Why does class() call new class() on itself  and return the reference?  I'd have
thought the new namespace.class() at the bottom would suffice (?)

Also (and perhaps this is related) I found I needed this in PerformanceMonitor: 

this.__proto__ = PerformanceMonitor.prototype;

in order to call prototype-defined methods from within the constructor.  Is
there a way to avoid that, or is it just standard style to do that (redundant,
IIUC) assignment early in the constructor if you want to call prototype-defined
methods?

Is this example something in the Chrome docs, or did you just write it?  Do you
mind if I use it as a basis for in-class (university course, I mean) examples? 
I'm planning a JS course at Poly in fall, and this is a good illustration of a
lot of OO JS approaches.

-  // Tabular setup for various time ranges, giving a descriptive name, time span
-  // prior to |now|, data point resolution, and time-label frequency and format
-  // for each.
+  /**
+   * @enum {{
+   *   value: !number,
+   *   name: !string,
+   *   timeSpan: !number,
+   *   resolution: !number,
+   *   labelEvery: !number,
+   *   format: !string
+   * }}
+   *
+   * Enum for time ranges, giving a descriptive name, time span prior to |now|,
+   * data point resolution, and time-label frequency and format for each.
+   */
   this.TimeRange = {
     // Prior day, resolution of 2 min, at most 720 points,
     // Labels at 90 point (3 hour) intervals
     day: {value: 0, name: 'Last Day', timeSpan: 24 * 3600 * 1000,
         resolution: 1000 * 60 * 2, labelEvery: 90, format: 'MM-dd'},
 
     // Prior week, resolution of 15 min -- at most 672 data points
     // Labels at 96 point (daily) intervals
     week: {value: 1, name: 'Last Week', timeSpan: 7 * 24 * 3600 * 1000,
         resolution: 1000 * 60 * 15, labelEvery: 96, format: 'M/d'},
 
     // Prior month (30 days), resolution of 1 hr -- at most 720 data points
     // Labels at 168 point (weekly) intervals
     month: {value: 2, name: 'Last Month', timeSpan: 30 * 24 * 3600 * 1000,
         resolution: 1000 * 3600, labelEvery: 168, format: 'M/d'},
 
     // Prior quarter (90 days), resolution of 3 hr -- at most 720 data points
     // Labels at 112 point (fortnightly) intervals
     quarter: {value: 3, name: 'Last Quarter', timeSpan: 90 * 24 * 3600 * 1000,
         resolution: 1000 * 3600 * 3, labelEvery: 112, format: 'M/yy'},
   };
 
-  // Parent container of all line graphs
-  this.chartDiv = $('#charts');
-
-  // Parent container of checkboxes to choose metrics to display
-  this.chooseMetricsDiv = $('#chooseMetrics');
-
-  // Parent container of checkboxes to choose event types to display
-  this.chooseEventsDiv = $('#chooseEvents');
-
-  // Parent container of radio buttons to select time range
-  this.timeDiv = $('#chooseTimeRange');
-
-  this.metricMap = {}; // MetricName => {div, lineChart, dataTable} objects
-  this.eventMap = {};  // EventName => event point lists, as returned by webui
-  this.intervals = []; // Array of objects {start, end}
-
+  /** @type {Object.<string, {

[Dan Beam, 2012/07/02 21:33:10]

move @type down a line past /**

[clintstaley, 2012/07/02 23:05:29]

Done.

+   *                   divs: !Array.<DOMElement>,
+   *                   yAxis: !{max: !number, color: !string},
+   *                   data: ?Array.<{time: !number, value: !number}>,
+   *                   description: !string,
+   *                   units: !string
+   *                 }>}
+   *

[Dan Beam, 2012/07/02 21:33:10]

nit: we usually put a method/members's description first and then all
@type/@param/@return/@private after this with no newline in the middle.

[clintstaley, 2012/07/02 23:05:29]

Done.

+   * All metrics have entries, but those not displayed have an empty div list.
+   * If a div list is not empty, the associated data will be non-null, or
+   * null but about to be filled by webui response.  Thus, any metric with

[Dan Beam, 2012/07/02 21:33:10]

only 1 space between sentences in comments (everywhere)

[clintstaley, 2012/07/02 23:05:29]

Oh all right.  But comments are monospace font, and I think the two spaces are
needed.  (This is a bit of a religious war, though, so I will shaddap and just
follow the rules :))

[Dan Beam, 2012/07/03 01:24:28]

This is a convention throughout all of chrome.

[clintstaley, 2012/07/04 00:01:03]

And I'm really not objecting so much as joking about it -- it's a reasonable
rule.  Fixed throughout.

+   * non-empty div list but null data is awaiting a data response from the
+   * webui.
+   */
+  this.metricMap = {

[Dan Beam, 2012/07/02 21:33:10]

btw, are any of these methods/members only used in this file? if so, they should
be named like other @private methods/members in webui (their name
endsWithAnUnderscore_).

[clintstaley, 2012/07/02 23:05:29]

Done.

+    jankiness: {
+      divs: [],
+      yAxis: {max: 100, color: 'rgb(255, 128, 128)'},
+      data: null,
+      description: 'Jankiness',
+      units: 'milliJanks'
+    },
+    oddness: {
+      divs: [],
+      yAxis: {max: 20, color: 'rgb(0, 192, 0)'},
+      data: null,
+      description: 'Oddness',
+      units: 'kOdds'
+    }
+  };
+
+  /** @type {Object.<string, {
+   *                   divs: !Array.<DOMElement>,
+   *                   description: !string,
+   *                   color: !string,
+   *                   data: ?Array.<{time: !number, longDescription: !string}>
+   *                 }>}
+   *
+   * Similar data for events, though no yAxis info is needed since events
+   * are simply labelled markers at X locations.  Rules regarding null data
+   * with non-empty div list apply here as for metricMap above.
+   */
+  this.eventMap = {
+    wampusAttacks: {
+      divs: [],
+      description: 'Wampus Attack',
+      color: 'rgb(0, 0, 255)',
+      data: null
+    },
+    solarEclipses: {
+      divs: [],
+      description: 'Solar Eclipse',
+      color: 'rgb(255, 0, 255)',
+      data: null
+    }
+  };
+
+  /**
+   * @type {!Array.<{start: !number, end: !number}>}
+   * Time periods in which the browser was active and collecting metrics
+   * and events.
+   */
+  this.intervals = [];
+
+  /**
+   * Set up the radio button set to choose time range.  Use div#radioTemplate
+   * as a template.
+   */
+  this.setupTimeRangeChooser = function() {
+    var timeDiv = querySelect('#chooseTimeRange');
+    var radioTemplate = querySelect('#radioTemplate');
+    var controller = this;
+
+    for (var time in this.TimeRange) {
+      var timeRange = this.TimeRange[time];
+      var radio = radioTemplate.cloneNode(true);
+      var input = radio.querySelector('input');
+
+      input.value = timeRange.value;
+      input.timeRange = timeRange;
+      radio.querySelector('span').innerText = timeRange.name;
+      timeDiv.appendChild(radio);
+      timeRange.element = input;
+    }

[Dan Beam, 2012/07/02 21:33:10]

nit: +1 \n

[clintstaley, 2012/07/02 23:05:29]

Done.

+    timeDiv.addEventListener('click', function(e) {
+      if (!e.target.webkitMatchesSelector('input[type="radio"]'))
+        return;
+
+      controller.setTimeRange(e.target.timeRange);
+    });
+  };
+
+  /**
+   * Generalized function for setting up checkbox blocks for either events
+   * or metrics.  Take a div ID |divId| into which to place the checkboxes,
+   * and a map |optionMap| with values that each include a property
+   * |description|.  Set up one checkbox for each entry in |optionMap|
+   * labelled with that description.  Arrange callbacks to function |check|
+   * or |uncheck|, passing them the key of the checked or unchecked option,
+   * when the relevant checkbox state changes.
+   *
+   * @param {!string} divId Id of division into which to put checkboxes
+   * @param {!Object} optionMap map of metric/event entries
+   * @param {!function(this:Controller, Object)} check
+   *     function to select an entry (metric or event)
+   * @param {!function(this:Controller, Object)} uncheck
+   *     function to deselect an entry (metric or event)
+   */

[Dan Beam, 2012/07/02 21:33:10]

nit: no \n

[clintstaley, 2012/07/02 23:05:29]

Done.

+
+  this.setupCheckboxes = function(divId, optionMap, check, uncheck) {
+    var checkboxTemplate = querySelect('#checkboxTemplate');
+    var chooseMetricsDiv = querySelect(divId);
+    var doCheck = check.bind(this);
+    var doUncheck = uncheck.bind(this);
+
+    for (var option in optionMap) {
+      var checkbox = checkboxTemplate.cloneNode(true);
+      checkbox.querySelector('span').innerText = 'Show ' +
+          optionMap[option].description;
+      chooseMetricsDiv.appendChild(checkbox);
+
+      var input = checkbox.querySelector('input');
+      input.option = option;
+      input.addEventListener('change', function(e) {
+        if (e.target.checked)
+          doCheck(e.target.option);

[Dan Beam, 2012/07/02 21:33:10]

check.call(this, e.target.option);

[clintstaley, 2012/07/02 23:05:29]

Ah.  Thanks.  That is better.  I had earlier been trying to keep the
event-source "this" for the listener, but that was only because of my earlier
bad design.

+        else
+          doUncheck(e.target.option);

[Dan Beam, 2012/07/02 21:33:10]

uncheck.call(this, e.target.option);

[clintstaley, 2012/07/02 23:05:29]

Done.

+      });

[Dan Beam, 2012/07/02 21:33:10]

}.bind(this));

[clintstaley, 2012/07/02 23:05:29]

Done.

+    }
+  };
+
+  /**
+   * Set up just one chart in which all metrics will be displayed
+   * initially.  But, the design readily accommodates addition of
+   * new charts, and movement of metrics into those other charts.
+   */
+  this.setupMainChart = function() {
+    this.chartParent = querySelect('#charts');
+    this.charts = [document.createElement('div')];
+    this.charts[0].className = 'chart';
+    this.chartParent.appendChild(this.charts[0]);
+  };
+
+  /**
+   * Set the time range for which to display metrics and events.  For
+   * now, the time range always ends at "now", but future implementations
+   * may allow time ranges not so anchored.
+   *
+   * @param {!{start: !number, end: !number, resolution: !number}} range
+   */
   this.setTimeRange = function(range) {
     this.range = range;
-    this.end = Math.floor(new Date().getTime() / range.resolution) *
+    this.end = Math.floor(Date.now() / range.resolution) *
         range.resolution;
+
+    // Take the GMT value of this.end ("now") and subtract from it the
+    // number of minutes by which we lag GMT in the present timezone,
+    // X mS/minute.  This will show time in the present timezone.
+    this.end -= new Date().getTimezoneOffset() * 60000;
     this.start = this.end - range.timeSpan;
     this.requestIntervals();
-  }
-
-  // Return mock interval set for testing
+  };
+
+  /**
+   * Return mock interval set for testing
+   * @return {!Array.<{start: !number, end: !number}>} intervals
+   */
   this.getMockIntervals = function() {
     var interval = this.end - this.start;
+
     return [
-        {'start': this.start + interval * .1,
-        'end': this.start + interval * .2},
-        {'start': this.start + interval * .7, 'end': this.start + interval * .9}
+      { start: this.start + interval * .1,

[Dan Beam, 2012/07/02 21:33:10]

nit: no space after {

  {start: ...
   end: ...},

[clintstaley, 2012/07/02 23:05:29]

Really?  Doesn't this clash with 2-space indent rules? Or 4-space continuation
rules?  I'll change it, but I'd like to understand the rule so I don't trip over
it later.

[Dan Beam, 2012/07/03 01:29:14]

I break into a floating { when the params are too long to fit on a line:

  var someObjectWithALongName = {
    someReallyLongKeyNameThatWouldntFitEasilyInEightyChars: 1
  };

though if you can fit the names, this way is encouraged:

  a.b({c: 1, d: 2};

or middle of the road:

  someClass.doSomething({someKindaLongName: 'a string',
                         someOtherDescriptiveKey: 2});

the "no space in front of an object literal" rule is right outta that google js
style guide:
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...

[clintstaley, 2012/07/04 00:01:03]

OK.  Thanks for clarification.  Fixed in latest.

+        end: this.start + interval * .2
+      },
+      { start: this.start + interval * .7,
+        end: this.start + interval
+      }
     ];
-  }
-  // Request array of objects with start and end fields showing browser
-  // activity intervals in the specified time range
+  };
+
+  /**
+   * Request activity intervals in the current time range
+   */
   this.requestIntervals = function() {
     this.onReceiveIntervals(this.getMockIntervals());
     // Replace with: chrome.send('getIntervals', this.start, this.end,
     // this.onReceiveIntervals);
-  }
-
-  // Webui callback delivering response from requestIntervals call.  Assumes
-  // this is a new time range choice, which results in complete refresh of
-  // all metrics and event types that are currently selected.
+  };
+
+  /**
+   * Webui callback delivering response from requestIntervals call.  Assumes
+   * this is a new time range choice, which results in complete refresh of
+   * all metrics and event types that are currently selected.
+   *
+   * @param {!Array.<{start: !number, end: !number}> intervals new intervals
+   */
   this.onReceiveIntervals = function(intervals) {
     this.intervals = intervals;
-    for (var metric in this.metricMap)
-      this.refreshMetric(metric);
-    for (var eventType in this.eventMap)
-      this.refreshEvent(eventType);
-  }
-
-  // Add a new metric, and its associated linegraph. The linegraph for
-  // each metric has a discrete domain of times.  This is not continuous
-  // because of breaks for each interval of activity.  (No point in showing
-  // a lot of "dead air" when the browser wasn't running.)  Column 1 of
-  // its DataTable is the metric data, and higher numbered columns are added
-  // in pairs for each event type currently chosen.  Each pair gives the
-  // event occurence points, and mouseover detailed description for one event
-  // type.
+
+    for (var metric in this.metricMap) {
+      var metricValue = this.metricMap[metric];
+      if (metricValue.divs.length > 0)    // If we're displaying this metric.

[Dan Beam, 2012/07/02 21:33:10]

  if (metricValue.divs.length > 0)  // If we're displaying this metric.

(only 2 spaces)

[clintstaley, 2012/07/02 23:05:29]

Done.

+        this.refreshMetric(metric);
+    }
+
+    for (var eventType in this.eventMap) {
+      var eventValue = this.eventMap[eventType];
+      if (eventValue.divs.length > 0)
+        this.refreshEventType(eventType);
+    }
+  };
+
+  /**
+   * Add a new metric to the main (currently only) chart.
+   *
+   * @param {!string} metric Metric to start displaying
+   */
   this.addMetric = function(metric) {
-    if (!(metric in this.metricMap)) {
-      var div = document.createElement('div');
-
-      div.className = 'chart';
-      this.chartDiv.appendChild(div);
-
-      var table = new google.visualization.DataTable();
-      var chart = new google.visualization.LineChart(div);
-      this.metricMap[metric] = {'div': div, 'chart': chart, 'table': table};
-
-      table.addColumn('string', 'Time');  // Default domain column
-      table.addColumn('number', 'Value'); // Only numerical range column
-      for (var event in this.events)
-        this.addEventColumns(table, event);
-
-      this.refreshMetric(metric);
-    }
-  }
-
-  // Remove a metric from the UI
+    this.metricMap[metric].divs.push(this.charts[0]);
+    this.refreshMetric(metric);
+  };
+
+  /**
+   * Remove a metric from the chart(s).
+   *
+   * @param {!string} metric Metric to stop displaying
+   */
   this.dropMetric = function(metric) {
-    if (metric in this.metricMap) {
-      this.chartDiv.removeChild(this.metricMap[metric].div);
-      delete this.metricMap[metric];
-    }
-  }
-
-  // Return mock metric data points for testing
-  this.getMockDataPoints = function() {
+    var metricValue = this.metricMap[metric];
+    var affectedCharts = metricValue.divs;
+    metricValue.divs = [];
+
+    affectedCharts.forEach(this.drawChart, this);
+  };
+
+  /**
+   * Return mock metric data points for testing. Give values ranging from
+   * offset to max-offset.  (This lets us avoid direct overlap of
+   * different mock data sets in an ugly way that will die in the next
+   * version anyway.)
+   *
+   * @param {!number} max Max data value to return, less offset
+   * @param {!number} offset  Adjustment factor

[Dan Beam, 2012/07/02 21:33:10]

* @param {!number} offset Adjustment factor

(there was an extra space)

[clintstaley, 2012/07/02 23:05:29]

Done.

+   * @return {!Array.<{time: !number, value: !number}>}
+   */
+  this.getMockDataPoints = function(max, offset) {
     var dataPoints = [];
 
-    for (var x = 0; x < this.intervals.length; x++) {
-      // Rise from low 0 to high 100 every 20 min
-      for (var time = this.intervals[x].start; time <= this.intervals[x].end;
-          time += this.range.resolution)
-        dataPoints.push({'time': time, 'value': time % 1000000 / 10000});
+    for (var i = 0; i < this.intervals.length; i++) {
+      // Rise from low offset to high max-offset in 100 point steps

[Dan Beam, 2012/07/02 21:33:10]

end comments with .

[clintstaley, 2012/07/02 23:05:29]

Done, but..

Does this include sentence fragments?

e.g.   if (max < last)  // not yet done
       var total;       // running total

[Dan Beam, 2012/07/03 01:29:14]

yes, but the solution is to just write sentences when you can, :)

[clintstaley, 2012/07/04 00:01:03]

OK, will do.

+      for (var time = this.intervals[i].start; time <= this.intervals[i].end;
+          time += this.range.resolution) {

[Dan Beam, 2012/07/02 21:33:10]

+ 1 \s before time +=

[clintstaley, 2012/07/02 23:05:29]

Sorry, I'm confused.  It's a 4-space indent.. ?

[Dan Beam, 2012/07/03 01:23:46]

I've generally seen lined up with (, i.e.

  for (...; ...;
       ...) {

[clintstaley, 2012/07/04 00:01:03]

Done.

+        var numPoints = time / this.range.resolution;
+        dataPoints.push({time: time, value: offset + (numPoints % 100) *
+            (max - 2 * offset) / 100});
+      }
     }
     return dataPoints;
-  }
-
-  // Request new metric data, assuming the metric table and chart already
-  // exist.
+  };
+
+  /**
+   * Request new metric data, assuming the metric table and chart already
+   * exist.
+   *
+   * @param {!string} metric Metric for which to get data
+   */
   this.refreshMetric = function(metric) {
-    this.onReceiveMetric(metric, this.getMockDataPoints());
+    var metricValue = this.metricMap[metric];
+
+    metricValue.data = null;  // Mark metric as awaiting response.
+    this.onReceiveMetric(metric,
+        this.getMockDataPoints(metricValue.yAxis.max, 5));
     // Replace with:
     // chrome.send("getMetric", this.range.start, this.range.end,
     //  this.range.resolution, this.onReceiveMetric);
-  }
-
-  // Receive new datapoints for |metric|, and completely refresh the DataTable
-  // for that metric, redrawing the chart.  (We cannot preserve the event
-  // columns because entirely new rows may be implied by the new metric
-  // datapoints.)
+  };
+
+  /**
+   * Receive new datapoints for |metric|, convert the data to Flot-usable
+   * form, and redraw all affected charts.
+   *
+   * @param {!string} metric Metric to which |points| applies
+   * @param {!Array.<{time: !number, value: !number}>} points
+   *     new data points
+   */
   this.onReceiveMetric = function(metric, points) {
+    var metricValue = this.metricMap[metric];
+
     // Might have been dropped while waiting for data
-    if (!(metric in this.metricMap))
+    if (metricValue.divs.length == 0)
       return;
 
-    var data = this.metricMap[metric].table;
-
-    data.removeRows(0, data.getNumberOfRows());
-
-    // Traverse the points, which are in time order, and the intervals,
-    // placing each value in the interval (if any) in which it belongs.
+    var series = [];
+    metricValue.data = [series];
+
+    // Traverse the points, and the intervals, in parallel.   Both are in
+    // ascending time order.  Create a sequence of data "series" (per Flot)
+    // arrays, with each series comprising all points within a given interval.
     var interval = this.intervals[0];
     var intervalIndex = 0;
-    var valueIndex = 0;
-    var value;
-    while (valueIndex < points.length &&
+    var point;

[Dan Beam, 2012/07/02 21:33:10]

remove

[clintstaley, 2012/07/02 23:05:29]

Done.

+    var pointIndex = 0;
+    while (pointIndex < points.length &&
         intervalIndex < this.intervals.length) {
-      value = points[valueIndex++];
-      while (value.time > interval.end &&
-          intervalIndex < this.intervals.length) {
+      point = points[pointIndex++];

[Dan Beam, 2012/07/02 21:33:10]

var point = ...

(just use var even though it's not truly making a loop local var)

[clintstaley, 2012/07/02 23:05:29]

Done.

+      while (intervalIndex < this.intervals.length &&
+          point.time > interval.end) {
         interval = this.intervals[++intervalIndex]; // Jump to new interval
-        data.addRow(null, null);                    // Force gap in line chart
+        if (series.length > 0) {
+          series = [];                   // Start a new series
+          metricValue.data.push(series); // Put it on the end of the data
+        }
       }
-      if (intervalIndex < this.intervals.length && value.time > interval.start)
-        if (data.getNumberOfRows() % this.range.labelEvery == 0)
-          data.addRow([new Date(value.time).toString(this.range.format),
-                      value.value]);
-        else
-          data.addRow(['', value.value]);
-     }
-     this.drawChart(metric);
-  }
-
-  // Add a new event to all line graphs.
+      if (intervalIndex < this.intervals.length && point.time > interval.start)
+          series.push([point.time, point.value]);

[Dan Beam, 2012/07/02 21:33:10]

-2 \s

[clintstaley, 2012/07/02 23:05:29]

Done.

+    }
+
+    metricValue.divs.forEach(this.drawChart, this);
+  };
+
+  /**
+   * Add a new event to the chart(s).
+   *
+   * @param {!string} eventType type of event to start displaying
+   */
   this.addEventType = function(eventType) {
-    if (!(eventType in this.eventMap)) {
-      this.eventMap[eventType] = [];
-      this.refreshEventType(eventType);
-    }
-  }
-
-  // Return mock event point for testing
+    this.eventMap[eventType].divs = this.charts; // Events show on all charts

[Dan Beam, 2012/07/02 21:33:10]

2 spaces between ; and //

[clintstaley, 2012/07/02 23:05:29]

Done.

+    this.refreshEventType(eventType);
+  };
+
+  /*
+   * Remove an event from the chart(s)
+   *
+   * @param {!string} eventType type of event to stop displaying
+   */
+  this.dropEventType = function(eventType) {
+    var eventValue = this.eventMap[eventType];
+    var affectedCharts = eventValue.divs;
+    eventValue.divs = [];
+
+    affectedCharts.forEach(this.drawChart, this);
+  };
+
+  /**
+   * Return mock event points for testing
+   *
+   * @param {!string} eventType type of event to generate mock data for
+   * @return {!Array.<{time: !number, longDescription: !string}>}
+   */
   this.getMockEventValues = function(eventType) {
     var mockValues = [];
     for (var i = 0; i < this.intervals.length; i++) {
       var interval = this.intervals[i];
 
       mockValues.push({
           time: interval.start,
-          shortDescription: eventType,
           longDescription: eventType + ' at ' +
           new Date(interval.start) + ' blah, blah blah'});

[Dan Beam, 2012/07/02 21:33:10]

I assume this'll be real text eventually?

[clintstaley, 2012/07/02 23:05:29]

Yes, supplied from the C++ side.  The getMock... methods are all throwaways.

       mockValues.push({
           time: (interval.start + interval.end) / 2,
-          shortDescription: eventType,
           longDescription: eventType + ' at ' +
           new Date((interval.start + interval.end) / 2) + ' blah, blah blah'});
       mockValues.push({
           time: interval.end,
-          shortDescription: eventType,
           longDescription: eventType + ' at ' + new Date(interval.end) +
           ' blah, blah blah'});
     }
     return mockValues;
-  }
+  };
 
-  // Request new data for |eventType|, for times in the current range.
+  /**
+   * Request new data for |eventType|, for times in the current range.
+   *
+   * @param {!string} eventType type of event to get new data for
+   */
   this.refreshEventType = function(eventType) {
+    this.eventMap[eventType].data = null; // Mark eventType as awaiting response

[Dan Beam, 2012/07/02 21:33:10]

2 spaces in front of comment

[clintstaley, 2012/07/02 23:05:29]

Done.

     this.onReceiveEventType(eventType, this.getMockEventValues(eventType));
     // Replace with:
     // chrome.send("getEvents", eventType, this.range.start, this.range.end);
-  }
+  };
 
-  // Add an event column pair to DataTable |table| for |eventType|
-  this.addEventColumns = function(table, eventType) {
-    var annotationCol = table.addColumn({'id': eventType, type: 'string',
-        role: 'annotation'});
-    var rolloverCol = table.addColumn({'id': eventType + 'Tip', type: 'string',
-        role: 'annotationText'});
-    var values = this.eventMap[eventType];
-    var interval = this.intervals[0], intervalIndex = 0;
+  /**
+   * Receive new data for |eventType|.  If the event has been deselected while
+   * awaiting webui response, do nothing.  Otherwise, save the data directly,
+   * since events are handled differently than metrics when drawing
+   * (no "series"), and redraw all the affected charts.
+   *
+   * @param {!string} eventType type of event the new data applies to
+   * @param {!Array.<{time: !number, longDescription: !string}>} values
+   *     new event values
+   */
+  this.onReceiveEventType = function(eventType, values) {
+    var eventValue = this.eventMap[eventType];
 
-    for (var i = 0; i < values.length; i++) {
-      var event = values[i];
-      var rowIndex = 0;
-      while (event.time > interval.end &&
-          intervalIndex < this.intervals.length - 1)
-      {
-        // Skip interval times, inclusive of interval.end, and of following null
-        rowIndex += (interval.end - interval.start) / this.range.resolution + 2;
-        interval = this.intervals[++intervalIndex];
-      }
-      if (event.time >= interval.start && event.time <= interval.end) {
-        table.setCell(rowIndex + (event.time - interval.start) /
-            this.range.resolution, annotationCol, event.shortDescription);
-        table.setCell(rowIndex + (event.time - interval.start) /
-            this.range.resolution, rolloverCol, event.longDescription);
+    if (eventValue.divs.length == 0)
+      return;
+
+    eventValue.data = values;
+    eventValue.divs.forEach(this.drawChart, this);
+  };
+
+  /**
+   * Return an object containing an array of metrics and another of events
+   * that include |chart| as one of the divs into which they display.
+   *
+   * @param {DOMElement} chart div for which to get relevant items

[Dan Beam, 2012/07/02 21:33:10]

Element, HTMLElement, or HTMLDivElement (there's no DOMElement)

[clintstaley, 2012/07/02 23:05:29]

Fixed throughout.

+   * @return {!{metrics: !Array,<Object>, events: !Array.<Object>}}
+   */
+  this.getChartData = function(chart) {
+    var result = {metrics: [], events: []};
+
+    for (var metric in this.metricMap) {
+      var metricValue = this.metricMap[metric];
+
+      if (metricValue.divs.indexOf(chart) != -1)
+        result.metrics.push(metricValue);
+    }
+
+    for (var eventType in this.eventMap) {
+      var eventValue = this.eventMap[eventType];
+
+      // Events post to all divs, if they post to any

[Dan Beam, 2012/07/02 21:33:10]

. at end of comment

[clintstaley, 2012/07/02 23:05:29]

Done.

+      if (eventValue.divs.length > 0)
+        result.events.push(eventValue);
+    }
+
+    return result;
+  };
+
+  /**
+   * Check all entries in an object of the type returned from getChartData,
+   * above, to see if all events and metrics have completed data (none is
+   * awaiting an asynchronous webui response to get their current data).
+   *
+   * @param {!{metrics: !Array,<Object>, events: !Array.<Object>}} chartData
+   *     event/metric data to check for readiness
+   * @return {boolean} is data ready?
+   */
+  this.isDataReady = function(chartData) {
+    for (var i = 0; i < chartData.metrics.length; i++) {
+      if (chartData.metrics[i].data == null)

[Dan Beam, 2012/07/02 21:33:10]

is there a reason you're loosely checking if it's null vs. just:

  if (!chartData.metrics[i].data)

?

[clintstaley, 2012/07/02 23:05:29]

Unfamiliarity with JS :).  Wasn't sure what negation of null resulted in.

Done.

+        return false;
+    }
+
+    for (var i = 0; i < chartData.events.length; i++) {
+      if (chartData.events[i].data == null)
+        return false;
+    }
+
+    return true;
+  };
+
+  /**
+   * Create and return an array of "markings" (per Flot), representing
+   * vertical lines at the event time, in the event's color.  Also add
+   * (not per Flot) a |description| property to each, to be used for hand
+   * creating description boxes.
+   *
+   * @param {!Array.<{
+   *                   description: !string,
+   *                   color: !string,
+   *                   data: !Array.<{time: !number}>
+   *                 }>} eventValues events to make markings for
+   * @return {!Array.<{
+   *                   color: !string,
+   *                   description: !string,
+   *                   xaxis: {from: !number, to: !number}
+   *                 }>} mark data structure for Flot to use
+   */
+  this.getEventMarks = function(eventValues) {
+    var markings = [];
+
+    for (var i = 0; i < eventValues.length; i++) {
+      var eventValue = eventValues[i];
+      for (var d = 0; d < eventValue.data.length; d++) {
+        var point = eventValue.data[d];
+        markings.push({
+          color: eventValue.color,
+          description: eventValue.description,
+          xaxis: {from: point.time, to: point.time}
+        });
       }
     }
-  }
 
-  this.dropEventColumns = function(table, eventType) {
-    var colIndex, numCols = table.getNumberOfColumns();
+    return markings;
+  };
 
-    for (colIndex = 0; colIndex < numCols; colIndex++)
-      if (table.getColumnId(colIndex) == eventType)
-        break;
+  /**
+   * Redraw the chart in div |chart|, IF all its dependent data is present.

[Dan Beam, 2012/07/02 21:33:10]

s/IF/if/

[clintstaley, 2012/07/02 23:05:29]

That's emphasis, not a misspelling.  Is there an accepted  way to show such in a
doc?  *if* perhaps?

[Dan Beam, 2012/07/03 01:29:14]

sure, that works

+   * Otherwise simply return, and await another call when all data is
+   * available.
+   *
+   * @param {DOMElement} chart div to redraw

[Dan Beam, 2012/07/02 21:33:10]

same type nit as above (no DOMElement)

[clintstaley, 2012/07/02 23:05:29]

Done.

+   */
+  this.drawChart = function(chart) {
+    var chartData = this.getChartData(chart);
 
-    if (colIndex < numCols) {
-       table.removeColumn(colIndex + 1);
-       table.removeColumn(colIndex);
+    if (!this.isDataReady(chartData))
+      return;
+
+    var seriesSeq = [];
+    var yAxes = [];
+    chartData.metrics.forEach(function(value) {
+      yAxes.push(value.yAxis);
+      for (var i = 0; i < value.data.length; i++) {
+        seriesSeq.push({
+          color: value.yAxis.color,
+          data: value.data[i],
+          label: i == 0 ? value.description + ' (' + value.units + ')' : null,
+          yaxis: yAxes.length,  // Use just-added Y axis

[Dan Beam, 2012/07/02 21:33:10]

. at end.

[clintstaley, 2012/07/02 23:05:29]

Fixed these throughout -- about a dozen cases -- sorry.  But, still want to
understand whether this is only required for full sentences.  

+        });
+      }
+    });
+
+    var markings = this.getEventMarks(chartData.events);
+    var chart = this.charts[0];
+    var plot = $.plot(chart, seriesSeq, {
+      yaxes: yAxes,
+      xaxis: {mode: 'time'},
+      grid: {markings: markings}
+    });
+
+    // For each event in |markings|, create also a label div, with left
+    // edge colinear with the event vertical-line.  Top of label is
+    // presently a hack-in, putting labels in three tiers of 25px height
+    // each to avoid overlap.  Will need something better.
+    var labelTemplate = querySelect('#labelTemplate');
+    for (var i = 0; i < markings.length; i++) {
+      var mark = markings[i];
+      var point =
+          plot.pointOffset({x: mark.xaxis.to, y: yAxes[0].max, yaxis: 1});
+      var labelDiv = labelTemplate.cloneNode(true);
+      labelDiv.innerText = mark.description;
+      labelDiv.style.left = point.left + 'px';
+      labelDiv.style.top = (point.top + 25 * (i % 3)) + 'px';
+      chart.appendChild(labelDiv);
     }
-  }
+  };
 
-  // Receive new data for |eventType|.  Save this in eventMap for future
-  // redraws.  Then, for each metric linegraph remove any current column pair
-  // for |eventType| and replace it with a new pair, which will reflect the
-  // new data.  Redraw the linegraph.
-  this.onReceiveEventType = function(eventType, values) {
-    this.eventMap[eventType] = values;
-
-    for (var metric in this.metricMap) {
-      var table = this.metricMap[metric].table;
-
-      this.dropEventColumns(table, eventType);
-      this.addEventColumns(table, eventType);
-      this.drawChart(metric);
-    }
-  }
-
-  this.dropEventType = function(eventType) {
-    delete this.eventMap[eventType];
-
-    for (var metric in this.metricMap) {
-      var table = this.metricMap[metric].table;
-
-      this.dropEventColumns(table, eventType);
-      this.drawChart(metric);
-    }
-  }
-
-
-  // Redraw the linegraph for |metric|, assuming its DataTable is fully up to
-  // date.
-  this.drawChart = function(metric) {
-    var entry = this.metricMap[metric];
-
-    entry.chart.draw(entry.table, {title: metric + ' for ' + this.range.name,
-        hAxis: {showTextEvery: this.range.labelEvery}});
-  }
-
-  this.setupTimeRangeChooser = function() {
-    var controller = this;
-    var radioTemplate = $('#radioTemplate');
-
-    for (var time in this.TimeRange) {
-       var range = this.TimeRange[time];
-       var radio = radioTemplate.cloneNode(true);
-       var input = radio.querySelector('input');
-
-       input.value = range.value;
-       radio.querySelector('span').innerText = range.name;
-       this.timeDiv.appendChild(radio);
-       range.element = input;
-       radio.range = range;
-       radio.addEventListener('click', function() {
-          controller.setTimeRange(this.range);
-       });
-    }
-  }
-
-  this.setupMetricChooser = function(metricTypes) {
-    var checkboxTemplate = $('#checkboxTemplate');
-
-    metricTypes.forEach(function(metric) {
-      var checkbox = checkboxTemplate.cloneNode(true);
-      var input = checkbox.querySelector('input');
-      input.addEventListener('change', function() {
-        if (input.checked)
-          this.addMetric(metric);
-        else
-          this.dropMetric(metric);
-        }.bind(this));
-      checkbox.getElementsByTagName('span')[0].innerText = 'Show ' + metric;
-      this.chooseMetricsDiv.appendChild(checkbox);
-    }, this);
-  }
-
-  this.setupEventChooser = function(eventTypes) {
-    var checkboxTemplate = $('#checkboxTemplate');
-
-    eventTypes.forEach(function(event) {
-      var checkbox = checkboxTemplate.cloneNode(true);
-      var input = checkbox.querySelector('input');
-      input.addEventListener('change', function() {
-        if (input.checked)
-          this.addEventType(event);
-        else
-          this.dropEventType(event);
-        }.bind(this));
-      checkbox.getElementsByTagName('span')[0].innerText = 'Show ' + event;
-      this.chooseEventsDiv.appendChild(checkbox);
-    }, this);
-  }
-
-  this.setupMetricChooser(['Oddness', 'Jankiness']);
-  this.setupEventChooser(['Wampus Attack', 'Solar Eclipse']);
+  this.setupCheckboxes(
+      '#chooseMetrics', this.metricMap, this.addMetric, this.dropMetric);
+  this.setupCheckboxes(
+      '#chooseEvents', this.eventMap, this.addEventType, this.dropEventType);
   this.setupTimeRangeChooser();
+  this.setupMainChart();
   this.TimeRange.day.element.click();
 }

[Dan Beam, 2012/07/02 21:33:10]

};

[clintstaley, 2012/07/02 23:05:29]

lint tool dings me for this, oddly.

[Dan Beam, 2012/07/03 01:23:46]

which tool? the js presubmit check?