Move more vector icons to more appropriate directories.

Move more vector icons to more appropriate directories.

Some of the icons are now duplicated because updating all references to
them at once is very onerous --- often, updating one icon requires
updating many other related icons, e.g. all infobar icons. These will be
de-duped soon.

BUG=626786
Review-Url: https://codereview.chromium.org/2685683004
Cr-Commit-Position: refs/heads/master@{#450549}
Committed: https://chromium.googlesource.com/chromium/src/+/1e235b69eb08d4d20006c5be62bc3cca37532801

[Evan Stade, 2017-02-09 01:20:07]

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

[commit-bot: I haz the power, 2017-02-09 01:20:46]

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-02-09 01:43:04]

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

[commit-bot: I haz the power, 2017-02-09 01:43:05]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Evan Stade, 2017-02-13 19:40:28]

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

[commit-bot: I haz the power, 2017-02-13 19:41:34]

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

[Evan Stade, 2017-02-13 20:11:17]

estade@chromium.org changed reviewers:
+ erickung@chromium.org, sky@chromium.org

[Evan Stade, 2017-02-13 20:11:17]

PTAL:

erickung:
  media/remoting/BUILD.gn
  media/remoting/DEPS
  media/remoting/interstitial.cc
sky:
  the rest

[sky, 2017-02-13 21:35:38]

https://codereview.chromium.org/2685683004/diff/20001/chrome/app/vector_icons...
File chrome/app/vector_icons/google_chrome/product.icon (right):

https://codereview.chromium.org/2685683004/diff/20001/chrome/app/vector_icons...
chrome/app/vector_icons/google_chrome/product.icon:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
Shouldn't this file be in src-internal?

https://codereview.chromium.org/2685683004/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_request_impl.cc (right):

https://codereview.chromium.org/2685683004/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_impl.cc:80: return
ui::kExtensionIcon;
How come this is using ExtensionIcon?

https://codereview.chromium.org/2685683004/diff/20001/ui/vector_icons/BUILD.gn
File ui/vector_icons/BUILD.gn (right):

https://codereview.chromium.org/2685683004/diff/20001/ui/vector_icons/BUILD.g...
ui/vector_icons/BUILD.gn:14: "error_circle.icon",
Why do we have this icons in ui instead of media?

[commit-bot: I haz the power, 2017-02-13 21:41:00]

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

[commit-bot: I haz the power, 2017-02-13 21:41:00]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2017-02-13 21:53:33]

https://codereview.chromium.org/2685683004/diff/20001/chrome/app/vector_icons...
File chrome/app/vector_icons/google_chrome/product.icon (right):

https://codereview.chromium.org/2685683004/diff/20001/chrome/app/vector_icons...
chrome/app/vector_icons/google_chrome/product.icon:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2017/02/13 21:35:38, sky wrote:
> Shouldn't this file be in src-internal?

I dunno, does it need to be? We have google_chrome_strings.grd in src/. I'm not
sure why we keep Google Chrome pngs in src-internal but maybe it's because of
trademark reasons?

https://codereview.chromium.org/2685683004/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_request_impl.cc (right):

https://codereview.chromium.org/2685683004/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_impl.cc:80: return
ui::kExtensionIcon;
On 2017/02/13 21:35:38, sky wrote:
> How come this is using ExtensionIcon?

just as a placeholder; it shouldn't be hit.

https://codereview.chromium.org/2685683004/diff/20001/ui/vector_icons/BUILD.gn
File ui/vector_icons/BUILD.gn (right):

https://codereview.chromium.org/2685683004/diff/20001/ui/vector_icons/BUILD.g...
ui/vector_icons/BUILD.gn:14: "error_circle.icon",
On 2017/02/13 21:35:38, sky wrote:
> Why do we have this icons in ui instead of media?

Mainly because it's a generic MD icon and therefore I suspect it will be reused
by some other piece of code eventually. In my experience when someone needs this
icon they will almost certainly not look in media/ for it and they'll end up
adding it twice.

[sky, 2017-02-13 23:28:43]

https://codereview.chromium.org/2685683004/diff/20001/chrome/app/vector_icons...
File chrome/app/vector_icons/google_chrome/product.icon (right):

