Move around more vector icons.

Move around more vector icons.

Aside from mechanical changes, this does the following:
- adds vector icons to relevant libraries in components/
- moves initialization work (including virtual fn calls) from BubbleIconView ctor to Init().

BUG=626786
Review-Url: https://codereview.chromium.org/2644903004
Cr-Commit-Position: refs/heads/master@{#446200}
Committed: https://chromium.googlesource.com/chromium/src/+/50d9301aee1d5794a5c89f40992ff372995f60e3

[Evan Stade, 2017-01-19 23:29:16]

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

[commit-bot: I haz the power, 2017-01-19 23:29:57]

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-01-19 23:39:02]

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

[commit-bot: I haz the power, 2017-01-19 23:39:03]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  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...)

[Evan Stade, 2017-01-20 01:36:32]

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

[commit-bot: I haz the power, 2017-01-20 01:37:01]

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-01-20 01:43:59]

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

[commit-bot: I haz the power, 2017-01-20 01:44:00]

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_...)
  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-01-20 01:55:21]

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

[commit-bot: I haz the power, 2017-01-20 01:56:17]

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-01-20 02:04:25]

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

[commit-bot: I haz the power, 2017-01-20 02:05:03]

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-01-20 03:05:53]

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

[commit-bot: I haz the power, 2017-01-20 03:05:54]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2017-01-20 15:42:36]

Description was changed from

==========
Move around more vector icons.

Aside from mechanical changes, this does the following:
- adds vector icon target for components/
- changes vector icon targets from source_set to shared_library
- moves initialization work (including virtual fn calls) from BubbleIconView
ctor to Init().

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

to

==========
Move around more vector icons.

Aside from mechanical changes, this does the following:
- adds vector icon target for components/
- moves initialization work (including virtual fn calls) from BubbleIconView
ctor to Init().

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

[Evan Stade, 2017-01-20 15:42:39]

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

[Evan Stade, 2017-01-20 15:42:40]

+sky for review
+tdanderson for possible comment on components/resources/vector_icons/BUILD.gn

This one is a little bit less of a cookie cutter because to get Windows to
compile it seems to be necessary to use component() (rather than static_library
or source_set) and to get Android/iOS to compile (and not have gn check
complain) we need an empty definition for VectorIcon. But aside from the build
changes the majority of the differences are still mechanical.

[sky, 2017-01-20 17:35:36]

What is the reason for Init() vs the constructor?

[Evan Stade, 2017-01-20 17:38:15]

On 2017/01/20 17:35:36, sky wrote:
> What is the reason for Init() vs the constructor?

AddChildView triggers calls to virtual functions, including GetVectorIcon. That
one is now pure virtual (for BubbleIconView) and this causes a crash.

[sky, 2017-01-20 18:07:05]

Thanks for the clarification. One more question. What is component/resources
for? I would think each component would contain it's own set of resources.

[Evan Stade, 2017-01-21 00:47:13]

On 2017/01/20 18:07:05, sky wrote:
> Thanks for the clarification. One more question. What is component/resources
> for? I would think each component would contain it's own set of resources.

I dunno, perhaps it's there to allow outsiders to depend on the resources
without depending on the rest of the code (which could create circular
references). I was following that model, but it occurs to me that it might be
simpler if we just tack on these icons to the targets we already have for
components --- since they compile down to code there's no need to create
separate targets. I think this way is a bit simpler. I'll ping when the bots are
green again (which might be a while).

[Evan Stade, 2017-01-21 00:47:22]

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

[commit-bot: I haz the power, 2017-01-21 00:47:45]

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-01-21 00:54:14]

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

[commit-bot: I haz the power, 2017-01-21 00:54:15]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  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...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Evan Stade, 2017-01-23 16:38:40]

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

[commit-bot: I haz the power, 2017-01-23 16:38:51]

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-01-23 17:46:32]

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

[commit-bot: I haz the power, 2017-01-23 17:46:33]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Evan Stade, 2017-01-24 00:00:18]

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

[commit-bot: I haz the power, 2017-01-24 00:01:31]

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-01-24 00:50:19]

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

[commit-bot: I haz the power, 2017-01-24 00:50:58]

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-01-24 01:49:15]

Description was changed from

==========
Move around more vector icons.

Aside from mechanical changes, this does the following:
- adds vector icon target for components/
- moves initialization work (including virtual fn calls) from BubbleIconView
ctor to Init().

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

to

==========
Move around more vector icons.

Aside from mechanical changes, this does the following:
- adds vector icons to relevant libraries in components/
- moves initialization work (including virtual fn calls) from BubbleIconView
ctor to Init().

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

