MD Settings: convert paper-icon-button to paper-icon-button-light.

MD Settings: convert paper-icon-button to paper-icon-button-light.

With this CL, grepping "chrome/browser/resources/settings/" for
"<paper-icon-button" should find nothing.

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2848973003
Cr-Commit-Position: refs/heads/master@{#479874}
Committed: https://chromium.googlesource.com/chromium/src/+/bf4adbb1a2b7066a30297d5df51707fd468d391d

[scottchen, 2017-04-28 20:52:20]

Description was changed from

==========
MD Settings: convert paper-icon-button to paper-icon-button-light.

BUG=None
==========

to

==========
MD Settings: convert paper-icon-button to paper-icon-button-light.

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[scottchen, 2017-05-03 23:18:12]

scottchen@chromium.org changed reviewers:
+ dbeam@chromium.org, dschuyler@chromium.org, stevenjb@chromium.org

[scottchen, 2017-05-03 23:18:14]

Sorry - I know this CL looks gigantic.. but I swear very few lines changed per
file.

With this CL, grepping "chrome/browser/resources/settings/" for
"<paper-icon-button" should find nothing.

I used two approaches:
- If the icon was "cr:something" and could be used in many places, I make a new
.svg and add a class in "cr_elements/shared_style_css.html"

- If the icon was "settings:something" or used only once, I just made it:
  <button is="paper-icon-button-light">
    <iron-icon icon="settings:comething">
  </button>

There are also minor js/css tweaks in here to fix some issues surfaced by the
swap.

[scottchen, 2017-05-03 23:19:16]

Description was changed from

==========
MD Settings: convert paper-icon-button to paper-icon-button-light.

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: convert paper-icon-button to paper-icon-button-light.

With this CL, grepping "chrome/browser/resources/settings/" for
"<paper-icon-button" should find nothing.

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[stevenjb, 2017-05-03 23:58:59]

https://codereview.chromium.org/2848973003/diff/380001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/user_list.html (right):

https://codereview.chromium.org/2848973003/diff/380001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/user_list.html:38:
background-color: transparent;
Ah. Hrm. I'd rather modify [scrollable] :focus in shared_style_css.html to
[scrollable] div:focus, but that should really be done in a separate CL.

[dschuyler, 2017-05-08 19:05:29]

https://codereview.chromium.org/2848973003/diff/380001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements_images.grdp (right):

https://codereview.chromium.org/2848973003/diff/380001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements_images.grdp:18: file="images/icon_settings.svg"
type="BINDATA" />
I only see one of these svg files in this CL - were they added separately?

[scottchen, 2017-05-22 17:39:46]

On 2017/05/08 19:05:29, dschuyler wrote:
>
https://codereview.chromium.org/2848973003/diff/380001/ui/webui/resources/cr_...
> File ui/webui/resources/cr_elements_images.grdp (right):
> 
>
https://codereview.chromium.org/2848973003/diff/380001/ui/webui/resources/cr_...
> ui/webui/resources/cr_elements_images.grdp:18: file="images/icon_settings.svg"
> type="BINDATA" />
> I only see one of these svg files in this CL - were they added separately?

dschuyler@ oops, I missed them in the commit. Just added.

[Dan Beam, 2017-05-23 16:36:22]

is there a way we don't have to keep copying these svgs everywhere?

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js
(right):

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js:37:
this.$$('button[is="paper-icon-button-light"]'))
curlies

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/change_picture.html (right):

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture.html:139:
tabindex="0" class="icon-delete" title="$i18n{discardPhoto}"
do you still need a tabindex=0 here?

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/fingerprint_list.html
(right):

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/fingerprint_list.html:23:
.add-link[focused] {
what's going on here?

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
File
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html
(right):

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html:86:
<iron-icon  icon="cr:arrow-drop-down"></iron-icon>
\s\s -> \s

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_subpage.html
(right):

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_subpage.html:62:
<iron-icon icon="settings:arrow-back"></iron-icon>
indent off

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:361:
--webkit-margin-start: 0;
i don't think this is working because this accidentally has 2 hyphens in front. 
fix or delete?

[stevenjb, 2017-05-23 22:16:02]

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/user_list.html (right):

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/user_list.html:39: }
I'd still prefer to make this change globally and separately; there is nothing
special about this case that I am aware of and we may have this problem
elsewhere. Also, we shouldn't need the class.

[scrollable] button[is='paper-icon-button-light'] {
  background-color: transparent;
}

[stevenjb, 2017-05-23 23:19:24]

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/user_list.html (right):

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/user_list.html:39: }
On 2017/05/23 22:16:02, stevenjb wrote:
> I'd still prefer to make this change globally and separately; there is nothing
> special about this case that I am aware of and we may have this problem
> elsewhere. Also, we shouldn't need the class.
> 
> [scrollable] button[is='paper-icon-button-light'] {
>   background-color: transparent;
> }