https://codereview.chromium.org/2685683004/diff/20001/chrome/app/vector_icons...
chrome/app/vector_icons/google_chrome/product.icon:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2017/02/13 21:53:33, Evan Stade wrote:
> On 2017/02/13 21:35:38, sky wrote:
> > Shouldn't this file be in src-internal?
> 
> I dunno, does it need to be? We have google_chrome_strings.grd in src/. I'm
not
> sure why we keep Google Chrome pngs in src-internal but maybe it's because of
> trademark reasons?

I would view this the same as a png, and we keep the pngs in src-internal, so I
would think this should go in src-internal too. But I could be wrong here. Not
sure who to ask.

https://codereview.chromium.org/2685683004/diff/20001/ui/vector_icons/BUILD.gn
File ui/vector_icons/BUILD.gn (right):

https://codereview.chromium.org/2685683004/diff/20001/ui/vector_icons/BUILD.g...
ui/vector_icons/BUILD.gn:14: "error_circle.icon",
On 2017/02/13 21:53:33, Evan Stade wrote:
> On 2017/02/13 21:35:38, sky wrote:
> > Why do we have this icons in ui instead of media?
> 
> Mainly because it's a generic MD icon and therefore I suspect it will be
reused
> by some other piece of code eventually. In my experience when someone needs
this
> icon they will almost certainly not look in media/ for it and they'll end up
> adding it twice.

Sorry for not being clear. I mean all these icons. They seem media specific and
should be in media, not here.

[Evan Stade, 2017-02-13 23:51:47]

https://codereview.chromium.org/2685683004/diff/20001/chrome/app/vector_icons...
File chrome/app/vector_icons/google_chrome/product.icon (right):

https://codereview.chromium.org/2685683004/diff/20001/chrome/app/vector_icons...
chrome/app/vector_icons/google_chrome/product.icon:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2017/02/13 23:28:43, sky wrote:
> On 2017/02/13 21:53:33, Evan Stade wrote:
> > On 2017/02/13 21:35:38, sky wrote:
> > > Shouldn't this file be in src-internal?
> > 
> > I dunno, does it need to be? We have google_chrome_strings.grd in src/. I'm
> not
> > sure why we keep Google Chrome pngs in src-internal but maybe it's because
of
> > trademark reasons?
> 
> I would view this the same as a png, and we keep the pngs in src-internal, so
I
> would think this should go in src-internal too. But I could be wrong here. Not
> sure who to ask.

I can send a note to chromium-dev but this is not a change; the file is just
being copied from another one that's already in src/ (see note about dupes in cl
desc.).

https://codereview.chromium.org/2685683004/diff/20001/ui/vector_icons/BUILD.gn
File ui/vector_icons/BUILD.gn (right):

https://codereview.chromium.org/2685683004/diff/20001/ui/vector_icons/BUILD.g...
ui/vector_icons/BUILD.gn:14: "error_circle.icon",
On 2017/02/13 23:28:43, sky wrote:
> On 2017/02/13 21:53:33, Evan Stade wrote:
> > On 2017/02/13 21:35:38, sky wrote:
> > > Why do we have this icons in ui instead of media?
> > 
> > Mainly because it's a generic MD icon and therefore I suspect it will be
> reused
> > by some other piece of code eventually. In my experience when someone needs
> this
> > icon they will almost certainly not look in media/ for it and they'll end up
> > adding it twice.
> 
> Sorry for not being clear. I mean all these icons. They seem media specific
and
> should be in media, not here.

Which ones do you mean? Location, extension, notifications, protocol etc. are
not related to media. The media router ones are used in browser ui as well as
being referenced in media/remoting. c/b doesn't depend on media/. Should it? I
dunno.

[sky, 2017-02-14 00:37:38]

