DevTools: eliminate ToolbarButton.setState method; cleanup toolbar.css

DevTools: eliminate ToolbarButton.setState method; cleanup toolbar.css

This patch is a preparation for the crrev.com/2473313002. It cleans up
toolbar.css so that for every icon in toolbarButtonGlyphs.png there's
exactly one css class which defines it.

Today, some clients of the ToolbarButton were abusing ToolbarButton
state to specify different icons. This patch moves these clients to
use ToolbarButton.setGlyph method.

As a result, the ToolbarButton.setState method became unneeded.
Instead, there's a ToolbarToggle.setToggleWithRed method which is there
to support needs of "recording" red icon.

As a nifty perk, the patch gets rid of two icons in
toolbarButtonGlyphs.svg.

BUG=none
R=dgozman

Committed: https://crrev.com/cec7d0e095a1fb1304b6bda02129ac4cc117f134
Cr-Commit-Position: refs/heads/master@{#430714}

[lushnikov, 2016-11-05 02:44:44]

please, take a look!

[lushnikov, 2016-11-05 02:45:33]

The CQ bit was checked by lushnikov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-05 02:45:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lushnikov, 2016-11-05 03:07:36]

Description was changed from

==========
DevTools: eliminate ToolbarButton.setState method; cleanup toolbar.css

This patch is a preparation for the crrev.com/2473313002. It cleans up
toolbar.css so that for every icon in toolbarButtonGlyphs.png there's
exactly one css class which defines it.

Today, some clients of the ToolbarButton were abusing ToolbarButton
state to specify a different icons. This patch moves these clients to
using ToolbarButton.setGlyph method.

As a result, the ToolbarButton.setState method became unneeded. Instead,
all the clients of the method were moved to the ToolbarToggle.

ToolbarToggle now has 4 (2x2) states:
- toggled/untoggled. Toggled state is shown as blue
- active/inactive. Active state is shown as red (console filter is
  a good example)

As a nifty perk, the patch gets rid of two icons in
toolbarButtonGlyphs.svg.

BUG=none
R=dgozman
==========

to

==========
DevTools: eliminate ToolbarButton.setState method; cleanup toolbar.css

This patch is a preparation for the crrev.com/2473313002. It cleans up
toolbar.css so that for every icon in toolbarButtonGlyphs.png there's
exactly one css class which defines it.

Today, some clients of the ToolbarButton were abusing ToolbarButton
state to specify different icons. This patch moves these clients to
use ToolbarButton.setGlyph method.

As a result, the ToolbarButton.setState method became unneeded.
And the ToolbarToggle now has 4 (2x2) states:
- toggled/untoggled. Toggled state is shown as blue
- active/inactive. Active state is shown as red (console filter is
  a good example)

As a nifty perk, the patch gets rid of two icons in
toolbarButtonGlyphs.svg.

BUG=none
R=dgozman
==========

[commit-bot: I haz the power, 2016-11-05 04:20:24]

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

[commit-bot: I haz the power, 2016-11-05 04:20:25]

Dry run: This issue passed the CQ dry run.

[lushnikov, 2016-11-05 17:36:39]

https://codereview.chromium.org/2482593003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/toolbar.css (right):

https://codereview.chromium.org/2482593003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/toolbar.css:528:
.active-recording-toolbar-item.toolbar-glyph,
this should be just 

.toolbar-state-active {
    background-color: rgb(216, 0, 0) !important;
}

https://codereview.chromium.org/2482593003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/toolbar.css:534:
:host-context(.-theme-with-dark-background) .toolbar-state-active
.filter-toolbar-item.toolbar-glyph,
ditto here:

:host-context(.-theme-with-dark-background) .toolbar-state-active {
    background-color: hsl(0, 100%, 65%) !important;
}

[lushnikov, 2016-11-07 05:05:06]

https://codereview.chromium.org/2482593003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/toolbar.css (right):

https://codereview.chromium.org/2482593003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/toolbar.css:528:
.active-recording-toolbar-item.toolbar-glyph,
On 2016/11/05 17:36:38, lushnikov wrote:
> this should be just 
> 
> .toolbar-state-active {
>     background-color: rgb(216, 0, 0) !important;
> }