I just re-read my first suggestion and actually like that approach better (with
a small adjustment). I put up a CL with that change:

https://codereview.chromium.org/2900173004/

[scottchen, 2017-05-27 00:18:03]

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js
(right):

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js:37:
this.$$('button[is="paper-icon-button-light"]'))
On 2017/05/23 16:36:22, Dan Beam wrote:
> curlies

Done.

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/change_picture.html (right):

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture.html:139:
tabindex="0" class="icon-delete" title="$i18n{discardPhoto}"
On 2017/05/23 16:36:22, Dan Beam wrote:
> do you still need a tabindex=0 here?

nope

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/fingerprint_list.html
(right):

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/fingerprint_list.html:23:
.add-link[focused] {
On 2017/05/23 16:36:22, Dan Beam wrote:
> what's going on here?

There was a really weird visual where when the button is focused, the ripple is
gray and the text is also gray, so it was reaaaally hard to read the text.

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/user_list.html (right):

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/user_list.html:39: }
On 2017/05/23 23:19:24, stevenjb wrote:
> On 2017/05/23 22:16:02, stevenjb wrote:
> > I'd still prefer to make this change globally and separately; there is
nothing
> > special about this case that I am aware of and we may have this problem
> > elsewhere. Also, we shouldn't need the class.
> > 
> > [scrollable] button[is='paper-icon-button-light'] {
> >   background-color: transparent;
> > }
> 
> I just re-read my first suggestion and actually like that approach better
(with
> a small adjustment). I put up a CL with that change:
> 
> https://codereview.chromium.org/2900173004/
> 

Cool, I'll remove it here.

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
File
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html
(right):

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html:86:
<iron-icon  icon="cr:arrow-drop-down"></iron-icon>
On 2017/05/23 16:36:22, Dan Beam wrote:
> \s\s -> \s

Done.

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_subpage.html
(right):

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_subpage.html:62:
<iron-icon icon="settings:arrow-back"></iron-icon>
On 2017/05/23 16:36:22, Dan Beam wrote:
> indent off

Done.

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2848973003/diff/400001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:361:
--webkit-margin-start: 0;
On 2017/05/23 16:36:22, Dan Beam wrote:
> i don't think this is working because this accidentally has 2 hyphens in
front. 
> fix or delete?

Done.

[dschuyler, 2017-05-30 18:34:20]

On 2017/05/03 23:18:14, scottchen wrote:
> Sorry - I know this CL looks gigantic.. but I swear very few lines changed per
> file.
> 
> With this CL, grepping "chrome/browser/resources/settings/" for
> "<paper-icon-button" should find nothing.
> 
> I used two approaches:
> - If the icon was "cr:something" and could be used in many places, I make a
new
> .svg and add a class in "cr_elements/shared_style_css.html"
> 
> - If the icon was "settings:something" or used only once, I just made it:
>   <button is="paper-icon-button-light">
>     <iron-icon icon="settings:comething">
>   </button>

What is the advantage of using iron-icon here? 

Here are some reasons not to use iron-icon here:
- it should be lighter weight (no shadow dom) to use a flattened svg than an
iron-icon 
- it's nicer to use the same pattern for all paper-icon-button-light instances

> 
> There are also minor js/css tweaks in here to fix some issues surfaced by the
> swap.

[scottchen, 2017-05-30 19:33:05]

On 2017/05/30 18:34:20, dschuyler wrote:
> On 2017/05/03 23:18:14, scottchen wrote:
> > Sorry - I know this CL looks gigantic.. but I swear very few lines changed
per
> > file.
> > 
> > With this CL, grepping "chrome/browser/resources/settings/" for
> > "<paper-icon-button" should find nothing.
> > 
> > I used two approaches:
> > - If the icon was "cr:something" and could be used in many places, I make a
> new
> > .svg and add a class in "cr_elements/shared_style_css.html"
> > 
> > - If the icon was "settings:something" or used only once, I just made it:
> >   <button is="paper-icon-button-light">
> >     <iron-icon icon="settings:comething">
> >   </button>
> 
> What is the advantage of using iron-icon here? 
> 
> Here are some reasons not to use iron-icon here:
> - it should be lighter weight (no shadow dom) to use a flattened svg than an
> iron-icon 
> - it's nicer to use the same pattern for all paper-icon-button-light instances
> 
> > 
> > There are also minor js/css tweaks in here to fix some issues surfaced by
the
> > swap.

Acknowledged. I discussed with Hector and we agreed on making similar classes
somewhere within the settings scope and include it in settings-shared-css.

[Dan Beam, 2017-05-31 00:40:16]

ok, i think we have a few options here:

1.

<button is="paper-icon-button-light">
  <iron-icon icon="..."></iron-icon>
</button>

this doesn't add more shadow layers than <paper-icon-button> (which had an
iron-icon embedded) but is 1 more shadow layer than options #2 and #3.

