Add icons for network cr-elements to cr-icons for Chrome OS.

Add icons for network cr-elements to cr-icons for Chrome OS.

This is a follow-up to crrev.com/1950163003 and crrev.com/1947533002.

BUG=605821
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/441c30696670030f8c941120f8aeeea51ee87d4b
Cr-Commit-Position: refs/heads/master@{#392507}

[michaelpg, 2016-05-05 00:54:40]

Description was changed from

==========
Add icons for network cr-elements to cr-icons for Chrome OS.

This is a follow-up to crrev.com/1950163003 and crrev.com/1947533002.

BUG=605821
R=stevenjb@chromium.org
==========

to

==========
Add icons for network cr-elements to cr-icons for Chrome OS.

This is a follow-up to crrev.com/1950163003 and crrev.com/1947533002.

BUG=605821
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-05-05 15:01:15]

PTAL, see crrev.com/1947533002 for context. (We've created
cr-icons to list icons specific to cr-elements; once we do
the same for individual WebUI pages, we can remove "iron-icons"
from Chrome. The end goal is to only load the SVGs we need, not
all 900 in "iron-icons".)

[stevenjb, 2016-05-05 16:29:07]

https://codereview.chromium.org/1949113004/diff/1/ui/webui/resources/cr_eleme...
File ui/webui/resources/cr_elements/cr_icons.html (right):