On Mon, Feb 13, 2017 at 3:51 PM,  <estade@chromium.org> wrote:
>
>
https://codereview.chromium.org/2685683004/diff/20001/chrome/app/vector_icons...
> File chrome/app/vector_icons/google_chrome/product.icon (right):
>
>
https://codereview.chromium.org/2685683004/diff/20001/chrome/app/vector_icons...
> chrome/app/vector_icons/google_chrome/product.icon:1: // Copyright 2015
> The Chromium Authors. All rights reserved.
> On 2017/02/13 23:28:43, sky wrote:
>> On 2017/02/13 21:53:33, Evan Stade wrote:
>> > On 2017/02/13 21:35:38, sky wrote:
>> > > Shouldn't this file be in src-internal?
>> >
>> > I dunno, does it need to be? We have google_chrome_strings.grd in
> src/. I'm
>> not
>> > sure why we keep Google Chrome pngs in src-internal but maybe it's
> because of
>> > trademark reasons?
>>
>> I would view this the same as a png, and we keep the pngs in
> src-internal, so I
>> would think this should go in src-internal too. But I could be wrong
> here. Not
>> sure who to ask.
>
> I can send a note to chromium-dev but this is not a change; the file is
> just being copied from another one that's already in src/ (see note
> about dupes in cl desc.).

Ok, fair enough.

>
>
https://codereview.chromium.org/2685683004/diff/20001/ui/vector_icons/BUILD.gn
> File ui/vector_icons/BUILD.gn (right):
>
>
https://codereview.chromium.org/2685683004/diff/20001/ui/vector_icons/BUILD.g...
> ui/vector_icons/BUILD.gn:14: "error_circle.icon",
> On 2017/02/13 23:28:43, sky wrote:
>> On 2017/02/13 21:53:33, Evan Stade wrote:
>> > On 2017/02/13 21:35:38, sky wrote:
>> > > Why do we have this icons in ui instead of media?
>> >
>> > Mainly because it's a generic MD icon and therefore I suspect it
> will be
>> reused
>> > by some other piece of code eventually. In my experience when
> someone needs
>> this
>> > icon they will almost certainly not look in media/ for it and
> they'll end up
>> > adding it twice.
>>
>> Sorry for not being clear. I mean all these icons. They seem media
> specific and
>> should be in media, not here.
>
> Which ones do you mean? Location, extension, notifications, protocol
> etc. are not related to media. The media router ones are used in browser
> ui as well as being referenced in media/remoting. c/b doesn't depend on
> media/. Should it? I dunno.

We should strive for grouping icons where used, near related code. It
seems like the media files should be moved to media/vector_icons.
Others that are only used in chrome should move to chrome. Is there a
reason not to do this?

ui should be solely about ui related building blocks that other
applications uses. ui/views and ui/aura are good examples of this.

  -Scott

>
> https://codereview.chromium.org/2685683004/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Dan Beam, 2017-02-14 01:41:55]

https://codereview.chromium.org/2685683004/diff/20001/chrome/app/vector_icons...
File chrome/app/vector_icons/google_chrome/product.icon (right):

https://codereview.chromium.org/2685683004/diff/20001/chrome/app/vector_icons...
chrome/app/vector_icons/google_chrome/product.icon:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2017/02/13 23:51:47, Evan Stade wrote:
> On 2017/02/13 23:28:43, sky wrote:
> > On 2017/02/13 21:53:33, Evan Stade wrote:
> > > On 2017/02/13 21:35:38, sky wrote:
> > > > Shouldn't this file be in src-internal?
> > > 
> > > I dunno, does it need to be? We have google_chrome_strings.grd in src/.
I'm
> > not
> > > sure why we keep Google Chrome pngs in src-internal but maybe it's because
> of
> > > trademark reasons?
> > 
> > I would view this the same as a png, and we keep the pngs in src-internal,
so
> I
> > would think this should go in src-internal too. But I could be wrong here.
Not
> > sure who to ask.
> 
> I can send a note to chromium-dev but this is not a change; the file is just
> being copied from another one that's already in src/ (see note about dupes in
cl
> desc.).

for what it's worth, I think it'd be very hard to change this in public code if
we ever wanted to rebrand, so keeping in src-internal also sounds sane to me

[Evan Stade, 2017-02-14 16:17:04]

> We should strive for grouping icons where used, near related code. It
> seems like the media files should be moved to media/vector_icons.
> Others that are only used in chrome should move to chrome. Is there a
> reason not to do this?
> 
> ui should be solely about ui related building blocks that other
> applications uses. ui/views and ui/aura are good examples of this.