this options requires:
* JS to be up and running
* a bit of app code to be loaded (i.e. Polymer core, definition of iron-iconset)
* iconset must fire an event
* making an <svg> element programmatically with this code
https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro...

this is probably the most expensive and flickery, but also allows us to commit
the least amount of cruft to chrome's binary (just a bunch of paths in 1 file,
easy to update).  but it probably doesn't matter thatttt much.

2.

<style> .blah { background: url(chrome://resources/blah.svg); } </style>
<button is="paper-icon-button-light" class="blah"></button>

duplicates any icons from existing iconsets.  mmaps memory from disk after some
IPC when responding to chrome://resources/ URL.  good for larger, slower, or
duplicate images that aren't on the critical rendering path (or to intentionally
lazy load images).

3.

flattenhtml="true" in .grd
<style> .blah { background: url(blah.svg); } </style>
<button is="paper-icon-button-light" class="blah"></button>

this base 64 encodes the SVG file as a data URI so that there's no IPC nor
mmapping.  good for small images that aren't used a bajillion times (the
inlining is repetitive) that you want to show up fast.

[scottchen, 2017-05-31 00:48:42]

> 3.
> 
> flattenhtml="true" in .grd
> <style> .blah { background: url(blah.svg); } </style>
> <button is="paper-icon-button-light" class="blah"></button>
> 
> this base 64 encodes the SVG file as a data URI so that there's no IPC nor
> mmapping.  good for small images that aren't used a bajillion times (the
> inlining is repetitive) that you want to show up fast.

Sounds like option 3 is the best for this case, as it doesn't have significant
downside since the icons are used < 100 times in settings?

[dschuyler, 2017-05-31 00:59:44]

On 2017/05/31 00:40:16, Dan Beam wrote:
> ok, i think we have a few options here:
> 
> 1.
> 
> <button is="paper-icon-button-light">
>   <iron-icon icon="..."></iron-icon>
> </button>
> 
> this doesn't add more shadow layers than <paper-icon-button> (which had an
> iron-icon embedded) but is 1 more shadow layer than options #2 and #3.
> 
> this options requires:
> * JS to be up and running
> * a bit of app code to be loaded (i.e. Polymer core, definition of
iron-iconset)
> * iconset must fire an event
> * making an <svg> element programmatically with this code
>
https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro...
> 
> this is probably the most expensive and flickery, but also allows us to commit
> the least amount of cruft to chrome's binary (just a bunch of paths in 1 file,
> easy to update).  but it probably doesn't matter thatttt much.

The icons are often small enough that the size file paths may be on the same
order of magnitude. (The subpage arrow svg *may even be smaller* than some full
file paths).

> 
> 2.
> 
> <style> .blah { background: url(chrome://resources/blah.svg); } </style>
> <button is="paper-icon-button-light" class="blah"></button>
> 
> duplicates any icons from existing iconsets.  

I can see how that could happen, but I think we've been (or should be) removing
unused icons from the iconsets (so it would be moved rather than duplicated).

> mmaps memory from disk after some
> IPC when responding to chrome://resources/ URL.  good for larger, slower, or
> duplicate images that aren't on the critical rendering path (or to
intentionally
> lazy load images).
> 
> 3.
> 
> flattenhtml="true" in .grd
> <style> .blah { background: url(blah.svg); } </style>
> <button is="paper-icon-button-light" class="blah"></button>
> 
> this base 64 encodes the SVG file as a data URI so that there's no IPC nor
> mmapping.  good for small images that aren't used a bajillion times (the
> inlining is repetitive) that you want to show up fast.

This sgtm if the icon is on the order of dozens or hundreds of bytes.

Also, had we made some change where the style include="foo" CSS were no longer
duplicated the styles? Would/could that reduce this overhead back to one copy?
(I could easily be mistaken on this point, I heard this second hand and may have
misunderstood).

[Dan Beam, 2017-05-31 01:12:06]

