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 docGet(criterion) {

[Dan Beam, 2012/06/28 21:39:14]

why did you change the name here to docGet? that seems like it'd return the
document itself or the owner document for a node...  additionally every single
use of docGet() (that I can tell) simply uses '#id-name', so can you simply
change this to document.getElementById() or include
src/chrome/browser/resources/shared/js/util.js via

  <script src="chrome://resources/js/util.js"></script>

from the HTML file that embeds this .js (preferred) or by:

  <include "../shared/js/util.js">

right after 'use strict';?

[clintstaley, 2012/06/29 22:34:20]

This was originally a hand-written $ function, cause I didn't know about
util.js.  But, in any event the I believe the $ in util.js will clash with the $
in JQuery, which latter $ produces not an Element but a wrapper around an
Element, or at least a decorated Element IIUC.  Given this, can I even use
util.js??  And, as for getElementById, aa@ dings me for that and asks for use of
querySelector, instead.  I can't win. :).

At any rate, I'll rename the (badly) named function, and await further comments.

[Dan Beam, 2012/06/29 23:18:59]

You *could* call jQuery.noConflict() <http://api.jquery.com/jQuery.noConflict/>
which returns $ to the original reference, and then do something along the lines
of:

  (function($) {
    // use $ as much as you'd like
  }(jQuery));

if you want to leave the existing code using $ in both places (though I
generally wouldn't recommend using two widely know [semi-]globals
interchangeably).

I'm also OK with reviewing jQuery-esque code, so if you want to just change to
using jQuery/Sizzle as your selector engine that's fine with me, I'll know what
you mean.  That way you can just say $('#blah') (and add a "[0]" if you want the
raw DOM node).
I would use querySelector() if I thought I'd ever need to change away from just
an ID, *but* Chrome's general philosophy is code for the now (as far as I
understand) so I'd assume somebody would tell me to only use getElementById()
(so I passed this on to you).  It's not bad to leave as querySelector() as it
allows more flexibility and if aa@ asked for it we can simply leave it be.
We use $ (which is a worse name) everywhere as well, :).

   return document.querySelector(criterion);
 }
 