Nah, this has to stay like this to support dark theme.

https://codereview.chromium.org/2482593003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/toolbar.css:534:
:host-context(.-theme-with-dark-background) .toolbar-state-active
.filter-toolbar-item.toolbar-glyph,
On 2016/11/05 17:36:38, lushnikov wrote:
> ditto here:
> 
> :host-context(.-theme-with-dark-background) .toolbar-state-active {
>     background-color: hsl(0, 100%, 65%) !important;
> }

Ditto.

[dgozman, 2016-11-07 18:32:46]

lgtm

https://codereview.chromium.org/2482593003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js (right):

https://codereview.chromium.org/2482593003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:584:
constructor(title, glyph, text, toggledGlyph) {
Does it make sense to pass toggledTitle as well?

https://codereview.chromium.org/2482593003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:589: this._state =
'';
Unused.

https://codereview.chromium.org/2482593003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/toolbar.css (right):

https://codereview.chromium.org/2482593003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/toolbar.css:528:
.active-recording-toolbar-item.toolbar-glyph,
On 2016/11/07 05:05:06, lushnikov wrote:
> On 2016/11/05 17:36:38, lushnikov wrote:
> > this should be just 
> > 
> > .toolbar-state-active {
> >     background-color: rgb(216, 0, 0) !important;
> > }
> 
> Nah, this has to stay like this to support dark theme.

I don't get it.

[lushnikov, 2016-11-08 01:44:45]

please, take another look

https://codereview.chromium.org/2482593003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js (right):

https://codereview.chromium.org/2482593003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:584:
constructor(title, glyph, text, toggledGlyph) {
On 2016/11/07 18:32:45, dgozman wrote:
> Does it make sense to pass toggledTitle as well?

Tooltips are done in a various of techniques - someone set them explicitly,
someone employs action.options. I'd rather not touch them.


Instead, I removed the third "text" argument - it's mostly unused.

https://codereview.chromium.org/2482593003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:589: this._state =
'';
On 2016/11/07 18:32:45, dgozman wrote:
> Unused.

Done.

https://codereview.chromium.org/2482593003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/toolbar.css (right):

https://codereview.chromium.org/2482593003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/toolbar.css:528:
.active-recording-toolbar-item.toolbar-glyph,
On 2016/11/07 18:32:45, dgozman wrote:
> On 2016/11/07 05:05:06, lushnikov wrote:
> > On 2016/11/05 17:36:38, lushnikov wrote:
> > > this should be just 
> > > 
> > > .toolbar-state-active {
> > >     background-color: rgb(216, 0, 0) !important;
> > > }
> > 
> > Nah, this has to stay like this to support dark theme.
> 
> I don't get it.

Removed the active state at all. Now there's a setToggleWithRed!

[lushnikov, 2016-11-08 01:46:38]

Description was changed from

==========
DevTools: eliminate ToolbarButton.setState method; cleanup toolbar.css

This patch is a preparation for the crrev.com/2473313002. It cleans up
toolbar.css so that for every icon in toolbarButtonGlyphs.png there's
exactly one css class which defines it.

Today, some clients of the ToolbarButton were abusing ToolbarButton
state to specify different icons. This patch moves these clients to
use ToolbarButton.setGlyph method.

As a result, the ToolbarButton.setState method became unneeded.
And the ToolbarToggle now has 4 (2x2) states:
- toggled/untoggled. Toggled state is shown as blue
- active/inactive. Active state is shown as red (console filter is
  a good example)

As a nifty perk, the patch gets rid of two icons in
toolbarButtonGlyphs.svg.

BUG=none
R=dgozman
==========

to

==========
DevTools: eliminate ToolbarButton.setState method; cleanup toolbar.css

This patch is a preparation for the crrev.com/2473313002. It cleans up
toolbar.css so that for every icon in toolbarButtonGlyphs.png there's
exactly one css class which defines it.

Today, some clients of the ToolbarButton were abusing ToolbarButton
state to specify different icons. This patch moves these clients to
use ToolbarButton.setGlyph method.

As a result, the ToolbarButton.setState method became unneeded.
Instead, there's a ToolbarToggle.setToggleWithRed method which is there
to support needs of "recording" red icon.

As a nifty perk, the patch gets rid of two icons in
toolbarButtonGlyphs.svg.

BUG=none
R=dgozman
==========

[dgozman, 2016-11-08 16:56:12]

lgtm

[lushnikov, 2016-11-08 17:10:57]

The CQ bit was checked by lushnikov@chromium.org

[commit-bot: I haz the power, 2016-11-08 17:11:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 21:15:40]

Description was changed from

==========
DevTools: eliminate ToolbarButton.setState method; cleanup toolbar.css

This patch is a preparation for the crrev.com/2473313002. It cleans up
toolbar.css so that for every icon in toolbarButtonGlyphs.png there's
exactly one css class which defines it.

Today, some clients of the ToolbarButton were abusing ToolbarButton
state to specify different icons. This patch moves these clients to
use ToolbarButton.setGlyph method.

As a result, the ToolbarButton.setState method became unneeded.
Instead, there's a ToolbarToggle.setToggleWithRed method which is there
to support needs of "recording" red icon.

As a nifty perk, the patch gets rid of two icons in
toolbarButtonGlyphs.svg.

BUG=none
R=dgozman
==========

to

==========
DevTools: eliminate ToolbarButton.setState method; cleanup toolbar.css

This patch is a preparation for the crrev.com/2473313002. It cleans up
toolbar.css so that for every icon in toolbarButtonGlyphs.png there's
exactly one css class which defines it.

Today, some clients of the ToolbarButton were abusing ToolbarButton
state to specify different icons. This patch moves these clients to
use ToolbarButton.setGlyph method.

As a result, the ToolbarButton.setState method became unneeded.
Instead, there's a ToolbarToggle.setToggleWithRed method which is there
to support needs of "recording" red icon.

As a nifty perk, the patch gets rid of two icons in
toolbarButtonGlyphs.svg.

BUG=none
R=dgozman
==========

[commit-bot: I haz the power, 2016-11-08 21:15:42]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-08 21:19:31]

Description was changed from

==========
DevTools: eliminate ToolbarButton.setState method; cleanup toolbar.css

This patch is a preparation for the crrev.com/2473313002. It cleans up
toolbar.css so that for every icon in toolbarButtonGlyphs.png there's
exactly one css class which defines it.

Today, some clients of the ToolbarButton were abusing ToolbarButton
state to specify different icons. This patch moves these clients to
use ToolbarButton.setGlyph method.

As a result, the ToolbarButton.setState method became unneeded.
Instead, there's a ToolbarToggle.setToggleWithRed method which is there
to support needs of "recording" red icon.

As a nifty perk, the patch gets rid of two icons in
toolbarButtonGlyphs.svg.

BUG=none
R=dgozman
==========

to

==========
DevTools: eliminate ToolbarButton.setState method; cleanup toolbar.css

This patch is a preparation for the crrev.com/2473313002. It cleans up
toolbar.css so that for every icon in toolbarButtonGlyphs.png there's
exactly one css class which defines it.

Today, some clients of the ToolbarButton were abusing ToolbarButton
state to specify different icons. This patch moves these clients to
use ToolbarButton.setGlyph method.

As a result, the ToolbarButton.setState method became unneeded.
Instead, there's a ToolbarToggle.setToggleWithRed method which is there
to support needs of "recording" red icon.

As a nifty perk, the patch gets rid of two icons in
toolbarButtonGlyphs.svg.

BUG=none
R=dgozman

Committed: https://crrev.com/cec7d0e095a1fb1304b6bda02129ac4cc117f134
Cr-Commit-Position: refs/heads/master@{#430714}
==========

[commit-bot: I haz the power, 2016-11-08 21:19:32]

Patchset 2 (id:??) landed as
https://crrev.com/cec7d0e095a1fb1304b6bda02129ac4cc117f134
Cr-Commit-Position: refs/heads/master@{#430714}

[pfeldman, 2016-12-10 00:11:38]

pfeldman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[pfeldman, 2016-12-10 00:11:39]

https://codereview.chromium.org/2482593003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/module.json (right):

https://codereview.chromium.org/2482593003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/module.json:46:
"toggleWithRedColor": true,
I don't think this is particularly extensible.