On 2017/05/31 00:59:44, dschuyler wrote:
> On 2017/05/31 00:40:16, Dan Beam wrote:
> > ok, i think we have a few options here:
> > 
> > 1.
> > 
> > <button is="paper-icon-button-light">
> >   <iron-icon icon="..."></iron-icon>
> > </button>
> > 
> > this doesn't add more shadow layers than <paper-icon-button> (which had an
> > iron-icon embedded) but is 1 more shadow layer than options #2 and #3.
> > 
> > this options requires:
> > * JS to be up and running
> > * a bit of app code to be loaded (i.e. Polymer core, definition of
> iron-iconset)
> > * iconset must fire an event
> > * making an <svg> element programmatically with this code
> >
>
https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro...
> > 
> > this is probably the most expensive and flickery, but also allows us to
commit
> > the least amount of cruft to chrome's binary (just a bunch of paths in 1
file,
> > easy to update).  but it probably doesn't matter thatttt much.
> 
> The icons are often small enough that the size file paths may be on the same
> order of magnitude. (The subpage arrow svg *may even be smaller* than some
full
> file paths).
> 
> > 
> > 2.
> > 
> > <style> .blah { background: url(chrome://resources/blah.svg); } </style>
> > <button is="paper-icon-button-light" class="blah"></button>
> > 
> > duplicates any icons from existing iconsets.  
> 
> I can see how that could happen, but I think we've been (or should be)
removing
> unused icons from the iconsets (so it would be moved rather than duplicated).
> 
> > mmaps memory from disk after some
> > IPC when responding to chrome://resources/ URL.  good for larger, slower, or
> > duplicate images that aren't on the critical rendering path (or to
> intentionally
> > lazy load images).
> > 
> > 3.
> > 
> > flattenhtml="true" in .grd
> > <style> .blah { background: url(blah.svg); } </style>
> > <button is="paper-icon-button-light" class="blah"></button>
> > 
> > this base 64 encodes the SVG file as a data URI so that there's no IPC nor
> > mmapping.  good for small images that aren't used a bajillion times (the
> > inlining is repetitive) that you want to show up fast.
> 
> This sgtm if the icon is on the order of dozens or hundreds of bytes.
> 
> Also, had we made some change where the style include="foo" CSS were no longer
> duplicated the styles? Would/could that reduce this overhead back to one copy?
> (I could easily be mistaken on this point, I heard this second hand and may
have
> misunderstood).

dschuyler@: yes, <style>s are no longer duplicated, so

<dom-module id="x-blah">
  <template>
    <style> :host { background: url(blah.svg); } </style>
  </template>
</dom-module>

<x-blah></x-blah>
<x-blah></x-blah>
<x-blah></x-blah>
<x-blah></x-blah>
<x-blah></x-blah>

does not blow up our resource size.  HOWEVER, this:

<dom-module id="x-foo">
  <template>
    <style>
      .a1 { background: url(foo.svg); }
      .a2 { background: url(foo.svg); }
      .a3 { background: url(foo.svg); }
      .a4 { background: url(foo.svg); }
      .a5 { background: url(foo.svg); }
    </style>
  </template>
</dom-module>

WOULD result in 5 copies of foo.svg inlined.  this is easy to spot in 5
consecutive lines.  harder to spot in many different url()'s across the
codebase.

[scottchen, 2017-05-31 18:44:47]

On 2017/05/31 01:12:06, Dan Beam wrote:
> 
> dschuyler@: yes, <style>s are no longer duplicated, so
> 
> <dom-module id="x-blah">
>   <template>
>     <style> :host { background: url(blah.svg); } </style>
>   </template>
> </dom-module>
> 
> <x-blah></x-blah>
> <x-blah></x-blah>
> <x-blah></x-blah>
> <x-blah></x-blah>
> <x-blah></x-blah>
> 
> does not blow up our resource size.  

This does create a shadow-dom per instance though, right?

> HOWEVER, this:
> 
> <dom-module id="x-foo">
>   <template>
>     <style>
>       .a1 { background: url(foo.svg); }
>       .a2 { background: url(foo.svg); }
>       .a3 { background: url(foo.svg); }
>       .a4 { background: url(foo.svg); }
>       .a5 { background: url(foo.svg); }
>     </style>
>   </template>
> </dom-module>
> 
> WOULD result in 5 copies of foo.svg inlined.  this is easy to spot in 5
> consecutive lines.  harder to spot in many different url()'s across the
> codebase.

I think we could do the following to make this approach more manageable:
- in the cr_elements directory, we create a cr_icons_css.html that is solely for
defining the bg-img of icon classes used under the cr_elements scope.
- make an equivalent (e.g. settings_icons_css.html) and include it in
settings/settings_shared_css.html for icons that are only used within the
settings scope.

(^This approach came out of a discussion I had with Hector while he was
replacing a couple icons)

Thoughts on that?

[dschuyler, 2017-05-31 18:54:32]

On 2017/05/31 18:44:47, scottchen wrote:
> On 2017/05/31 01:12:06, Dan Beam wrote:
> > 
> > dschuyler@: yes, <style>s are no longer duplicated, so
> > 
> > <dom-module id="x-blah">
> >   <template>
> >     <style> :host { background: url(blah.svg); } </style>
> >   </template>
> > </dom-module>
> > 
> > <x-blah></x-blah>
> > <x-blah></x-blah>
> > <x-blah></x-blah>
> > <x-blah></x-blah>
> > <x-blah></x-blah>
> > 
> > does not blow up our resource size.  
> 
> This does create a shadow-dom per instance though, right?
> 
> > HOWEVER, this:
> > 
> > <dom-module id="x-foo">
> >   <template>
> >     <style>
> >       .a1 { background: url(foo.svg); }
> >       .a2 { background: url(foo.svg); }
> >       .a3 { background: url(foo.svg); }
> >       .a4 { background: url(foo.svg); }
> >       .a5 { background: url(foo.svg); }
> >     </style>
> >   </template>
> > </dom-module>
> > 
> > WOULD result in 5 copies of foo.svg inlined.  this is easy to spot in 5
> > consecutive lines.  harder to spot in many different url()'s across the
> > codebase.

