Add a Canary-specific icon in VisualElementsManifest.xml

Add a Canary-specific icon in VisualElementsManifest.xml

This will fix the tile in Win10 being always the full-colour version,
even for Canary.

Removes the now unused SecondaryTile.png. (I think it was for Metro mode in
Win8? I'm not sure... but it was already unused as of this CL.)

R=grt@chromium.org
BUG=637477
Committed: https://crrev.com/8c120019e3da7d32e3f9deb23cc91a7db92ac541
Cr-Commit-Position: refs/heads/master@{#414529}

[scottmg, 2016-08-17 23:01:55]

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

[scottmg, 2016-08-17 23:02:06]

Description was changed from

==========
Add a Canary-specific icon in VisualElementsManifest.xml.

This will fix the tile in Win10 being always the full-colour version,
even for Canary.

R=grt@chromium.org
BUG=637477
==========

to

==========
Add a Canary-specific icon in VisualElementsManifest.xml

This will fix the tile in Win10 being always the full-colour version,
even for Canary.

R=grt@chromium.org
BUG=637477
==========

[commit-bot: I haz the power, 2016-08-17 23:02: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, 2016-08-18 00:18:56]

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

[commit-bot: I haz the power, 2016-08-18 00:18:57]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2016-08-18 08:24:50]

Meh. I just noticed that Logo and SmallLogo aren't in
chrome/app/theme/google_chrome (with chromium variants in theme/chromium). This
means that chromium builds have the Google-branded icons. This is not so good.
The canary icons should only be included in Google-branded builds (SxS isn't
supported for Chromium), and the code to write the proper string should have an
#if defined(GOOGLE_CHROME_BUILD) around it.

[grt (UTC plus 2), 2016-08-22 13:06:08]

On 2016/08/18 08:24:50, grt (UTC plus 2) wrote:
> Meh. I just noticed that Logo and SmallLogo aren't in
> chrome/app/theme/google_chrome (with chromium variants in theme/chromium).
This
> means that chromium builds have the Google-branded icons. This is not so good.
> The canary icons should only be included in Google-branded builds (SxS isn't
> supported for Chromium), and the code to write the proper string should have
an
> #if defined(GOOGLE_CHROME_BUILD) around it.

To add insult to injury, I just noticed that our build config always specifies
--distribution=_google_chrome even for chromium builds. Double ugh. Do you have
the time/energy to fix this (https://crbug.com/180489 is related, but clearly
Robert didn't realize that the GOOGLE_CHROME section is being used for Chromium
builds, too)?

[scottmg, 2016-08-24 20:35:41]

Just testing locally, but I think (? maybe?) this is all that's required to get
things split properly by Chromium/Chrome, and get the Canary icon in.

https://codereview.chromium.org/2253323002/diff/20001/chrome/BUILD.gn
File chrome/BUILD.gn (right):

https://codereview.chromium.org/2253323002/diff/20001/chrome/BUILD.gn#newcode...
chrome/BUILD.gn:1271:
"//chrome/app/theme/$branding_path_component/win/tiles/Logo.png",
There's a separate internal CL that will have to land and roll before this one
that adds these in the Chrome location.

https://codereview.chromium.org/2253323002/diff/20001/chrome/installer/setup/...
File chrome/installer/setup/install.cc (right):

https://codereview.chromium.org/2253323002/diff/20001/chrome/installer/setup/...
chrome/installer/setup/install.cc:411: InstallUtil::IsChromeSxSProcess() ?
L"Canary" : L"";
Did you want GOOGLE_CHROME_BRANDED around this? It sort of looks like
IsChromeSxSProcess() should be false for Chromium, but maybe I'm misreading it.

[scottmg, 2016-08-24 20:36:41]

On 2016/08/22 13:06:08, grt (UTC plus 2) wrote:
> On 2016/08/18 08:24:50, grt (UTC plus 2) wrote:
> > Meh. I just noticed that Logo and SmallLogo aren't in
> > chrome/app/theme/google_chrome (with chromium variants in theme/chromium).
> This
> > means that chromium builds have the Google-branded icons. This is not so
good.
> > The canary icons should only be included in Google-branded builds (SxS isn't
> > supported for Chromium), and the code to write the proper string should have
> an
> > #if defined(GOOGLE_CHROME_BUILD) around it.
> 
> To add insult to injury, I just noticed that our build config always specifies
> --distribution=_google_chrome even for chromium builds. Double ugh. Do you
have
> the time/energy to fix this (https://crbug.com/180489 is related, but clearly
> Robert didn't realize that the GOOGLE_CHROME section is being used for
Chromium
> builds, too)?

Sorry, I'm not entirely sure what this part means so I didn't do anything about
this part (looks like that crbug is a typo).

[grt (UTC plus 2), 2016-08-25 10:22:56]

please add to the CL description that this also removes the vestigial secondary
tile image.

lgtm with some motion in chrome.release.

https://codereview.chromium.org/2253323002/diff/20001/chrome/installer/mini_i...
File chrome/installer/mini_installer/chrome.release (right):

https://codereview.chromium.org/2253323002/diff/20001/chrome/installer/mini_i...
chrome/installer/mini_installer/chrome.release:70: # VisualElements sub-dir.
move this up above line 49 so that it's out of the [GOOGLE_CHROME] block.

https://codereview.chromium.org/2253323002/diff/20001/chrome/installer/setup/...
File chrome/installer/setup/install.cc (right):

https://codereview.chromium.org/2253323002/diff/20001/chrome/installer/setup/...
chrome/installer/setup/install.cc:411: InstallUtil::IsChromeSxSProcess() ?
L"Canary" : L"";
On 2016/08/24 20:35:41, scottmg wrote:
> Did you want GOOGLE_CHROME_BRANDED around this? It sort of looks like
> IsChromeSxSProcess() should be false for Chromium, but maybe I'm misreading
it.

Naah. I think it'd be better for CheckIsChromeSxSProcess to just return false on
Chromium builds. That's for another CL, though.

[grt (UTC plus 2), 2016-08-25 10:36:20]

On 2016/08/24 20:36:41, scottmg wrote:
> On 2016/08/22 13:06:08, grt (UTC plus 2) wrote:
> > On 2016/08/18 08:24:50, grt (UTC plus 2) wrote:
> > > Meh. I just noticed that Logo and SmallLogo aren't in
> > > chrome/app/theme/google_chrome (with chromium variants in theme/chromium).
> > This
> > > means that chromium builds have the Google-branded icons. This is not so
> good.
> > > The canary icons should only be included in Google-branded builds (SxS
isn't
> > > supported for Chromium), and the code to write the proper string should
have
> > an
> > > #if defined(GOOGLE_CHROME_BUILD) around it.
> > 
> > To add insult to injury, I just noticed that our build config always
specifies
> > --distribution=_google_chrome even for chromium builds. Double ugh. Do you
> have
> > the time/energy to fix this (https://crbug.com/180489 is related, but
clearly
> > Robert didn't realize that the GOOGLE_CHROME section is being used for
> Chromium
> > builds, too)?
> 
> Sorry, I'm not entirely sure what this part means so I didn't do anything
about
> this part (looks like that crbug is a typo).

Oops, https://crbug.com/180589. Here's what I mean:

https://cs.chromium.org/chromium/src/chrome/installer/mini_installer/BUILD.gn...

Notice that distribution is hardcoded to google_chrome for all builds. As far as
I can tell it's been that way for a very long time, but it shouldn't be.

[scottmg, 2016-08-25 16:18:48]

Description was changed from

==========
Add a Canary-specific icon in VisualElementsManifest.xml

This will fix the tile in Win10 being always the full-colour version,
even for Canary.

R=grt@chromium.org
BUG=637477
==========

to

==========
Add a Canary-specific icon in VisualElementsManifest.xml

This will fix the tile in Win10 being always the full-colour version,
even for Canary.

Removes the now unused SecondaryTile.png. (I think it was for Metro mode in
Win8? I'm not sure... but it was already unused as of this CL.)

R=grt@chromium.org
BUG=637477
==========

[scottmg, 2016-08-25 16:30:50]

On 2016/08/25 10:36:20, grt (UTC plus 2) wrote:
> On 2016/08/24 20:36:41, scottmg wrote:
> > On 2016/08/22 13:06:08, grt (UTC plus 2) wrote:
> > > On 2016/08/18 08:24:50, grt (UTC plus 2) wrote:
> > > > Meh. I just noticed that Logo and SmallLogo aren't in
> > > > chrome/app/theme/google_chrome (with chromium variants in
theme/chromium).
> > > This
> > > > means that chromium builds have the Google-branded icons. This is not so
> > good.
> > > > The canary icons should only be included in Google-branded builds (SxS
> isn't
> > > > supported for Chromium), and the code to write the proper string should
> have
> > > an
> > > > #if defined(GOOGLE_CHROME_BUILD) around it.
> > > 
> > > To add insult to injury, I just noticed that our build config always
> specifies
> > > --distribution=_google_chrome even for chromium builds. Double ugh. Do you
> > have
> > > the time/energy to fix this (https://crbug.com/180489 is related, but
> clearly
> > > Robert didn't realize that the GOOGLE_CHROME section is being used for
> > Chromium
> > > builds, too)?
> > 
> > Sorry, I'm not entirely sure what this part means so I didn't do anything
> about
> > this part (looks like that crbug is a typo).
> 
> Oops, https://crbug.com/180589. Here's what I mean:
> 
>
https://cs.chromium.org/chromium/src/chrome/installer/mini_installer/BUILD.gn...
> 
> Notice that distribution is hardcoded to google_chrome for all builds. As far
as
> I can tell it's been that way for a very long time, but it shouldn't be.

Ah, OK. That seems a little more scary to change, definitely not going to try
that here. Maybe we can just avoid create_installer_archive needing to know that
entirely?

[scottmg, 2016-08-25 16:33:11]

https://codereview.chromium.org/2253323002/diff/20001/chrome/installer/mini_i...
File chrome/installer/mini_installer/chrome.release (right):

https://codereview.chromium.org/2253323002/diff/20001/chrome/installer/mini_i...
chrome/installer/mini_installer/chrome.release:70: # VisualElements sub-dir.
On 2016/08/25 10:22:56, grt (UTC plus 2) wrote:
> move this up above line 49 so that it's out of the [GOOGLE_CHROME] block.

Ah, I see. Done.

[scottmg, 2016-08-25 17:10:13]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2016-08-25 17:10:13]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/2253323002/#ps40001
(title: "chrome.release fixes")

[commit-bot: I haz the power, 2016-08-25 17:11:06]

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-08-25 17:15:06]

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

