Move brand- and mode-specific app icon resource id into InstallConstants.

Move brand- and mode-specific app icon resource id into InstallConstants.

This will ease handling of icons for dev and beta.

BUG=373987
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Review-Url: https://codereview.chromium.org/2689233005
Cr-Commit-Position: refs/heads/master@{#453106}
Committed: https://chromium.googlesource.com/chromium/src/+/4d6747c1f649591794536e52993de4b88a274ba8

[grt (UTC plus 2), 2017-02-15 11:53:42]

Description was changed from

==========
Move brand- and mode-specific app icon resource id into InstallConstants.

BUG=373987
==========

to

==========
Move brand- and mode-specific app icon resource id into InstallConstants.

BUG=373987
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[grt (UTC plus 2), 2017-02-15 11:54:35]

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

[commit-bot: I haz the power, 2017-02-15 11:54:52]

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

[grt (UTC plus 2), 2017-02-15 12:00:07]

Description was changed from

==========
Move brand- and mode-specific app icon resource id into InstallConstants.

BUG=373987
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Move brand- and mode-specific app icon resource id into InstallConstants.

This will ease handling of icons for dev and beta.

BUG=373987
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[grt (UTC plus 2), 2017-02-15 12:00:07]

grt@chromium.org changed reviewers:
+ gab@chromium.org

[grt (UTC plus 2), 2017-02-15 12:00:21]

PTAL

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

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

[commit-bot: I haz the power, 2017-02-15 13:57:15]

Dry run: This issue passed the CQ dry run.

[gab, 2017-02-15 14:30:53]

Surprised this doesn't also affect
BrowserDistribution::GetIcon(Index|Filename)()? Or is this just about the
resource?

https://codereview.chromium.org/2689233005/diff/1/chrome/install_static/BUILD.gn
File chrome/install_static/BUILD.gn (right):

https://codereview.chromium.org/2689233005/diff/1/chrome/install_static/BUILD...
chrome/install_static/BUILD.gn:22: "../app/chrome_dll_resource.h",
This is weird? i.e. as a DEPS it makes sense but as a source?

[grt (UTC plus 2), 2017-02-15 15:11:42]

The BrowserDistribution changes will happen as well, they just aren't needed for
this particular item.

https://codereview.chromium.org/2689233005/diff/1/chrome/install_static/BUILD.gn
File chrome/install_static/BUILD.gn (right):

https://codereview.chromium.org/2689233005/diff/1/chrome/install_static/BUILD...
chrome/install_static/BUILD.gn:22: "../app/chrome_dll_resource.h",
On 2017/02/15 14:30:53, gab wrote:
> This is weird? i.e. as a DEPS it makes sense but as a source?

Maybe this isn't the right thing/needed. I'll check with a build/OWNER.

[grt (UTC plus 2), 2017-02-15 15:13:20]

grt@chromium.org changed reviewers:
+ scottmg@chromium.org

[grt (UTC plus 2), 2017-02-15 15:13:21]

+scottmg: would you take a look at the BUILD.gn change? is it necessary to list
the .h as a source given that there's currently no appropriate minimal target on
which to depend?

[scottmg, 2017-02-15 17:51:29]

On 2017/02/15 15:13:21, grt (UTC plus 1) wrote:
> +scottmg: would you take a look at the BUILD.gn change? is it necessary to
list
> the .h as a source given that there's currently no appropriate minimal target
on
> which to depend?

I guess it's a bit weird because there is chrome/app:chrome_dll_resources, which
is almost what you want. The problem with that is that you don't want to
actually have the .res here, just the #define, is that right?

I think here, it's OK to do what you've done because install_static and app are
already closely related.

(There's no technical reason that I know of that it's a problem, it's only an
aesthetic thing either way.)

[grt (UTC plus 2), 2017-02-15 17:57:55]

On 2017/02/15 17:51:29, scottmg wrote:
> On 2017/02/15 15:13:21, grt (UTC plus 1) wrote:
> > +scottmg: would you take a look at the BUILD.gn change? is it necessary to
> list
> > the .h as a source given that there's currently no appropriate minimal
target
> on
> > which to depend?
> 
> I guess it's a bit weird because there is chrome/app:chrome_dll_resources,
which
> is almost what you want. The problem with that is that you don't want to
> actually have the .res here, just the #define, is that right?

Exactly.

> I think here, it's OK to do what you've done because install_static and app
are
> already closely related.
> 
> (There's no technical reason that I know of that it's a problem, it's only an
> aesthetic thing either way.)

Sounds good. Thanks for taking a look. May I have your stamp of approval?

[scottmg, 2017-02-15 18:03:04]

lgtm

[gab, 2017-02-15 18:36:00]

On 2017/02/15 15:11:42, grt (UTC plus 1) wrote:
> The BrowserDistribution changes will happen as well, they just aren't needed
for
> this particular item.

ok lgtm then given scottmg's review on the GN bits

> 
>
https://codereview.chromium.org/2689233005/diff/1/chrome/install_static/BUILD.gn
> File chrome/install_static/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2689233005/diff/1/chrome/install_static/BUILD...
> chrome/install_static/BUILD.gn:22: "../app/chrome_dll_resource.h",
> On 2017/02/15 14:30:53, gab wrote:
> > This is weird? i.e. as a DEPS it makes sense but as a source?
> 
> Maybe this isn't the right thing/needed. I'll check with a build/OWNER.

[grt (UTC plus 2), 2017-02-25 20:40:36]

The CQ bit was checked by grt@chromium.org

[grt (UTC plus 2), 2017-02-25 20:40:37]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/2689233005/#ps20001
(title: "sync to position 453103")

[commit-bot: I haz the power, 2017-02-25 20:40: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-02-25 22:16:33]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488055236977580,
"parent_rev": "2625b25c0aca5e17160f27380009ea88f32e8f1c", "commit_rev":
"4d6747c1f649591794536e52993de4b88a274ba8"}

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

Description was changed from

==========
Move brand- and mode-specific app icon resource id into InstallConstants.

This will ease handling of icons for dev and beta.

BUG=373987
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Move brand- and mode-specific app icon resource id into InstallConstants.

This will ease handling of icons for dev and beta.

BUG=373987
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Review-Url: https://codereview.chromium.org/2689233005
Cr-Commit-Position: refs/heads/master@{#453106}
Committed:
https://chromium.googlesource.com/chromium/src/+/4d6747c1f649591794536e52993d...
==========

[commit-bot: I haz the power, 2017-02-25 22:17:18]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/4d6747c1f649591794536e52993d...