Excellent. I think we can work within that.

> 
> I think we could do the following to make this approach more manageable:
> - in the cr_elements directory, we create a cr_icons_css.html that is solely
for
> defining the bg-img of icon classes used under the cr_elements scope.
> - make an equivalent (e.g. settings_icons_css.html) and include it in
> settings/settings_shared_css.html for icons that are only used within the
> settings scope.
> 
> (^This approach came out of a discussion I had with Hector while he was
> replacing a couple icons)
> 
> Thoughts on that?

Sounds good to me. iiuc, that would help assure than only ".a1" were created
(without .a2, .a3, .a4, .a5; in Dan's example above).

[Dan Beam, 2017-05-31 20:53:51]

On 2017/05/31 18:44:47, scottchen wrote:
> On 2017/05/31 01:12:06, Dan Beam wrote:
> > 
> > dschuyler@: yes, <style>s are no longer duplicated, so
> > 
> > <dom-module id="x-blah">
> >   <template>
> >     <style> :host { background: url(blah.svg); } </style>
> >   </template>
> > </dom-module>
> > 
> > <x-blah></x-blah>
> > <x-blah></x-blah>
> > <x-blah></x-blah>
> > <x-blah></x-blah>
> > <x-blah></x-blah>
> > 
> > does not blow up our resource size.  
> 
> This does create a shadow-dom per instance though, right?

yes

> 
> > HOWEVER, this:
> > 
> > <dom-module id="x-foo">
> >   <template>
> >     <style>
> >       .a1 { background: url(foo.svg); }
> >       .a2 { background: url(foo.svg); }
> >       .a3 { background: url(foo.svg); }
> >       .a4 { background: url(foo.svg); }
> >       .a5 { background: url(foo.svg); }
> >     </style>
> >   </template>
> > </dom-module>
> > 
> > WOULD result in 5 copies of foo.svg inlined.  this is easy to spot in 5
> > consecutive lines.  harder to spot in many different url()'s across the
> > codebase.
> 
> I think we could do the following to make this approach more manageable:
> - in the cr_elements directory, we create a cr_icons_css.html that is solely
for
> defining the bg-img of icon classes used under the cr_elements scope.
> - make an equivalent (e.g. settings_icons_css.html) and include it in
> settings/settings_shared_css.html for icons that are only used within the
> settings scope.
> 
> (^This approach came out of a discussion I had with Hector while he was
> replacing a couple icons)
> 
> Thoughts on that?

i think that's fine

[scottchen, 2017-06-05 22:09:00]

Patchset #31 (id:580001) has been deleted

[scottchen, 2017-06-06 00:46:14]

I've made a few new CSS files based on discussed solutions and converted more
<iron-icons> to bg-image:svg classes.

PTAL.

[dschuyler, 2017-06-06 18:58:40]

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.html
(right):

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.html:4:
<link rel="import"
href="chrome://resources/polymer/v1_0/iron-icon/iron-icon.html">
Is this still needed?

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.html:39:
}
Is this now unused?

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/camera.html (right):

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/camera.html:4: <link rel="import"
href="chrome://resources/polymer/v1_0/iron-icon/iron-icon.html">
Is this needed?

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/camera.html:68: }
I suspect that --iron-icon-fill-color is no longer having an effect.

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/fingerprint_list.html
(right):

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/fingerprint_list.html:29: }
This may be a fine change, but I'm missing how it relates to this CL.

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_icons_css.html (right):

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_icons_css.html:20: </if>
The indentation looks incorrect here.

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:348: }
fyi, I'm working on a CL at https://codereview.chromium.org/2918953004/
We may need to coordinate :)

https://codereview.chromium.org/2848973003/diff/620001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_icons_css.html (right):

https://codereview.chromium.org/2848973003/diff/620001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_icons_css.html:7: 
nit: no blank line

https://codereview.chromium.org/2848973003/diff/620001/ui/webui/resources/ima...
File ui/webui/resources/images/icon_more_vert.svg (right):

https://codereview.chromium.org/2848973003/diff/620001/ui/webui/resources/ima...
ui/webui/resources/images/icon_more_vert.svg:1: <svg
xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"
fill="#757575" preserveAspectRatio="xMidYMid meet"><path d="M12 8c1.1 0 2-.9
2-2s-.9-2-2-2-2 .9-2 2 .9 2 2 2zm0 2c-1.1 0-2 .9-2 2s.9 2 2 2 2-.9
2-2-.9-2-2-2zm0 6c-1.1 0-2 .9-2 2s.9 2 2 2 2-.9 2-2-.9-2-2-2z" /></svg>
maybe sub " />" with "></path>"

i.e.
s+ />+></path>+