+
 var controller = new function() {
   // 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.
   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');
+  // All metrics have entries, but those not displayed have empty div list.

[Dan Beam, 2012/06/28 21:39:14]

have an empty div list?

[clintstaley, 2012/06/29 22:34:20]

I tend to omit definite articles in phrases qualifying a plural, but I admit it
reads a little oddly.  Fixed.

+  // If a div list is nonempty, the associated data will be nonull, or null but

[Dan Beam, 2012/06/28 21:39:14]

not empty, non-null

[clintstaley, 2012/06/29 22:34:20]

Done.

+  // about to be filled by webui response.  Any metric with nonempty div

[Dan Beam, 2012/06/28 21:39:14]

I think all these object literals would be easier to read if indented like so:

  this.metricMap = {
    jankiness: {
      data: null,
      description: 'Jankiness',
      units: 'milliJanks'
    },
    ...

[clintstaley, 2012/06/29 22:34:20]

Done.

+  // list but null data is awaiting a data response from the webui.
+  this.metricMap = {
+    jankiness: {divs: [], yAxis: {max: 100, color: 'rgb(255, 128, 128)'},
+        data: null, description: 'Jankiness', units: 'milliJanks'},

[Dan Beam, 2012/06/28 21:39:14]

I think it'd be easier to read if indented like so:

  this.metricMap = {
    jankiness: {
      data: null,
      description: 'Jankiness',
      units: 'milliJanks'
    },
    ...

[clintstaley, 2012/06/29 22:34:20]

Done.

+    oddness: {divs: [], yAxis: {max: 20, color: 'rgb(0, 192, 0)'},
+        data: null, description: 'Oddness', units: 'kOdds'}
+  };
 
-  // Parent container of checkboxes to choose metrics to display
-  this.chooseMetricsDiv = $('#chooseMetrics');
+  // Similar data for events, though no yAxis info is needed since events
+  // are simply labelled markers at X locations.  Rules regarding null data
+  // with nonempty 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}
+  };
 
-  // Parent container of checkboxes to choose event types to display
-  this.chooseEventsDiv = $('#chooseEvents');
+  // Array of objects {start, end} representing times the browser was active
+  // and collecting metrics and events.
+  this.intervals = [];
 
-  // 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}
+  // Set up the radio button set to choose time range.  Use div#radioTemplate
+  // as a model.

[Dan Beam, 2012/06/28 21:39:14]

as a template. (model implies something else)

[clintstaley, 2012/06/29 22:34:20]

Done.

+  this.setupTimeRangeChooser = function() {
+    var timeDiv = docGet('#chooseTimeRange');
+    var radioTemplate = docGet('#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;
+      radio.querySelector('span').innerText = timeRange.name;
+      timeDiv.appendChild(radio);
+      timeRange.element = input;
+      radio.timeRange = timeRange;
+      radio.addEventListener('click', function() {

[Dan Beam, 2012/06/28 21:39:14]

instead of creating an event listener for each radio, it'd seem like you could
delegate these event handlers higher in the DOM and take advantage of event
propagation, e.g.:

  timeDiv.addEventListener('click', function(e) {
    if (!e.target.webkitMatchesSelector('input[type="radio"]'))
      return;

    controller.setTimeRange(e.target.timeRange);
  });

when you click anywhere in timeDiv we check to see if it was a click on a radio,
and if it was, we use e.target (which is the radio) to set the time range.

[clintstaley, 2012/06/29 22:34:20]

Done.

+        controller.setTimeRange(this.timeRange);
+      });
+    }
+  };
+
+  // Generalized function for setting up checkbox blocks for either events
+  // or metrics.  Take a div ID |divId| into which to place the checkboxes,

[Dan Beam, 2012/06/28 21:39:14]

comments describing the parameters to this function are more standardized using
JSDoc notation, i.e.

  /**
   * @param {string} divId The ID of the div to set up.
   */

(and on). Here's a guide to how to do this -
https://developers.google.com/closure/compiler/docs/js-for-compiler - we do it
pretty much everywhere else in WebUI code.

[clintstaley, 2012/06/29 22:34:20]

thanks.  I'd been wondering about these, but didn't have the reference.  Fixed
throughout.

+  // 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.
+  this.setupCheckboxes = function(divId, optionMap, check, uncheck) {
+    var checkboxTemplate = docGet('#checkboxTemplate');
+    var chooseMetricsDiv = docGet(divId);
+
+    for (var option in optionMap) {
+      var checkbox = checkboxTemplate.cloneNode(true);
+      checkbox.getElementsByTagName('span')[0].innerText = 'Show ' +

[Dan Beam, 2012/06/28 21:39:14]

you could also just .querySelector('span') here

[clintstaley, 2012/06/29 22:34:20]

Done.

+          optionMap[option].description;
+      chooseMetricsDiv.appendChild(checkbox);
+
+      var input = checkbox.querySelector('input');
+      input.addEventListener('change', function(check, uncheck) {
+        if (this.checked)
+          check();
+        else
+          uncheck();
+      }.bind(input, check.bind(this, option), uncheck.bind(this, option)));

[Dan Beam, 2012/06/28 21:39:14]

why are you doing all these binds?

[clintstaley, 2012/06/29 22:34:20]

Cause I'm an idiot :).  More specifically, I'm new enough at this that I built
from an example where the event handler had no parameters, not realizing there's
an alternative where it has an Event parameter, as you show above.  Rewriting
with that vastly reduces the bind-cuteness.

+    }
+  };
+
+  /* Outdated specialized functions replaced by setupCheckboxes.  Included

[Dan Beam, 2012/06/28 21:39:14]

I'm confused, what do you want me to do with this?  Generally I only publish for
review what I think is ready to be committed right after LG, so making something
like this makes you modify the code after somebody reviews or makes somebody
re-review again.

[clintstaley, 2012/06/29 22:34:20]

The function with all the binds could be viewed as too "fancy".  I wanted the
reviewer to have the chance to say "No, just do this in a somewhat repetitious
but straightforward way".  I'll drop these now that the other function is less
intricate.

+  here in case the reviewer prefers them.  Will be deleted before landing CL
+  otherwise.
+  this.setupMetricChooser = function() {
+    var checkboxTemplate = docGet('#checkboxTemplate');
+    var chooseMetricsDiv = docGet('#chooseMetrics');
+    var controller = this;
+
+    for (var metric in this.metricMap) {
+      var checkbox = checkboxTemplate.cloneNode(true);
+      checkbox.getElementsByTagName('span')[0].innerText = 'Show ' +
+          this.metricMap[metric].description;
+      chooseMetricsDiv.appendChild(checkbox);
+
+      var input = checkbox.querySelector('input');
+      input.metric = metric;
+      input.addEventListener('change', function() {
+        if (this.checked)
+          controller.addMetric(this.metric);
+        else
+          controller.dropMetric(this.metric);
+      });
+    }
+  };
+
+  this.setupEventChooser = function() {
+    var checkboxTemplate = docGet('#checkboxTemplate');
+    var chooseEventsDiv = docGet('#chooseEvents');
+    var controller = this;
+
+    for (var event in this.eventMap) {
+      var checkbox = checkboxTemplate.cloneNode(true);
+      checkbox.getElementsByTagName('span')[0].innerText = 'Show ' +
+          this.eventMap[event].description;
+      chooseEventsDiv.appendChild(checkbox);
+
+      var input = checkbox.querySelector('input');
+      input.event = event;
+      input.addEventListener('change', function() {
+        if (this.checked)
+          controller.addEventType(this.event);
+        else
+          controller.dropEventType(this.event);
+      });
+    }
+  };
+  */
+
+  // 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 = docGet('#charts');
+    this.charts = [document.createElement('div')];
+    this.charts[0].className = 'chart';
+    this.chartParent.appendChild(this.charts[0]);
+  }

[Dan Beam, 2012/06/28 21:39:14]

all function expressions (something = function(){};) should end with a ; after
the end curly }, so this would be:

  this.setupMainChart = function() {
    // ...
  };