Icons like videocam and microphone are pulled directly from the stock MD icon
archive. They are building blocks provided by other projects that Chrome is
using, so it makes sense to me they'd be available for reuse by other apps or
components like //ash. In fact I'm pretty sure that png equivalents for
microphone and some of the others are being used in ash, in places like the
message center and app launcher, and they would ideally be updated to use vector
versions.

Of the ones that start with media_, only two of them are used in media/, but I
didn't want to separate related icons. Do you want me to move all 4 of those to
//media?

https://codereview.chromium.org/2685683004/diff/20001/chrome/app/vector_icons...
File chrome/app/vector_icons/google_chrome/product.icon (right):

https://codereview.chromium.org/2685683004/diff/20001/chrome/app/vector_icons...
chrome/app/vector_icons/google_chrome/product.icon:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2017/02/14 01:41:55, Dan Beam wrote:
> On 2017/02/13 23:51:47, Evan Stade wrote:
> > On 2017/02/13 23:28:43, sky wrote:
> > > On 2017/02/13 21:53:33, Evan Stade wrote:
> > > > On 2017/02/13 21:35:38, sky wrote:
> > > > > Shouldn't this file be in src-internal?
> > > > 
> > > > I dunno, does it need to be? We have google_chrome_strings.grd in src/.
> I'm
> > > not
> > > > sure why we keep Google Chrome pngs in src-internal but maybe it's
because
> > of
> > > > trademark reasons?
> > > 
> > > I would view this the same as a png, and we keep the pngs in src-internal,
> so
> > I
> > > would think this should go in src-internal too. But I could be wrong here.
> Not
> > > sure who to ask.
> > 
> > I can send a note to chromium-dev but this is not a change; the file is just
> > being copied from another one that's already in src/ (see note about dupes
in
> cl
> > desc.).
> 
> for what it's worth, I think it'd be very hard to change this in public code
if
> we ever wanted to rebrand, so keeping in src-internal also sounds sane to me

I don't know of any secret branding roll outs that Google Chrome has ever had,
but if we did want to do that we could utilize src-internal then regardless of
what we do now (and we'd have to worry about the strings as well at that time).
Please weigh in with these concerns when I start the email thread.

[sky, 2017-02-14 18:15:08]

Ok, you convinced me, LGTM

[miu, 2017-02-14 21:26:33]

miu@chromium.org changed reviewers:
+ miu@chromium.org
- erickung@chromium.org

[miu, 2017-02-14 21:26:34]

media/remoting lgtm

[Evan Stade, 2017-02-14 22:08:09]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2017-02-14 22:09:27]

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-02-14 22:14:01]

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

[commit-bot: I haz the power, 2017-02-14 22:14:02]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Evan Stade, 2017-02-14 23:23:05]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-02-14 23:23:06]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
miu@chromium.org
Link to the patchset: https://codereview.chromium.org/2685683004/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-14 23:23:57]

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-02-15 01:40:38]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487114585894160,
"parent_rev": "7bbeadb70f71ff8809edb8b7b4c3c5b34ea49db1", "commit_rev":
"1e235b69eb08d4d20006c5be62bc3cca37532801"}

[commit-bot: I haz the power, 2017-02-15 01:44:43]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487114585894160,
"parent_rev": "7bbeadb70f71ff8809edb8b7b4c3c5b34ea49db1", "commit_rev":
"1e235b69eb08d4d20006c5be62bc3cca37532801"}

[commit-bot: I haz the power, 2017-02-15 01:44:46]

Description was changed from

==========
Move more vector icons to more appropriate directories.

Some of the icons are now duplicated because updating all references to
them at once is very onerous --- often, updating one icon requires
updating many other related icons, e.g. all infobar icons. These will be
de-duped soon.

BUG=626786
==========

to

==========
Move more vector icons to more appropriate directories.

Some of the icons are now duplicated because updating all references to
them at once is very onerous --- often, updating one icon requires
updating many other related icons, e.g. all infobar icons. These will be
de-duped soon.

BUG=626786

Review-Url: https://codereview.chromium.org/2685683004
Cr-Commit-Position: refs/heads/master@{#450549}
Committed:
https://chromium.googlesource.com/chromium/src/+/1e235b69eb08d4d20006c5be62bc...
==========

[commit-bot: I haz the power, 2017-02-15 01:44:47]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/1e235b69eb08d4d20006c5be62bc...