[scottchen, 2017-06-06 22:33:12]

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.html
(right):

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.html:4:
<link rel="import"
href="chrome://resources/polymer/v1_0/iron-icon/iron-icon.html">
On 2017/06/06 18:58:39, dschuyler wrote:
> Is this still needed?

Nope, removing.

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.html:39:
}
On 2017/06/06 18:58:39, dschuyler wrote:
> Is this now unused?

Yes, removing!

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/camera.html (right):

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/camera.html:4: <link rel="import"
href="chrome://resources/polymer/v1_0/iron-icon/iron-icon.html">
On 2017/06/06 18:58:39, dschuyler wrote:
> Is this needed?

Nope! removing.

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/camera.html:68: }
On 2017/06/06 18:58:39, dschuyler wrote:
> I suspect that --iron-icon-fill-color is no longer having an effect.

Yep, removing!

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/fingerprint_list.html
(right):

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/fingerprint_list.html:29: }
On 2017/06/06 18:58:39, dschuyler wrote:
> This may be a fine change, but I'm missing how it relates to this CL.

It's not related, I'll remove it from this CL for the sake of making sense.

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_icons_css.html (right):

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_icons_css.html:20: </if>
On 2017/06/06 18:58:39, dschuyler wrote:
> The indentation looks incorrect here.

Wow, my sublime text went crazy! Fixed.

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2848973003/diff/620001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:348: }
On 2017/06/06 18:58:39, dschuyler wrote:
> fyi, I'm working on a CL at https://codereview.chromium.org/2918953004/
> We may need to coordinate :)

Acknowledged.

https://codereview.chromium.org/2848973003/diff/620001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_icons_css.html (right):

https://codereview.chromium.org/2848973003/diff/620001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_icons_css.html:7: 
On 2017/06/06 18:58:39, dschuyler wrote:
> nit: no blank line

Done.

https://codereview.chromium.org/2848973003/diff/620001/ui/webui/resources/ima...
File ui/webui/resources/images/icon_more_vert.svg (right):

https://codereview.chromium.org/2848973003/diff/620001/ui/webui/resources/ima...
ui/webui/resources/images/icon_more_vert.svg:1: <svg
xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"
fill="#757575" preserveAspectRatio="xMidYMid meet"><path d="M12 8c1.1 0 2-.9
2-2s-.9-2-2-2-2 .9-2 2 .9 2 2 2zm0 2c-1.1 0-2 .9-2 2s.9 2 2 2 2-.9
2-2-.9-2-2-2zm0 6c-1.1 0-2 .9-2 2s.9 2 2 2 2-.9 2-2-.9-2-2-2z" /></svg>
On 2017/06/06 18:58:39, dschuyler wrote:
> maybe sub " />" with "></path>"
> 
> i.e.
> s+ />+></path>+

Done.

[dschuyler, 2017-06-07 23:21:22]

https://codereview.chromium.org/2848973003/diff/640001/chrome/browser/resourc...
File chrome/browser/resources/settings/internet_page/internet_subpage.html
(right):

https://codereview.chromium.org/2848973003/diff/640001/chrome/browser/resourc...
chrome/browser/resources/settings/internet_page/internet_subpage.html:62:
<iron-icon icon="settings:add-wifi"></iron-icon>
Thanks for removing iron-icon in other files in this CL. Is this file an
oversight or are these more tricky to remove the iron-icon?

https://codereview.chromium.org/2848973003/diff/640001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_icons_css.html (right):

https://codereview.chromium.org/2848973003/diff/640001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_icons_css.html:3: <!-- Common icon
classes for Material Design WebUI -->
Please add a period and the end of the comment.

https://codereview.chromium.org/2848973003/diff/640001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2848973003/diff/640001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:342: iron-list {
wdyt of something like
.list-item button
akin to line 42 above.

Also, is the rule at line 42 needed with this change?

https://codereview.chromium.org/2848973003/diff/640001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:344:
-webkit-margin-start: 0;
Where is it necessary to set the -start to zero?

https://codereview.chromium.org/2848973003/diff/640001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_icons_css.html (right):

https://codereview.chromium.org/2848973003/diff/640001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_icons_css.html:3: <!-- Common icon classes for
Material Design WebUI -->
period on comment.

https://codereview.chromium.org/2848973003/diff/640001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/shared_style_css.html (right):

https://codereview.chromium.org/2848973003/diff/640001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/shared_style_css.html:53:
@apply(--cr-paper-icon-button-margin);
This change allows preceding elements in a row to get too close to the button
(which will regress a recent bug fix).

[scottchen, 2017-06-08 22:13:15]

https://codereview.chromium.org/2848973003/diff/640001/chrome/browser/resourc...
File chrome/browser/resources/settings/internet_page/internet_subpage.html
(right):

https://codereview.chromium.org/2848973003/diff/640001/chrome/browser/resourc...
chrome/browser/resources/settings/internet_page/internet_subpage.html:62:
<iron-icon icon="settings:add-wifi"></iron-icon>
On 2017/06/07 23:21:21, dschuyler wrote:
> Thanks for removing iron-icon in other files in this CL. Is this file an
> oversight or are these more tricky to remove the iron-icon?