[clintstaley, 2012/06/29 22:34:20]

Done.

+
+  // 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.
   this.setTimeRange = function(range) {
     this.range = range;
     this.end = Math.floor(new Date().getTime() / range.resolution) *

[Dan Beam, 2012/06/28 21:39:14]

instead of new Date().getTime() -> Date.now()

[clintstaley, 2012/06/29 22:34:20]

Done.

         range.resolution;
+
+    // Do an offset to accommodate Flot, which has no timezone management
+    this.end -= new Date().getTimezoneOffset() * 60000;

[Dan Beam, 2012/06/28 21:39:14]

what is this doing?

[clintstaley, 2012/06/29 22:34:20]

Commment expanded upon...

     this.start = this.end - range.timeSpan;
     this.requestIntervals();
   }
 
   // Return mock interval set for testing
   this.getMockIntervals = function() {
     var interval = this.end - this.start;
+
     return [
         {'start': this.start + interval * .1,

[Dan Beam, 2012/06/28 21:39:14]

remove quotes around 'key': names and line things up like this if they need to
wrap and you don't want to expand like I mentioned above (putting new lines like
in pretty printed JSON):

  {start: this.start + interval * .1,
   end: this.start + interval * .2},

[clintstaley, 2012/06/29 22:34:20]

Done.

         'end': this.start + interval * .2},
-        {'start': this.start + interval * .7, 'end': this.start + interval * .9}
+        {'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 specified time range
   this.requestIntervals = function() {
     this.onReceiveIntervals(this.getMockIntervals());
     // Replace with: chrome.send('getIntervals', this.start, this.end,

[Dan Beam, 2012/06/28 21:39:14]

when will you do this?

[clintstaley, 2012/06/29 22:34:20]

Next phase, once we get the UI code OK'ed and landed, and once we get the webui
code that supports it landed (nearly there now)

[Dan Beam, 2012/06/29 23:18:59]

OK.

     // 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.
   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.
-  this.addMetric = function(metric) {
-    if (!(metric in this.metricMap)) {
-      var div = document.createElement('div');
+    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/06/28 21:39:14]

nit: no curlies for 1 line if (/* conditional */) and { /* expression */ } (and
any number of else if () {} and else {} statements afterward), i.e.

  if (who)
    what();
  else if (where)
    when();
  else
    why();

[clintstaley, 2012/06/29 22:34:20]

Done.

+        this.refreshMetric(metric);
+      }
+    }
 
-      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);
+    for (var eventType in this.eventMap) {
+      var eventValue = this.eventMap[eventType];
+      if (eventValue.divs.length > 0) {

[Dan Beam, 2012/06/28 21:39:14]

nit: no curlies

[clintstaley, 2012/06/29 22:34:20]

Done.

+        this.refreshEventType(eventType);
+      }
     }
   }
 
