DevTools: Refactor memory counter display

DevTools: Refactor memory counter display

Improve usability of jsHeapSize readability by formatting as bytes.
Refactor counter checkboxes to reuse CheckboxFilterUI. Delete swatch component.
Change default height of memory counters from 200 to 112px.

BUG=523697
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201608

[paulirish, 2015-08-23 07:28:35]

paulirish@chromium.org changed reviewers:
+ alph@chromium.org

[paulirish, 2015-08-23 07:28:35]

Screenshot: https://code.google.com/p/chromium/issues/detail?id=523697#c1

Pleased this worked out as a net-negative-LOC CL!

Let me know what you think

[alph, 2015-08-24 17:19:51]

Great! Thanks for doing that.

Btw, shouldn't the current value for memory be also shown in MB.

https://codereview.chromium.org/1309723003/diff/1/Source/devtools/front_end/t...
File Source/devtools/front_end/timeline/CountersGraph.js (right):

https://codereview.chromium.org/1309723003/diff/1/Source/devtools/front_end/t...
Source/devtools/front_end/timeline/CountersGraph.js:406: this._filter = new
WebInspector.CheckboxFilterUI(title, WebInspector.UIString(title));
No need to use UIString here. It should be used right on the string literal.

https://codereview.chromium.org/1309723003/diff/1/Source/devtools/front_end/t...
Source/devtools/front_end/timeline/CountersGraph.js:408: if (color !== null)
if (color)

https://codereview.chromium.org/1309723003/diff/1/Source/devtools/front_end/t...
Source/devtools/front_end/timeline/CountersGraph.js:412: this._range =
this._filter.labelElement().createChild("span", "range");
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