[commit-bot: I haz the power, 2016-08-25 17:15:07]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  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_...)

[scottmg, 2016-08-25 17:50:11]

Patchset #4 (id:60001) has been deleted

[scottmg, 2016-08-25 17:52:11]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2016-08-25 17:52:11]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/2253323002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-25 17:52:49]

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-08-25 20:41:12]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-08-25 20:43:24]

Description was changed from

==========
Add a Canary-specific icon in VisualElementsManifest.xml

This will fix the tile in Win10 being always the full-colour version,
even for Canary.

Removes the now unused SecondaryTile.png. (I think it was for Metro mode in
Win8? I'm not sure... but it was already unused as of this CL.)

R=grt@chromium.org
BUG=637477
==========

to

==========
Add a Canary-specific icon in VisualElementsManifest.xml

This will fix the tile in Win10 being always the full-colour version,
even for Canary.

Removes the now unused SecondaryTile.png. (I think it was for Metro mode in
Win8? I'm not sure... but it was already unused as of this CL.)

R=grt@chromium.org
BUG=637477

Committed: https://crrev.com/8c120019e3da7d32e3f9deb23cc91a7db92ac541
Cr-Commit-Position: refs/heads/master@{#414529}
==========

[commit-bot: I haz the power, 2016-08-25 20:43:26]

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