DevTools: Refactor memory counter display

Source/devtools/front_end/timeline/CountersGraph.js

 /*
  * Copyright (C) 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 20 matching lines @@
 /**
  * @constructor
  * @extends {WebInspector.SplitWidget}
  * @implements {WebInspector.TimelineModeView}
  * @param {string} title
  * @param {!WebInspector.TimelineModeViewDelegate} delegate
  * @param {!WebInspector.TimelineModel} model
  */
 WebInspector.CountersGraph = function(title, delegate, model)
 {
-    WebInspector.SplitWidget.call(this, true, false);
+    WebInspector.SplitWidget.call(this, true, false, "memoryCountersSidebar");
 
     this.element.id = "memory-graphs-container";
 
     this._delegate = delegate;
     this._model = model;
     this._calculator = new WebInspector.TimelineCalculator(this._model);
 
     this._graphsContainer = new WebInspector.VBox();
     this.setMainWidget(this._graphsContainer);
     this._createCurrentValuesBar();
@@ skipping 343 matching lines @@
  * @param {string} title
  * @param {string} currentValueLabel
  * @param {string} graphColor
  * @param {!WebInspector.CountersGraph.Counter} counter
  */
 WebInspector.CountersGraph.CounterUI = function(memoryCountersPane, title, currentValueLabel, graphColor, counter)
 {
     this._memoryCountersPane = memoryCountersPane;
     this.counter = counter;
     var container = memoryCountersPane._infoWidget.element.createChild("div", "memory-counter-sidebar-info");
-    var swatchColor = graphColor;
-    this._swatch = new WebInspector.SwatchCheckbox(WebInspector.UIString(title), swatchColor);
-    this._swatch.addEventListener(WebInspector.SwatchCheckbox.Events.Changed, this._toggleCounterGraph.bind(this));
-    container.appendChild(this._swatch.element);
-    this._range = this._swatch.element.createChild("span");
+
+    this._filter = new WebInspector.CheckboxFilterUI(title, WebInspector.UIString(title));

[alph, 2015/08/24 17:19:50]

No need to use UIString here. It should be used right on the string literal.

[paulirish, 2015/08/25 23:18:46]

Done

+    var color = WebInspector.Color.parse(graphColor).setAlpha(0.5).asString(WebInspector.Color.Format.RGBA);
+    if (color !== null)

[alph, 2015/08/24 17:19:50]

if (color)

[paulirish, 2015/08/25 23:18:46]

Done

+        this._filter.setColor(color, "rgba(0,0,0,0.3)");
+    this._filter.addEventListener(WebInspector.FilterUI.Events.FilterChanged, this._toggleCounterGraph.bind(this));
+    container.appendChild(this._filter.element());
+    this._range = this._filter.labelElement().createChild("span", "range");

[alph, 2015/08/24 17:19:50]

I see the span text goes one pixel higher than the label.
Btw you can just add the span after the _filter element, so you don't need to
expose the label

[paulirish, 2015/08/25 23:18:46]

Tried that but that makes the label clickable and the range not. It's a little
awkward, so I prefer having the entire title be clickable for the checkbox.

 
     this._value = memoryCountersPane._currentValuesBar.createChild("span", "memory-counter-value");
     this._value.style.color = graphColor;
     this.graphColor = graphColor;
     this.limitColor = WebInspector.Color.parse(graphColor).setAlpha(0.3).asString(WebInspector.Color.Format.RGBA);
     this.graphYValues = [];
     this._verticalPadding = 10;
 
     this._currentValueLabel = currentValueLabel;
     this._marker = memoryCountersPane._canvasContainer.createChild("div", "memory-counter-marker");
     this._marker.style.backgroundColor = graphColor;
     this._clearCurrentValueAndMarker();
 }
 
 WebInspector.CountersGraph.CounterUI.prototype = {
     reset: function()
     {
         this._range.textContent = "";
     },
 
     /**
      * @param {number} minValue
      * @param {number} maxValue
+     * @param {!WebInspector.CountersGraph.Counter} counter
      */
-    setRange: function(minValue, maxValue)
+    setRange: function(minValue, maxValue, counter)
     {
-        this._range.textContent = WebInspector.UIString("[%.0f:%.0f]", minValue, maxValue);
+        if (counter == this._memoryCountersPane.counters("jsHeapSizeUsed")) {

[alph, 2015/08/24 17:19:50]

There should be no knowledge of a particular counter in the base class.
You can pass a custom formatter function to the constructor instead.

[paulirish, 2015/08/25 23:18:46]

good idea. Does the new patch handle this how you'd expect?

+            var min = Number.bytesToString(minValue);
+            var max = Number.bytesToString(maxValue);
+        } else {
+            var min = Number.withThousandsSeparator(minValue);
+            var max = Number.withThousandsSeparator(maxValue);
+        }
+        this._range.textContent = WebInspector.UIString("[%s\u2009\u2013\u2009%s]", min, max);
     },
 
     _toggleCounterGraph: function(event)

[alph, 2015/08/24 17:19:50]

nit: could you please annotate.

[paulirish, 2015/08/25 23:18:46]

Done.

     {
-        this._value.classList.toggle("hidden", !this._swatch.checked);
+        this._value.classList.toggle("hidden", !this._filter.checked());
         this._memoryCountersPane.refresh();
     },
 
     /**
      * @param {number} x
      * @return {number}
      */
     _recordIndexAt: function(x)
     {
         return this.counter.x.upperBound(x * window.devicePixelRatio, null, this.counter._minimumIndex + 1, this.counter._maximumIndex + 1) - 1;
     },
 
     /**
      * @param {number} x
      */
     updateCurrentValue: function(x)
     {
         if (!this.visible() || !this.counter.values.length || !this.counter.x)
             return;
         var index = this._recordIndexAt(x);
-        this._value.textContent = WebInspector.UIString(this._currentValueLabel, this.counter.values[index]);
+        var value = Number.withThousandsSeparator(this.counter.values[index]);
+        this._value.textContent = WebInspector.UIString(this._currentValueLabel, value);
         var y = this.graphYValues[index] / window.devicePixelRatio;
         this._marker.style.left = x + "px";
         this._marker.style.top = y + "px";
         this._marker.classList.remove("hidden");
     },
 
     _clearCurrentValueAndMarker: function()
     {
         this._value.textContent = "";
         this._marker.classList.add("hidden");
@@ skipping 14 matching lines @@
         var originY = this._verticalPadding;
         var counter = this.counter;
         var values = counter.values;
 
         if (!values.length)
             return;
 
         var bounds = counter._calculateBounds();
         var minValue = bounds.min;
         var maxValue = bounds.max;
-        this.setRange(minValue, maxValue);
+        this.setRange(minValue, maxValue, counter);
 
         if (!this.visible())
             return;
 
         var yValues = this.graphYValues;
         var maxYRange = maxValue - minValue;
         var yFactor = maxYRange ? height / (maxYRange) : 1;
 
         ctx.save();
         ctx.lineWidth = window.devicePixelRatio;
@@ skipping 26 matching lines @@
         }
         ctx.closePath();
         ctx.restore();
     },
 
     /**
      * @return {boolean}
      */
     visible: function()
     {
-        return this._swatch.checked;
+        return this._filter.checked();
     }
 }
-
-
-/**
- * @constructor
- * @extends {WebInspector.Object}
- */
-WebInspector.SwatchCheckbox = function(title, color)
-{
-    this.element = createElement("div");
-    this._swatch = this.element.createChild("div", "swatch");
-    this.element.createChild("span", "title").textContent = title;
-    this._color = color;
-    this.checked = true;
-
-    this.element.addEventListener("click", this._toggleCheckbox.bind(this), true);
-}
-
-WebInspector.SwatchCheckbox.Events = {
-    Changed: "Changed"
-}
-
-WebInspector.SwatchCheckbox.prototype = {
-    get checked()
-    {
-        return this._checked;
-    },
-
-    set checked(v)
-    {
-        this._checked = v;
-        if (this._checked)
-            this._swatch.style.backgroundColor = this._color;
-        else
-            this._swatch.style.backgroundColor = "";
-    },
-
-    _toggleCheckbox: function(event)
-    {
-        this.checked = !this.checked;
-        this.dispatchEventToListeners(WebInspector.SwatchCheckbox.Events.Changed);
-    },
-
-    __proto__: WebInspector.Object.prototype
-}