https://codereview.chromium.org/1309723003/diff/1/Source/devtools/front_end/t...
Source/devtools/front_end/timeline/CountersGraph.js:440: if (counter ==
this._memoryCountersPane.counters("jsHeapSizeUsed")) {
There should be no knowledge of a particular counter in the base class.
You can pass a custom formatter function to the constructor instead.

https://codereview.chromium.org/1309723003/diff/1/Source/devtools/front_end/t...
Source/devtools/front_end/timeline/CountersGraph.js:450: _toggleCounterGraph:
function(event)
nit: could you please annotate.

https://codereview.chromium.org/1309723003/diff/1/Source/devtools/front_end/t...
File Source/devtools/front_end/timeline/MemoryCountersGraph.js (right):

https://codereview.chromium.org/1309723003/diff/1/Source/devtools/front_end/t...
Source/devtools/front_end/timeline/MemoryCountersGraph.js:83: * @return
{!WebInspector.CountersGraph.Counter} counter
drop the "counter" name

https://codereview.chromium.org/1309723003/diff/1/Source/devtools/front_end/t...
Source/devtools/front_end/timeline/MemoryCountersGraph.js:85: counters:
function(counterName) {
nit: counters -> counter
nit: move { to the new line

[paulirish, 2015-08-25 23:18:47]

Tell me if the custom formatter style makes sense.

https://codereview.chromium.org/1309723003/diff/1/Source/devtools/front_end/t...
File Source/devtools/front_end/timeline/CountersGraph.js (right):

https://codereview.chromium.org/1309723003/diff/1/Source/devtools/front_end/t...
Source/devtools/front_end/timeline/CountersGraph.js:406: this._filter = new
WebInspector.CheckboxFilterUI(title, WebInspector.UIString(title));
On 2015/08/24 at 17:19:50, alph wrote:
> No need to use UIString here. It should be used right on the string literal.

Done

https://codereview.chromium.org/1309723003/diff/1/Source/devtools/front_end/t...
Source/devtools/front_end/timeline/CountersGraph.js:408: if (color !== null)
On 2015/08/24 at 17:19:50, alph wrote:
> if (color)

Done

https://codereview.chromium.org/1309723003/diff/1/Source/devtools/front_end/t...
Source/devtools/front_end/timeline/CountersGraph.js:412: this._range =
this._filter.labelElement().createChild("span", "range");
On 2015/08/24 17:19:50, alph wrote:
> 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

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.

https://codereview.chromium.org/1309723003/diff/1/Source/devtools/front_end/t...
Source/devtools/front_end/timeline/CountersGraph.js:440: if (counter ==
this._memoryCountersPane.counters("jsHeapSizeUsed")) {
On 2015/08/24 17:19:50, alph wrote:
> There should be no knowledge of a particular counter in the base class.
> You can pass a custom formatter function to the constructor instead.

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

https://codereview.chromium.org/1309723003/diff/1/Source/devtools/front_end/t...
Source/devtools/front_end/timeline/CountersGraph.js:450: _toggleCounterGraph:
function(event)
On 2015/08/24 17:19:50, alph wrote:
> nit: could you please annotate.

Done.

[alph, 2015-08-25 23:59:56]

lgtm

https://codereview.chromium.org/1309723003/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/timeline/CountersGraph.js (right):

https://codereview.chromium.org/1309723003/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/timeline/CountersGraph.js:400: * @param
{function(number): string} formatter
nit: If you want, you can make it optional defaulting to
Number.withThousandsSeparator

https://codereview.chromium.org/1309723003/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/timeline/CountersGraph.js:406: this.formatter =
formatter;
nit: this._formatter

[paulirish, 2015-08-26 16:54:38]

moved to a default formatter. 

can you verify this is the right pattern?

[alph, 2015-08-26 16:59:09]

https://codereview.chromium.org/1309723003/diff/80001/Source/devtools/front_e...
File Source/devtools/front_end/timeline/CountersGraph.js (right):

https://codereview.chromium.org/1309723003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/timeline/CountersGraph.js:95: * @param
{(function(number): string)|undefined} formatter
Use {function(number):string=} to make the argument optional.

https://codereview.chromium.org/1309723003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/timeline/CountersGraph.js:442: var formatter =
this._formatter || Number.withThousandsSeparator;
Move "|| Number.withThousandsSeparator" to the constructor.

https://codereview.chromium.org/1309723003/diff/80001/Source/devtools/front_e...
File Source/devtools/front_end/timeline/MemoryCountersGraph.js (right):

https://codereview.chromium.org/1309723003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/timeline/MemoryCountersGraph.js:43:
this._countersByName["documents"] =
this.createCounter(WebInspector.UIString("Documents"),
WebInspector.UIString("Documents: %s"), "hsl(0, 90%, 43%)", undefined);
Once you make it optional, you can omit "undefined"

[paulirish, 2015-08-26 18:04:57]

thx

https://codereview.chromium.org/1309723003/diff/80001/Source/devtools/front_e...
File Source/devtools/front_end/timeline/CountersGraph.js (right):

https://codereview.chromium.org/1309723003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/timeline/CountersGraph.js:95: * @param
{(function(number): string)|undefined} formatter
On 2015/08/26 16:59:09, alph wrote:
> Use {function(number):string=} to make the argument optional.

Done.

https://codereview.chromium.org/1309723003/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/timeline/CountersGraph.js:442: var formatter =
this._formatter || Number.withThousandsSeparator;
On 2015/08/26 16:59:08, alph wrote:
> Move "|| Number.withThousandsSeparator" to the constructor.

Done.

[paulirish, 2015-08-26 18:05:03]

The CQ bit was checked by paulirish@chromium.org

[paulirish, 2015-08-26 18:05:03]

The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org
Link to the patchset: https://codereview.chromium.org/1309723003/#ps100001
(title: "default formatter complete")

[commit-bot: I haz the power, 2015-08-26 18:05:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1309723003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1309723003/100001

[commit-bot: I haz the power, 2015-08-26 18:27:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-26 18:27:04]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[paulirish, 2015-09-01 19:49:57]

The CQ bit was checked by paulirish@chromium.org

[commit-bot: I haz the power, 2015-09-01 19:50:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1309723003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1309723003/100001

[paulirish, 2015-09-01 19:53:22]

The CQ bit was unchecked by paulirish@chromium.org

[paulirish, 2015-09-01 19:55:03]

Patchset #7 (id:120001) has been deleted

[paulirish, 2015-09-02 03:29:29]

The CQ bit was checked by paulirish@chromium.org

[paulirish, 2015-09-02 03:29:30]

The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org
Link to the patchset: https://codereview.chromium.org/1309723003/#ps100001
(title: "default formatter complete")

[commit-bot: I haz the power, 2015-09-02 03:29:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1309723003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1309723003/100001

[commit-bot: I haz the power, 2015-09-02 03:54:00]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201608