-  // Remove a metric from the UI
-  this.dropMetric = function(metric) {
-    if (metric in this.metricMap) {
-      this.chartDiv.removeChild(this.metricMap[metric].div);
-      delete this.metricMap[metric];
-    }
+  // Add a new metric to the main (currently only) chart.
+  this.addMetric = function(metric) {
+    this.metricMap[metric].divs.push(this.charts[0]);
+    this.refreshMetric(metric);
   }
 
-  // Return mock metric data points for testing
-  this.getMockDataPoints = function() {
+  // Remove a metric from the chart(s).
+  this.dropMetric = function(metric) {
+    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 let us avoid direct overlap of
+  // different mock data sets in an ugly way that will die in the next
+  // version anyway.)
+  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
+      // Rise from low offset to high max-offset in 100 point steps
       for (var time = this.intervals[x].start; time <= this.intervals[x].end;
           time += this.range.resolution)
-        dataPoints.push({'time': time, 'value': time % 1000000 / 10000});
+        dataPoints.push({'time': time, 'value': offset + time /
+            this.range.resolution % 100 * (max - 2 * offset) / 100});
     }
     return dataPoints;
   }
 
   // Request new metric data, assuming the metric table and chart already
   // exist.
   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.
   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;
+    var series = [];
+    metricValue.data = [series];
 
-    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.
+    // 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;
+    var pointIndex = 0;
+    while (pointIndex < points.length &&
         intervalIndex < this.intervals.length) {
-      value = points[valueIndex++];
-      while (value.time > interval.end &&
+      point = points[pointIndex++];
+      while (point.time > interval.end &&
           intervalIndex < this.intervals.length) {
         interval = this.intervals[++intervalIndex]; // Jump to new interval
-        data.addRow(null, null);                    // Force gap in line chart
+        if (series.length > 0)
+          metricValue.data.push(series = []);
       }
-      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);
+      if (intervalIndex < this.intervals.length && point.time > interval.start)
+          series.push([point.time, point.value]);
+    }
+
+    metricValue.divs.forEach(this.drawChart, this);
   }
 
-  // Add a new event to all line graphs.
+  // Add a new event to the chart(s).
   this.addEventType = function(eventType) {
-    if (!(eventType in this.eventMap)) {
-      this.eventMap[eventType] = [];
-      this.refreshEventType(eventType);
-    }
+    this.eventMap[eventType].divs = this.charts; // Events show on all charts
+    this.refreshEventType(eventType);
   }
 