https://codereview.chromium.org/1949113004/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_icons.html:7: -->
Not to be a pain but, is this content lifted directly from the polymer icon
set(s)? I have two concerns about that:
1. Changes by the Polymer team will not be reflected here (probably not a
problem, but we should make sure to discuss that with UX if we haven't).
2. We should at least document the source(s) of these.

https://codereview.chromium.org/1949113004/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_icons.html:16: </if>
Would it make more sense to have a separate file for chromeos specific icons? In
general that is preferred where possible unless the difference is trivial.

[michaelpg, 2016-05-05 21:13:33]

https://codereview.chromium.org/1949113004/diff/1/ui/webui/resources/cr_eleme...
File ui/webui/resources/cr_elements/cr_icons.html (right):

https://codereview.chromium.org/1949113004/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_icons.html:7: -->
On 2016/05/05 16:29:07, stevenjb wrote:
> Not to be a pain but, is this content lifted directly from the polymer icon
> set(s)?

Yes.

> I have two concerns about that:
> 1. Changes by the Polymer team will not be reflected here (probably not a
> problem, but we should make sure to discuss that with UX if we haven't).

Yes. However, this is how Polymer recommends iconsets be created, so presumably
they won't update their SVGs. They would probably just add new ones instead.
(Otherwise it would be risky to ever use an icon, as it could change on you
without you realizing.)

> 2. We should at least document the source(s) of these.

I did. I'll make it clearer.

https://codereview.chromium.org/1949113004/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_icons.html:16: </if>
On 2016/05/05 16:29:07, stevenjb wrote:
> Would it make more sense to have a separate file for chromeos specific icons?
In
> general that is preferred where possible unless the difference is trivial.

I think it would be best not to. I suspect we will be moving some or all of
these to all platforms, which is easier to do by removing <if>s than by cutting
them from one file and pasting in another. That's also likely to lead to
duplication, i.e., having an icon both in cr_icons_chromeos.html and
cr_icons.html (which would result in it being duplicated in the CrOS build, for
no reason).

[stevenjb, 2016-05-05 21:24:21]

https://codereview.chromium.org/1949113004/diff/1/ui/webui/resources/cr_eleme...
File ui/webui/resources/cr_elements/cr_icons.html (right):

https://codereview.chromium.org/1949113004/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_icons.html:7: -->
On 2016/05/05 21:13:33, michaelpg wrote:
> On 2016/05/05 16:29:07, stevenjb wrote:
> > Not to be a pain but, is this content lifted directly from the polymer icon
> > set(s)?
> 
> Yes.
> 
> > I have two concerns about that:
> > 1. Changes by the Polymer team will not be reflected here (probably not a
> > problem, but we should make sure to discuss that with UX if we haven't).
> 
> Yes. However, this is how Polymer recommends iconsets be created, so
presumably
> they won't update their SVGs. They would probably just add new ones instead.
> (Otherwise it would be risky to ever use an icon, as it could change on you
> without you realizing.)
> 
> > 2. We should at least document the source(s) of these.
> 
> I did. I'll make it clearer.
The inline comment is good, but I think that a top level comment would also be a
good idea.

Well, changing without realizing is what you expect when using a library.
Presumably the change would be to fix a bug or add clarity. That said this is
generally fine.

https://codereview.chromium.org/1949113004/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_icons.html:16: </if>
On 2016/05/05 21:13:33, michaelpg wrote:
> On 2016/05/05 16:29:07, stevenjb wrote:
> > Would it make more sense to have a separate file for chromeos specific
icons?
> In
> > general that is preferred where possible unless the difference is trivial.
> 
> I think it would be best not to. I suspect we will be moving some or all of
> these to all platforms, which is easier to do by removing <if>s than by
cutting
> them from one file and pasting in another. That's also likely to lead to
> duplication, i.e., having an icon both in cr_icons_chromeos.html and
> cr_icons.html (which would result in it being duplicated in the CrOS build,
for
> no reason).

OK. Looking more closely I guess these aren't especially chromeos specific. I
agree that we shouldn't have an icon in both, it would be in one or the other
(and some html files might need to include both, same as with polymer icon
sets).

[michaelpg, 2016-05-09 13:53:36]

https://codereview.chromium.org/1949113004/diff/1/ui/webui/resources/cr_eleme...
File ui/webui/resources/cr_elements/cr_icons.html (right):

https://codereview.chromium.org/1949113004/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_icons.html:7: -->
On 2016/05/05 21:24:21, stevenjb wrote:
> On 2016/05/05 21:13:33, michaelpg wrote:
> > On 2016/05/05 16:29:07, stevenjb wrote:
> > > Not to be a pain but, is this content lifted directly from the polymer
icon
> > > set(s)?
> > 
> > Yes.
> > 
> > > I have two concerns about that:
> > > 1. Changes by the Polymer team will not be reflected here (probably not a
> > > problem, but we should make sure to discuss that with UX if we haven't).
> > 
> > Yes. However, this is how Polymer recommends iconsets be created, so
> presumably
> > they won't update their SVGs. They would probably just add new ones instead.
> > (Otherwise it would be risky to ever use an icon, as it could change on you
> > without you realizing.)
> > 
> > > 2. We should at least document the source(s) of these.
> > 
> > I did. I'll make it clearer.
> The inline comment is good, but I think that a top level comment would also be
a
> good idea.
> 
> Well, changing without realizing is what you expect when using a library.
> Presumably the change would be to fix a bug or add clarity. That said this is
> generally fine.

Agreed + done in prior CL: http://crrev.com/1957313002.

https://codereview.chromium.org/1949113004/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_icons.html:16: </if>
On 2016/05/05 21:24:21, stevenjb wrote:
> On 2016/05/05 21:13:33, michaelpg wrote:
> > On 2016/05/05 16:29:07, stevenjb wrote:
> > > Would it make more sense to have a separate file for chromeos specific
> icons?
> > In
> > > general that is preferred where possible unless the difference is trivial.
> > 
> > I think it would be best not to. I suspect we will be moving some or all of
> > these to all platforms, which is easier to do by removing <if>s than by
> cutting
> > them from one file and pasting in another. That's also likely to lead to
> > duplication, i.e., having an icon both in cr_icons_chromeos.html and
> > cr_icons.html (which would result in it being duplicated in the CrOS build,
> for
> > no reason).
> 
> OK. Looking more closely I guess these aren't especially chromeos specific. I
> agree that we shouldn't have an icon in both, it would be in one or the other
> (and some html files might need to include both, same as with polymer icon
> sets).

I can confirm that the follow-ups remove most of the <if>s.

Settings icons will have a ton of <if>s if they're all in the same file, but
I'll let you look at that and LMKWYT at that time.

[stevenjb, 2016-05-09 16:13:48]

lgtm

[michaelpg, 2016-05-10 00:39:49]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2016-05-10 00:40:20]

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

[commit-bot: I haz the power, 2016-05-10 01:28:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-10 01:29:05]

Description was changed from

==========
Add icons for network cr-elements to cr-icons for Chrome OS.

This is a follow-up to crrev.com/1950163003 and crrev.com/1947533002.

BUG=605821
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Add icons for network cr-elements to cr-icons for Chrome OS.

This is a follow-up to crrev.com/1950163003 and crrev.com/1947533002.

BUG=605821
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/441c30696670030f8c941120f8aeeea51ee87d4b
Cr-Commit-Position: refs/heads/master@{#392507}
==========

[commit-bot: I haz the power, 2016-05-10 01:29:06]

Patchset 4 (id:??) landed as
https://crrev.com/441c30696670030f8c941120f8aeeea51ee87d4b
Cr-Commit-Position: refs/heads/master@{#392507}