Use a response file for aggregate_vector_icons in GN

Use a response file for aggregate_vector_icons in GN

Modify the aggregate_vector_icons step in
ui/gfx/BUILD.gn to accept a response file
containing only the desired .icon files to
process rather than have every build configuration
process and include all .icon files. This will
make it possible to separate icons into
subdirectories and conditionally process them
on a per-platform basis.

BUG=535386
TEST=builds with GN and GYP

Committed: https://crrev.com/3664dfc4b6d8c46708c7ad2a9d95b8eb6ac710b1
Cr-Commit-Position: refs/heads/master@{#404512}

[tdanderson, 2016-07-06 18:23:23]

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

[tdanderson, 2016-07-06 18:23:24]

Brett and Evan, can you please take a first look at this CL and my inline
comments?

https://codereview.chromium.org/2129683002/diff/1/.gn
File .gn (right):

https://codereview.chromium.org/2129683002/diff/1/.gn#newcode334
.gn:334: "//ui/gfx/BUILD.gn",
Based on the stern warning I received when adding exec_script() to
ui/gfx/BUILD.gn, I recognize this is probably not what we want to be doing.
However I was unsuccessful in getting other approaches to work (i.e., not
storing the files in .gypi format and needing to call exec_script()). Any
guidance you can provide here would be appreciated.

https://codereview.chromium.org/2129683002/diff/1/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/2129683002/diff/1/ui/gfx/BUILD.gn#newcode25
ui/gfx/BUILD.gn:25: [ rebase_path("vector_icons_sources.gypi") ],
"vector_icons_sources.gypi" hasn't been added to |sources| anywhere within this
file. Should it be?

https://codereview.chromium.org/2129683002/diff/1/ui/gfx/vector_icons/aggrega...
File ui/gfx/vector_icons/aggregate_vector_icons.py (right):

https://codereview.chromium.org/2129683002/diff/1/ui/gfx/vector_icons/aggrega...
ui/gfx/vector_icons/aggregate_vector_icons.py:92: #    returns "foo_bar_baz".
baz.icon will have a VectorIconId of FOO_BAR_BAZ.
When this lands I will take care of updating the public documentation on
chromium.org explaining how icons are separated into subdirectories, the
convention used for VectorIconIds, etc.

https://codereview.chromium.org/2129683002/diff/1/ui/gfx/vector_icons_sources...
File ui/gfx/vector_icons_sources.gypi (right):

https://codereview.chromium.org/2129683002/diff/1/ui/gfx/vector_icons_sources...
ui/gfx/vector_icons_sources.gypi:8: 'vector_icons/ash/kittens.icon',
This is just a proof-of-concept. Let's land this with everything listed
underneath the 'common' variable, and separate out icons in follow-up CLs (I
believe 'ash' and 'mac' are currently the only two that apply).

[Evan Stade, 2016-07-06 20:54:49]

awesome!!

https://codereview.chromium.org/2129683002/diff/1/ui/gfx/vector_icons/aggrega...
File ui/gfx/vector_icons/aggregate_vector_icons.py (right):

https://codereview.chromium.org/2129683002/diff/1/ui/gfx/vector_icons/aggrega...
ui/gfx/vector_icons/aggregate_vector_icons.py:85: # different directories will
have a different VectorIconId in |output_cc|
I would say we should ban duplicate .icon file names in a single build (even in
different directories). Can we just fail during compilation if people try to add
a dupe .icon filename?

[tdanderson, 2016-07-06 23:52:00]

https://codereview.chromium.org/2129683002/diff/1/ui/gfx/vector_icons/aggrega...
File ui/gfx/vector_icons/aggregate_vector_icons.py (right):

https://codereview.chromium.org/2129683002/diff/1/ui/gfx/vector_icons/aggrega...
ui/gfx/vector_icons/aggregate_vector_icons.py:85: # different directories will
have a different VectorIconId in |output_cc|
On 2016/07/06 20:54:49, Evan Stade wrote:
> I would say we should ban duplicate .icon file names in a single build (even
in
> different directories). Can we just fail during compilation if people try to
add
> a dupe .icon filename?

I considered that, but my preference is to allow duplicate filenames across
subdirectories; I think it scales better and will end up being less hassle for
users.

[Evan Stade, 2016-07-07 15:13:46]

On 2016/07/06 23:52:00, tdanderson wrote:
>
https://codereview.chromium.org/2129683002/diff/1/ui/gfx/vector_icons/aggrega...
> File ui/gfx/vector_icons/aggregate_vector_icons.py (right):
> 
>
https://codereview.chromium.org/2129683002/diff/1/ui/gfx/vector_icons/aggrega...
> ui/gfx/vector_icons/aggregate_vector_icons.py:85: # different directories will
> have a different VectorIconId in |output_cc|
> On 2016/07/06 20:54:49, Evan Stade wrote:
> > I would say we should ban duplicate .icon file names in a single build (even
> in
> > different directories). Can we just fail during compilation if people try to
> add
> > a dupe .icon filename?
> 
> I considered that, but my preference is to allow duplicate filenames across
> subdirectories; I think it scales better and will end up being less hassle for
> users.

Scales better how? This would allow us to have two menu.icon files, for example,
but that's confusing. We should come up with a better/more specific name for
such an icon instead of prepending ASH. What .icon filename do you foresee
adding that /should/ be duplicated? Do we have any png assets with the same name
in different directories?

[brettw, 2016-07-07 19:28:34]

lgtm

https://codereview.chromium.org/2129683002/diff/1/.gn
File .gn (right):

https://codereview.chromium.org/2129683002/diff/1/.gn#newcode334
.gn:334: "//ui/gfx/BUILD.gn",
I'm OK with this for now since the list is very long and you're going to be
making changes to it. We should be able to delete all of these with GYP compat
shortly anyway.

https://codereview.chromium.org/2129683002/diff/1/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/2129683002/diff/1/ui/gfx/BUILD.gn#newcode25
ui/gfx/BUILD.gn:25: [ rebase_path("vector_icons_sources.gypi") ],
On 2016/07/06 18:23:23, tdanderson wrote:
> "vector_icons_sources.gypi" hasn't been added to |sources| anywhere within
this
> file. Should it be?

This is actually what the 4th parameter to this function is for. It tells GN to
re-run the build when the file changes. This looks OK to me. We should be able
to delete the .gypi and put the file list in here when we delete GYP (hopefully
soon).

https://codereview.chromium.org/2129683002/diff/1/ui/gfx/BUILD.gn#newcode423
ui/gfx/BUILD.gn:423: inputs += rebase_path(vector_icons_sources_gypi.ash)
It looks like the file names in the .gypi are already relative to the current
directory of this BUILD file, so you shouldn't need any rebase_path calls here
(next one also).

[tdanderson, 2016-07-07 23:22:16]

On 2016/07/07 15:13:46, Evan Stade wrote:
> On 2016/07/06 23:52:00, tdanderson wrote:
> >
>
https://codereview.chromium.org/2129683002/diff/1/ui/gfx/vector_icons/aggrega...
> > File ui/gfx/vector_icons/aggregate_vector_icons.py (right):
> > 
> >
>
https://codereview.chromium.org/2129683002/diff/1/ui/gfx/vector_icons/aggrega...
> > ui/gfx/vector_icons/aggregate_vector_icons.py:85: # different directories
will
> > have a different VectorIconId in |output_cc|
> > On 2016/07/06 20:54:49, Evan Stade wrote:
> > > I would say we should ban duplicate .icon file names in a single build
(even
> > in
> > > different directories). Can we just fail during compilation if people try
to
> > add
> > > a dupe .icon filename?
> > 
> > I considered that, but my preference is to allow duplicate filenames across
> > subdirectories; I think it scales better and will end up being less hassle
for
> > users.
> 
> Scales better how? This would allow us to have two menu.icon files, for
example,
> but that's confusing.

Why do you think that would be confusing? For anyone
browsing through the vector_icons/ subdirectory tree,
the location of the icon provides the needed context.
For anyone using these icons, their VectorIconId
gives sufficient context.

> We should come up with a better/more specific name for
> such an icon instead of prepending ASH.

Consider the icons I'm adding for the Ash status tray and 
the system menu. Right now they are named 
system_menu_caps_lock.icon and
system_tray_caps_lock.icon. With my suggestion they could
be organized as:

ui/gfx/vector_icons/ash/system_tray/caps_lock.icon
and
ui/gfx/vector_icons/ash/system_menu/caps_lock.icon

As a result, we get a better/more specific name for free
(the VectorIconIds will be ASH_SYSTEM_TRAY_CAPS_LOCK
and ASH_SYSTEM_MENU_CAPS_LOCK) without enforcing the
restriction of globally-unique filenames.

> What .icon filename do you foresee adding that /should/ be duplicated? 

Aside from the caps_lock example, I think it's reasonable for
anything that has different-looking variants across
platforms to have a common filename. 

Not enforcing globally unique filenames lets people name
the .icon file something meaningful within the context
of a single directory, without needing to worry about the
whole system. Consider the following (made up) files:

ui/gfx/vector_icons/ash/controls/power.icon
ui/gfx/vector_icons/ash/controls/lock.icon
ui/gfx/vector_icons/ash/controls/sign_out.icon
ui/gfx/vector_icons/omnibox/lock.icon
ui/gfx/vector_icons/omnibox/star.icon

It seems like an unnecessary burden to have to rename
one of the lock.icon files to be something else.

> Do we have any png assets with the same name
> in different directories?

Yes, there are 90 instances of icons with the same name in
different directories (and this is after de-duping
the default_100_percent vs default_200_percent versions
and not counting the ones used in tests). You can see
the filenames along with how many files have that name
here: https://paste.googleplex.com/6732912837787648

[Evan Stade, 2016-07-08 02:42:27]

On 2016/07/07 23:22:16, tdanderson wrote:
> On 2016/07/07 15:13:46, Evan Stade wrote:
> > On 2016/07/06 23:52:00, tdanderson wrote:
> > >
> >
>
https://codereview.chromium.org/2129683002/diff/1/ui/gfx/vector_icons/aggrega...
> > > File ui/gfx/vector_icons/aggregate_vector_icons.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2129683002/diff/1/ui/gfx/vector_icons/aggrega...
> > > ui/gfx/vector_icons/aggregate_vector_icons.py:85: # different directories
> will
> > > have a different VectorIconId in |output_cc|
> > > On 2016/07/06 20:54:49, Evan Stade wrote:
> > > > I would say we should ban duplicate .icon file names in a single build
> (even
> > > in
> > > > different directories). Can we just fail during compilation if people
try
> to
> > > add
> > > > a dupe .icon filename?
> > > 
> > > I considered that, but my preference is to allow duplicate filenames
across
> > > subdirectories; I think it scales better and will end up being less hassle
> for
> > > users.
> > 
> > Scales better how? This would allow us to have two menu.icon files, for
> example,
> > but that's confusing.
> 
> Why do you think that would be confusing? For anyone
> browsing through the vector_icons/ subdirectory tree,
> the location of the icon provides the needed context.
> For anyone using these icons, their VectorIconId
> gives sufficient context.

There used to be two totally different BubbleDelegate classes. (Luckily I was
able to factor one of them out of existence.) They had different directories and
different namespaces, but I still got confused constantly by references to
BubbleDelegate in comments or other class's names.

> 
> > We should come up with a better/more specific name for
> > such an icon instead of prepending ASH.
> 
> Consider the icons I'm adding for the Ash status tray and 
> the system menu. Right now they are named 
> system_menu_caps_lock.icon and
> system_tray_caps_lock.icon. With my suggestion they could
> be organized as:
> 
> ui/gfx/vector_icons/ash/system_tray/caps_lock.icon
> and
> ui/gfx/vector_icons/ash/system_menu/caps_lock.icon
> 
> As a result, we get a better/more specific name for free
> (the VectorIconIds will be ASH_SYSTEM_TRAY_CAPS_LOCK
> and ASH_SYSTEM_MENU_CAPS_LOCK) without enforcing the
> restriction of globally-unique filenames.

I'm not sure how this is "free". All you've done is replaced an underscore with
a slash.

> 
> > What .icon filename do you foresee adding that /should/ be duplicated? 
> 
> Aside from the caps_lock example, I think it's reasonable for
> anything that has different-looking variants across
> platforms to have a common filename. 

Yes, but there'd be no conflict there because you'd only get one of them per
platform.

> 
> Not enforcing globally unique filenames lets people name
> the .icon file something meaningful within the context
> of a single directory, without needing to worry about the
> whole system. Consider the following (made up) files:
> 
> ui/gfx/vector_icons/ash/controls/power.icon
> ui/gfx/vector_icons/ash/controls/lock.icon
> ui/gfx/vector_icons/ash/controls/sign_out.icon
> ui/gfx/vector_icons/omnibox/lock.icon
> ui/gfx/vector_icons/omnibox/star.icon
> 
> It seems like an unnecessary burden to have to rename
> one of the lock.icon files to be something else.
> 
> > Do we have any png assets with the same name
> > in different directories?
> 
> Yes, there are 90 instances of icons with the same name in
> different directories (and this is after de-duping
> the default_100_percent vs default_200_percent versions
> and not counting the ones used in tests). You can see
> the filenames along with how many files have that name
> here: https://paste.googleplex.com/6732912837787648

Looks like many (most?) of those are cases where *different* platforms, i.e.
different compiles, each have a foo.png. Those are not cases where a single
compile has two different foo.pngs.

[Evan Stade, 2016-07-08 02:49:12]

now that I think about it, this presents an actual functional difficulty.
Consider the case (that you already mentioned) where two platforms have
variations on a single icon. The bookmark folder icons are an example of this
because on Windows they're different than elsewhere. So then you'd have:

vector_icons/win/bookmark_folder.icon
vector_icons/not_win/bookmark_folder.icon

(not_win/ probably wouldn't exist, I guess instead you'd use a os!=win
conditional in the gypi .icon list, but anyway...)

in code you can then refer to both as BOOKMARK_FOLDER and magically get the
right one. Otherwise how do you get the right one? use some platform ifdefs?

[tdanderson, 2016-07-08 21:10:11]

Description was changed from

==========
Separate vector icons into subdirectories

Proof-of-concept CL to show how .icon files in
ui/gfx/vector_icons can be separated into
platform-specific subdirectories. This CL
modifies the aggregate_vector_icons step in
ui/gfx/BUILD.gn to accept a response file
containing only the desired .icon files to
process rather than have every build configuration
process and include all .icon files.

To guard against VectorIconId naming collisions,
IDs will be prepended with their subdirectory
path, if applicable. For instance,
ui/gfx/vector_icons/baz.icon will have the
VectorIconId of BAZ, and
ui/gfx/vector_icons/foo/bar/baz.icon will have
the VectorIconId of FOO_BAR_BAZ.

BUG=535386
TEST=builds with GN and GYP
==========

to

==========
Use a response file for aggregate_vector_icons in GN

Modify the aggregate_vector_icons step in
ui/gfx/BUILD.gn to accept a response file
containing only the desired .icon files to
process rather than have every build configuration
process and include all .icon files. This will
make it possible to separate icons into
subdirectories and conditionally process them
on a per-platform basis.

BUG=535386
TEST=builds with GN and GYP
==========

[tdanderson, 2016-07-08 21:20:37]

https://codereview.chromium.org/2129683002/diff/1/.gn
File .gn (right):

https://codereview.chromium.org/2129683002/diff/1/.gn#newcode334
.gn:334: "//ui/gfx/BUILD.gn",
On 2016/07/07 19:28:33, brettw (plz ping after 24h) wrote:
> I'm OK with this for now since the list is very long and you're going to be
> making changes to it. We should be able to delete all of these with GYP compat
> shortly anyway.

Acknowledged.

https://codereview.chromium.org/2129683002/diff/1/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/2129683002/diff/1/ui/gfx/BUILD.gn#newcode25
ui/gfx/BUILD.gn:25: [ rebase_path("vector_icons_sources.gypi") ],
On 2016/07/07 19:28:33, brettw (plz ping after 24h) wrote:
> On 2016/07/06 18:23:23, tdanderson wrote:
> > "vector_icons_sources.gypi" hasn't been added to |sources| anywhere within
> this
> > file. Should it be?
> 
> This is actually what the 4th parameter to this function is for. It tells GN
to
> re-run the build when the file changes. This looks OK to me. We should be able
> to delete the .gypi and put the file list in here when we delete GYP
(hopefully
> soon).

Acknowledged.

https://codereview.chromium.org/2129683002/diff/1/ui/gfx/BUILD.gn#newcode423
ui/gfx/BUILD.gn:423: inputs += rebase_path(vector_icons_sources_gypi.ash)
On 2016/07/07 19:28:33, brettw (plz ping after 24h) wrote:
> It looks like the file names in the .gypi are already relative to the current
> directory of this BUILD file, so you shouldn't need any rebase_path calls here
> (next one also).

Done.

https://codereview.chromium.org/2129683002/diff/1/ui/gfx/vector_icons/aggrega...
File ui/gfx/vector_icons/aggregate_vector_icons.py (right):

https://codereview.chromium.org/2129683002/diff/1/ui/gfx/vector_icons/aggrega...
ui/gfx/vector_icons/aggregate_vector_icons.py:85: # different directories will
have a different VectorIconId in |output_cc|
On 2016/07/06 23:52:00, tdanderson wrote:
> On 2016/07/06 20:54:49, Evan Stade wrote:
> > I would say we should ban duplicate .icon file names in a single build (even
> in
> > different directories). Can we just fail during compilation if people try to
> add
> > a dupe .icon filename?
> 
> I considered that, but my preference is to allow duplicate filenames across
> subdirectories; I think it scales better and will end up being less hassle for
> users.

Based on the discussion between Evan and I here, Evan's suggestion of expecting
unique filenames per build looks to be the better approach (instead of
prepending the file path). I've removed prependIconPath(). The enforcement of
unique filenames per build will be enforced for free at compile time (since
duplicate filenames will show up as identical values in the VectorIconId enum)

[Evan Stade, 2016-07-08 21:25:26]

lgtm

[tdanderson, 2016-07-08 21:26:19]

The CQ bit was checked by tdanderson@chromium.org

[tdanderson, 2016-07-08 21:26:19]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2129683002/#ps20001
(title: "do not prepend directory")

[commit-bot: I haz the power, 2016-07-08 21:26:46]

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

[commit-bot: I haz the power, 2016-07-08 22:33:22]

Description was changed from

==========
Use a response file for aggregate_vector_icons in GN

Modify the aggregate_vector_icons step in
ui/gfx/BUILD.gn to accept a response file
containing only the desired .icon files to
process rather than have every build configuration
process and include all .icon files. This will
make it possible to separate icons into
subdirectories and conditionally process them
on a per-platform basis.

BUG=535386
TEST=builds with GN and GYP
==========

to

==========
Use a response file for aggregate_vector_icons in GN

Modify the aggregate_vector_icons step in
ui/gfx/BUILD.gn to accept a response file
containing only the desired .icon files to
process rather than have every build configuration
process and include all .icon files. This will
make it possible to separate icons into
subdirectories and conditionally process them
on a per-platform basis.

BUG=535386
TEST=builds with GN and GYP
==========

[commit-bot: I haz the power, 2016-07-08 22:33:23]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-08 22:33:39]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 22:34:39]

Description was changed from

==========
Use a response file for aggregate_vector_icons in GN

Modify the aggregate_vector_icons step in
ui/gfx/BUILD.gn to accept a response file
containing only the desired .icon files to
process rather than have every build configuration
process and include all .icon files. This will
make it possible to separate icons into
subdirectories and conditionally process them
on a per-platform basis.

BUG=535386
TEST=builds with GN and GYP
==========

to

==========
Use a response file for aggregate_vector_icons in GN

Modify the aggregate_vector_icons step in
ui/gfx/BUILD.gn to accept a response file
containing only the desired .icon files to
process rather than have every build configuration
process and include all .icon files. This will
make it possible to separate icons into
subdirectories and conditionally process them
on a per-platform basis.

BUG=535386
TEST=builds with GN and GYP

Committed: https://crrev.com/3664dfc4b6d8c46708c7ad2a9d95b8eb6ac710b1
Cr-Commit-Position: refs/heads/master@{#404512}
==========

[commit-bot: I haz the power, 2016-07-08 22:34:40]

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