-  // Return mock event point for testing
+  // Remove an event from the chart(s)
+  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
   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'});
       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.
   this.refreshEventType = function(eventType) {
+    this.eventMap[eventType].data = null; // Mark eventType as awaiting response
     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.
+  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 (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.
+  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];
+
+      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).
+  this.isDataReady = function(chartData) {
+    for (var x = 0; x < chartData.metrics.length; x++)

[Dan Beam, 2012/06/28 21:39:14]

nit: curlies around the for loop (as it's more than 1 line)

[clintstaley, 2012/06/29 22:34:20]

But it's one statement :)

Done

+      if (chartData.metrics[x].data == null)
+        return false;
+
+    for (var x = 0; x < chartData.events.length; x++)

[Dan Beam, 2012/06/28 21:39:14]

nit: see above, also why are you using x as a loop variable?

[clintstaley, 2012/06/29 22:34:20]

Seen it done other places; never sure what the rules are for naming loop vars. 
Is the the one-letter (you didn't ding me for "i") or the implied geometry of an
"x"?  For now, I've changed to "i" where I used x.

[Dan Beam, 2012/06/29 23:18:59]

implied geometry, ya (you're also using {x,y}axis in this file a lot)

+      if (chartData.events[x].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.
+  this.getEventMarks = function(eventValues) {
+    var markings = [];
+
+    for (var x = 0; x < eventValues.length; x++) {
+      var eventValue = eventValues[x];
+      for (var d = 0; d < eventValue.data.length; d++) {
+        var point = eventValue.data[d];
+        markings.push({xaxis: {from: point.time, to: point.time},

[Dan Beam, 2012/06/28 21:39:14]

I think it'd be cleaner to read if there were line breaks here, i.e.:

  markings.push({
    color: eventValue.color,
    description: eventValue.description,
    xaxis: {from: point.time, to: point.time},
  });

[clintstaley, 2012/06/29 22:34:20]

Done.

+            color: eventValue.color, description: eventValue.description});
       }
-      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);
+    }
+
+    return markings;
+  }
+
+  // Redraw the chart in div |chart|, IF all its dependent data is present.
+  // Otherwise simply return, and await another call when all data is
+  // available.
+  this.drawChart = function(chart) {
+    var chartData = this.getChartData(chart);
+    var seriesSeq = [];
+    var yAxes = [];
+
+    if (!this.isDataReady(chartData))

[Dan Beam, 2012/06/28 21:39:14]

move this if () to before:

  var seriesSeq = [];
  var yAxes = [];

[clintstaley, 2012/06/29 22:34:20]

Done.

+      return;
+
+    chartData.metrics.forEach(function(value) {
+      yAxes.push(value.yAxis);
+      for (var run = 0; run < value.data.length; run++) {
+        seriesSeq.push({

[Dan Beam, 2012/06/28 21:39:14]

I think only 2 spaces indented here (instead of 4)

[clintstaley, 2012/06/29 22:34:20]

Yeah, wasn't sure if it counted as a line continuation or a level of
indentation.  Done.

+            color: value.yAxis.color,
+            data: value.data[run],
+            label: run == 0 ? value.description + '(' + value.label + ')' :
+                null,
+            yaxis: yAxes.length,  // Use just-added Y axis
+        });
       }
+    });
+
+    var markings = this.getEventMarks(chartData.events);
+    var chart = this.charts[0];
+    var plot = $.plot(chart, seriesSeq, {

[Dan Beam, 2012/06/28 21:39:14]

same here about 2 \s instead of 4

[clintstaley, 2012/06/29 22:34:20]

Done.

+        yaxes: yAxes,
+        xaxis: {mode: 'time'},
+        grid: {markings: markings}});
+
+    // Fore 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 = docGet('#labelTemplate');
+    for (var x = 0; x < markings.length; x++) {
+      var mark = markings[x];
+      var point = plot.pointOffset({x: mark.xaxis.to, y: yAxes[0].max,

[Dan Beam, 2012/06/28 21:39:14]

optional nit: may be easier to read if you do this instead:

 var point =
     plot.pointOffset({x: mark.xaxis.to, y: yAxes[0].max, yaxis: 1});

[clintstaley, 2012/06/29 22:34:20]

Done.

+          yaxis: 1});
+      var labelDiv = labelTemplate.cloneNode(true);
+      labelDiv.innerText = mark.description;
+      labelDiv.style.left = point.left + 'px';
+      labelDiv.style.top = (point.top + 25 * (x % 3)) + 'px';
+      chart.appendChild(labelDiv);
     }
   }
 
-  this.dropEventColumns = function(table, eventType) {
-    var colIndex, numCols = table.getNumberOfColumns();
-
-    for (colIndex = 0; colIndex < numCols; colIndex++)
-      if (table.getColumnId(colIndex) == eventType)
-        break;
-
-    if (colIndex < numCols) {
-       table.removeColumn(colIndex + 1);
-       table.removeColumn(colIndex);
-    }
-  }
-
-  // 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,

[Dan Beam, 2012/06/28 21:39:14]

optional nit: same general readability comment here

  this.setupCheckboxes(
      '#chooseEvents', this.eventMap, this.addEventType,this.dropMetric);

[clintstaley, 2012/06/29 22:34:20]

Done.

+      this.dropMetric);
+  this.setupCheckboxes('#chooseEvents', this.eventMap, this.addEventType,
+      this.dropEventType);
   this.setupTimeRangeChooser();
+  this.setupMainChart();
   this.TimeRange.day.element.click();
 }