Missed this one, nice catch.

https://codereview.chromium.org/2848973003/diff/640001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_icons_css.html (right):

https://codereview.chromium.org/2848973003/diff/640001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_icons_css.html:3: <!-- Common icon
classes for Material Design WebUI -->
On 2017/06/07 23:21:21, dschuyler wrote:
> Please add a period and the end of the comment.

Done.

https://codereview.chromium.org/2848973003/diff/640001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2848973003/diff/640001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:342: iron-list {
On 2017/06/07 23:21:21, dschuyler wrote:
> wdyt of something like
> .list-item button
> akin to line 42 above.
> 
> Also, is the rule at line 42 needed with this change?

there's no more paper-icon-button after this cl, so removing line 42.

https://codereview.chromium.org/2848973003/diff/640001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:344:
-webkit-margin-start: 0;
On 2017/06/07 23:21:22, dschuyler wrote:
> Where is it necessary to set the -start to zero?
> 

It helps align items with their headers. Otherwise it'll look like this:
http://imgur.com/a/IjK5j

EDIT: 
addressed offline, we always want the spacing before the icon-button, so
removing this margin-start:0 rule.

https://codereview.chromium.org/2848973003/diff/640001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_icons_css.html (right):

https://codereview.chromium.org/2848973003/diff/640001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_icons_css.html:3: <!-- Common icon classes for
Material Design WebUI -->
On 2017/06/07 23:21:22, dschuyler wrote:
> period on comment.

Done.

https://codereview.chromium.org/2848973003/diff/640001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/shared_style_css.html (right):

https://codereview.chromium.org/2848973003/diff/640001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/shared_style_css.html:53:
@apply(--cr-paper-icon-button-margin);
On 2017/06/07 23:21:22, dschuyler wrote:
> This change allows preceding elements in a row to get too close to the button
> (which will regress a recent bug fix).

This value defaults to 16px, so that 16px will be there unless the custom
element in action really needs to override the front margin (i.e.
http://imgur.com/a/IjK5j to fix misalignment issue).

EDIT:
We addressed this offline, the conclusion was we always want the spacing before
the icon-button based on spec. Will make a separate issue to fix misalignment.

[dschuyler, 2017-06-09 22:30:46]

https://codereview.chromium.org/2848973003/diff/680001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements_images.grdp (right):

https://codereview.chromium.org/2848973003/diff/680001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements_images.grdp:24:
file="images/icon_arrow_dropdown.svg" type="BINDATA" />
I think these may not be needed if they are flattened into the css.

https://codereview.chromium.org/2848973003/diff/680001/ui/webui/resources/set...
File ui/webui/resources/settings_images.grdp (right):

https://codereview.chromium.org/2848973003/diff/680001/ui/webui/resources/set...
ui/webui/resources/settings_images.grdp:17: </grit-part>
Please check if we can get by without including the svgs separately (i.e. would
be redundant with flattening them into the css).

[scottchen, 2017-06-09 23:19:26]

https://codereview.chromium.org/2848973003/diff/680001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements_images.grdp (right):

https://codereview.chromium.org/2848973003/diff/680001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements_images.grdp:24:
file="images/icon_arrow_dropdown.svg" type="BINDATA" />
On 2017/06/09 22:30:46, dschuyler wrote:
> I think these may not be needed if they are flattened into the css.

Acknowledged.

https://codereview.chromium.org/2848973003/diff/680001/ui/webui/resources/set...
File ui/webui/resources/settings_images.grdp (right):

https://codereview.chromium.org/2848973003/diff/680001/ui/webui/resources/set...
ui/webui/resources/settings_images.grdp:17: </grit-part>
On 2017/06/09 22:30:46, dschuyler wrote:
> Please check if we can get by without including the svgs separately (i.e.
would
> be redundant with flattening them into the css).

Done.

[dschuyler, 2017-06-09 23:48:03]

lgtm

https://codereview.chromium.org/2848973003/diff/700001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_icons_css.html (right):

https://codereview.chromium.org/2848973003/diff/700001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_icons_css.html:8: background-image:
url(./images/settings_icon_arrow_back.svg);
nit: maybe go without the ./

[scottchen, 2017-06-10 00:15:22]

https://codereview.chromium.org/2848973003/diff/700001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_icons_css.html (right):

https://codereview.chromium.org/2848973003/diff/700001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_icons_css.html:8: background-image:
url(./images/settings_icon_arrow_back.svg);
On 2017/06/09 23:48:03, dschuyler wrote:
> nit: maybe go without the ./

I actually had the "./" intentionally, to be more explicit about which images/
this is referring to. (Discussed with reviewer offline and reviewer is OK with
this.)

[scottchen, 2017-06-10 00:15:31]

The CQ bit was checked by scottchen@chromium.org

[commit-bot: I haz the power, 2017-06-10 00:15:56]

CQ is trying da patch.

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

[scottchen, 2017-06-10 00:18:47]

The CQ bit was unchecked by scottchen@chromium.org

[scottchen, 2017-06-10 00:22:54]

dbeam@ stevenjb@ PTAL.

[scottchen, 2017-06-12 18:24:31]

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

[commit-bot: I haz the power, 2017-06-12 18:25:20]

Dry run: 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, 2017-06-12 19:25:40]

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