[Evan Stade, 2017-01-24 01:51:40]

ok, looks like it's working. I broke from the convention of creating a distinct
build target; as these are code and not actual PNG assets I see no reason to
create a separate (potentially very small) target for each component, or to
follow the example of a single large target as set by components/resources/.

[commit-bot: I haz the power, 2017-01-24 01:58:31]

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

[commit-bot: I haz the power, 2017-01-24 01:58:32]

Dry run: This issue passed the CQ dry run.

[sky, 2017-01-24 17:33:02]

I'm not an owner of the components directory you're changing. Everything else
looks reasonable.

https://codereview.chromium.org/2644903004/diff/160001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/location_bar_decoration.h (right):

https://codereview.chromium.org/2644903004/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/location_bar_decoration.h:163: // Gets the
id of the decoration's Material Design vector icon. Subclasses
update 'id'.

https://codereview.chromium.org/2644903004/diff/160001/ui/views/resources/vec...
File ui/views/resources/vector_icons/vector_icons.h.template (right):

https://codereview.chromium.org/2644903004/diff/160001/ui/views/resources/vec...
ui/views/resources/vector_icons/vector_icons.h.template:26: #undef
VECTOR_ICON_TEMPLATE_H
Why do you undef in all these files? Wouldn't that mean VECTOR_ICON_TEMPLATE_H
isn't defined? No doubt no macro skills are weak.

[Evan Stade, 2017-01-24 19:00:30]

estade@chromium.org changed reviewers:
+ brettw@chromium.org

[Evan Stade, 2017-01-24 19:00:31]

+brettw for components/ (particularly changes to BUILD files)

https://codereview.chromium.org/2644903004/diff/160001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/location_bar_decoration.h (right):

https://codereview.chromium.org/2644903004/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/location_bar_decoration.h:163: // Gets the
id of the decoration's Material Design vector icon. Subclasses
On 2017/01/24 17:33:02, sky wrote:
> update 'id'.

Done.

https://codereview.chromium.org/2644903004/diff/160001/ui/views/resources/vec...
File ui/views/resources/vector_icons/vector_icons.h.template (right):

https://codereview.chromium.org/2644903004/diff/160001/ui/views/resources/vec...
ui/views/resources/vector_icons/vector_icons.h.template:26: #undef
VECTOR_ICON_TEMPLATE_H
On 2017/01/24 17:33:02, sky wrote:
> Why do you undef in all these files? Wouldn't that mean VECTOR_ICON_TEMPLATE_H
> isn't defined? No doubt no macro skills are weak.

The template is only used in this header. The script replaces
TEMPLATE_PLACEHOLDER (which not a macro) with a bunch of icon definitions that
make use of the macro. The macro is un-deffed at the end because if a single .cc
file somewhere includes two vector_icons.h files (e.g. one in components and one
in chrome/app) then the macro would be defined twice.

[Evan Stade, 2017-01-24 21:56:15]

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

[commit-bot: I haz the power, 2017-01-24 21:56:33]

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

[sky, 2017-01-24 21:58:17]

LGTM

[commit-bot: I haz the power, 2017-01-24 23:26:27]

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

[commit-bot: I haz the power, 2017-01-24 23:26:29]

Dry run: This issue passed the CQ dry run.

[brettw, 2017-01-25 22:46:49]

components lgtm

[Evan Stade, 2017-01-26 00:07:52]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2017-01-26 00:08:48]

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-01-26 01:48:10]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1485389272109790,
"parent_rev": "39f5ad11373872bbb7e398fb7943b289cc790c39", "commit_rev":
"50d9301aee1d5794a5c89f40992ff372995f60e3"}

[commit-bot: I haz the power, 2017-01-26 01:48:41]

Description was changed from

==========
Move around more vector icons.

Aside from mechanical changes, this does the following:
- adds vector icons to relevant libraries in components/
- moves initialization work (including virtual fn calls) from BubbleIconView
ctor to Init().

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

to

==========
Move around more vector icons.

Aside from mechanical changes, this does the following:
- adds vector icons to relevant libraries in components/
- moves initialization work (including virtual fn calls) from BubbleIconView
ctor to Init().

BUG=626786

Review-Url: https://codereview.chromium.org/2644903004
Cr-Commit-Position: refs/heads/master@{#446200}
Committed:
https://chromium.googlesource.com/chromium/src/+/50d9301aee1d5794a5c89f40992f...
==========

[commit-bot: I haz the power, 2017-01-26 01:48:43]

Committed patchset #10 (id:170001) as
https://chromium.googlesource.com/chromium/src/+/50d9301aee1d5794a5c89f40992f...