[DevTools] Make toolbar counters declarative.

[DevTools] Make toolbar counters declarative.

This includes error-warning and devices counters.

BUG=none

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

[dgozman, 2014-07-21 13:14:44]

Could you please take a look?

[apavlov, 2014-07-21 13:34:06]

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ma...
File Source/devtools/front_end/main/AdvancedApp.js (right):

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ma...
Source/devtools/front_end/main/AdvancedApp.js:194:
this._counter.setCounter("device-icon-small", count,
WebInspector.UIString(((count > 1) ? "%d devices found" : "%d device found"),
count));
the parentheses around (count > 1) are not required and impact readability.

Also, what if the device count becomes zero?

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ma...
File Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ma...
Source/devtools/front_end/main/Main.js:852:
this._counter.setCounter("error-icon-small", errors, WebInspector.UIString("%d
error%s", errors, errors > 1 ? "s" : ""));
I believe this should be String.sprintf() rather than WebInspector.UIString(),
since it does not leave any possibility to i18n-ize the strings. Or, at least,
use string formatting similar to that for the device counter.

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ma...
Source/devtools/front_end/main/Main.js:853:
this._counter.setCounter("warning-icon-small", warnings,
WebInspector.UIString("%d warning%s", warnings, warnings > 1 ? "s" : ""));
ditto

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ui...
File Source/devtools/front_end/ui/StatusBarButton.js (right):

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ui...
Source/devtools/front_end/ui/StatusBarButton.js:92: this._counters = [];
This should be annotated at least with a @typedef (we have a non-enforced policy
to annotate all arrays to avoid type errors)

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ui...
Source/devtools/front_end/ui/StatusBarButton.js:93: for (var i = 0; i <
counters.length; i++) {
++i for consistency

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ui...
Source/devtools/front_end/ui/StatusBarButton.js:111: if
(this._counters[i].counter == counter) {
===

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ui...
Source/devtools/front_end/ui/StatusBarButton.js:125: if
(!this._counters[i].value) {
this._counters[i].value can be extracted into a variable, as it is used 3 times
inside the loop

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ui...
Source/devtools/front_end/ui/StatusBarButton.js:129:
this._counters[i].element.style.marginLeft = total ? "6px" : "0";
can we implement this using classes? This seems a bit fragile. E.g. let 6px be
the default value on a CSS class and add a class with 0 for when |total| is
truthy

[dgozman, 2014-07-21 14:24:35]

PTAL

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ma...
File Source/devtools/front_end/main/AdvancedApp.js (right):

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ma...
Source/devtools/front_end/main/AdvancedApp.js:194:
this._counter.setCounter("device-icon-small", count,
WebInspector.UIString(((count > 1) ? "%d devices found" : "%d device found"),
count));
On 2014/07/21 13:34:06, apavlov wrote:
> the parentheses around (count > 1) are not required and impact readability.
Done.

> Also, what if the device count becomes zero?

Zero counters are hidden by StatusBarCounter.

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ma...
File Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ma...
Source/devtools/front_end/main/Main.js:852:
this._counter.setCounter("error-icon-small", errors, WebInspector.UIString("%d
error%s", errors, errors > 1 ? "s" : ""));
On 2014/07/21 13:34:06, apavlov wrote:
> I believe this should be String.sprintf() rather than WebInspector.UIString(),
> since it does not leave any possibility to i18n-ize the strings. Or, at least,
> use string formatting similar to that for the device counter.

Did the same as device counter.

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ma...
Source/devtools/front_end/main/Main.js:853:
this._counter.setCounter("warning-icon-small", warnings,
WebInspector.UIString("%d warning%s", warnings, warnings > 1 ? "s" : ""));
On 2014/07/21 13:34:06, apavlov wrote:
> ditto

Done.

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ui...
File Source/devtools/front_end/ui/StatusBarButton.js (right):

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ui...
Source/devtools/front_end/ui/StatusBarButton.js:92: this._counters = [];
On 2014/07/21 13:34:06, apavlov wrote:
> This should be annotated at least with a @typedef (we have a non-enforced
policy
> to annotate all arrays to avoid type errors)

Nice catch. Done.

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ui...
Source/devtools/front_end/ui/StatusBarButton.js:93: for (var i = 0; i <
counters.length; i++) {
On 2014/07/21 13:34:06, apavlov wrote:
> ++i for consistency

Done.

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ui...
Source/devtools/front_end/ui/StatusBarButton.js:111: if
(this._counters[i].counter == counter) {
On 2014/07/21 13:34:06, apavlov wrote:
> ===

Done.

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ui...
Source/devtools/front_end/ui/StatusBarButton.js:125: if
(!this._counters[i].value) {
On 2014/07/21 13:34:06, apavlov wrote:
> this._counters[i].value can be extracted into a variable, as it is used 3
times
> inside the loop

Done.

https://codereview.chromium.org/408853002/diff/1/Source/devtools/front_end/ui...
Source/devtools/front_end/ui/StatusBarButton.js:129:
this._counters[i].element.style.marginLeft = total ? "6px" : "0";
On 2014/07/21 13:34:06, apavlov wrote:
> can we implement this using classes? This seems a bit fragile. E.g. let 6px be
> the default value on a CSS class and add a class with 0 for when |total| is
> truthy

Done.

[apavlov, 2014-07-22 16:18:37]

lgtm with comments

https://codereview.chromium.org/408853002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/ui/StatusBarButton.js (right):

https://codereview.chromium.org/408853002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/StatusBarButton.js:92: /** @type {!Array.<{element:
!Element, counter: string, value: number, title: string}>} */
This should be !Array.<!{element:...}>

https://codereview.chromium.org/408853002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/StatusBarButton.js:133:
counter.element.classList.toggle("status-bar-counter-item-first", !total);
Should this be !i for the semantics sake?

[dgozman, 2014-07-24 14:55:10]

https://codereview.chromium.org/408853002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/ui/StatusBarButton.js (right):

https://codereview.chromium.org/408853002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/StatusBarButton.js:92: /** @type {!Array.<{element:
!Element, counter: string, value: number, title: string}>} */
On 2014/07/22 16:18:37, apavlov wrote:
> This should be !Array.<!{element:...}>

Done.

https://codereview.chromium.org/408853002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/StatusBarButton.js:133:
counter.element.classList.toggle("status-bar-counter-item-first", !total);
On 2014/07/22 16:18:36, apavlov wrote:
> Should this be !i for the semantics sake?

There may be some zero counters at the start which are hidden. Thus, !total may
be true for i != 0.

[dgozman, 2014-07-24 14:55:17]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2014-07-24 14:56:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dgozman@chromium.org/408853002/60001

[commit-bot: I haz the power, 2014-07-24 16:01:11]

Change committed as 178848