[commit-bot: I haz the power, 2017-06-12 19:25:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[scottchen, 2017-06-12 22:56:08]

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

[commit-bot: I haz the power, 2017-06-12 22:56:44]

Dry run: 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, 2017-06-13 00:17:50]

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

[commit-bot: I haz the power, 2017-06-13 00:17:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[scottchen, 2017-06-13 01:52:38]

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

[commit-bot: I haz the power, 2017-06-13 01:53:11]

Dry run: 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, 2017-06-13 03:10:00]

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

[commit-bot: I haz the power, 2017-06-13 03:10:02]

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

[scottchen, 2017-06-13 20:20:22]

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

[commit-bot: I haz the power, 2017-06-13 20:21:26]

Dry run: CQ is trying da patch.

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

[scottchen, 2017-06-13 21:04:42]

Patchset #39 (id:760001) has been deleted

[scottchen, 2017-06-13 22:56:18]

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

[commit-bot: I haz the power, 2017-06-13 22:56:49]

Dry run: CQ is trying da patch.

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

[scottchen, 2017-06-14 00:15:13]

dschuyler@ as mentioned offline, I had to add the cr_elements svg images back
into the .grdp because vulcanization will alter the relative pathing in the CSS
files, such that flattenhtml doesn't run as expected. 

dpapad@ is making a separate bug to fix that issue, but for the interim it looks
like including the SVGs is unavoidable.

dschuyler@ PTAL and re-LG.

[commit-bot: I haz the power, 2017-06-14 00:24:42]

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

[commit-bot: I haz the power, 2017-06-14 00:24:43]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-06-14 01:19:22]

rs lgtm

https://codereview.chromium.org/2848973003/diff/780001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_icons_css.html (right):

https://codereview.chromium.org/2848973003/diff/780001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_icons_css.html:13: <if
expr="chromeos">
ideally, this <if> should be 0-indented so the selectors all line up.

i get that this is a little odd compared to .js

[scottchen, 2017-06-14 17:40:42]

https://codereview.chromium.org/2848973003/diff/780001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_icons_css.html (right):

https://codereview.chromium.org/2848973003/diff/780001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_icons_css.html:13: <if
expr="chromeos">
On 2017/06/14 01:19:21, Dan Beam wrote:
> ideally, this <if> should be 0-indented so the selectors all line up.
> 
> i get that this is a little odd compared to .js

Fixed.

[dschuyler, 2017-06-15 20:58:37]

On 2017/06/14 00:15:13, scottchen wrote:
> dschuyler@ as mentioned offline, I had to add the cr_elements svg images back
> into the .grdp because vulcanization will alter the relative pathing in the
CSS
> files, such that flattenhtml doesn't run as expected. 
> 
> dpapad@ is making a separate bug to fix that issue, but for the interim it
looks
> like including the SVGs is unavoidable.
> 
> dschuyler@ PTAL and re-LG.

That's unfortunate. Thanks for filing a bug to follow up on it.
still lgtm.

[scottchen, 2017-06-15 21:03:44]

The CQ bit was checked by scottchen@chromium.org

[scottchen, 2017-06-15 21:03:45]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2848973003/#ps820001
(title: "move unrelated fix to another CL")

[commit-bot: I haz the power, 2017-06-15 21:04:10]

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, 2017-06-15 22:37:18]

CQ is committing da patch.

Bot data: {"patchset_id": 820001, "attempt_start_ts": 1497560624560430,
"parent_rev": "f3c3acf46bb30d7f15f7baffda2b8013664cba50", "commit_rev":
"bf4adbb1a2b7066a30297d5df51707fd468d391d"}

[commit-bot: I haz the power, 2017-06-15 22:37:36]

Description was changed from

==========
MD Settings: convert paper-icon-button to paper-icon-button-light.

With this CL, grepping "chrome/browser/resources/settings/" for
"<paper-icon-button" should find nothing.

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: convert paper-icon-button to paper-icon-button-light.

With this CL, grepping "chrome/browser/resources/settings/" for
"<paper-icon-button" should find nothing.

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2848973003
Cr-Commit-Position: refs/heads/master@{#479874}
Committed:
https://chromium.googlesource.com/chromium/src/+/bf4adbb1a2b7066a30297d5df517...
==========

[commit-bot: I haz the power, 2017-06-15 22:37:41]

Committed patchset #41 (id:820001) as
https://chromium.googlesource.com/chromium/src/+/bf4adbb1a2b7066a30297d5df517...