Use InstallDetails in setup.

Use InstallDetails in setup.

This change brings InstallDetails to the installer with the install mode
chosen on the basis of a command line switch. This makes InstallDetails
and various install_static APIs usable from within the installer.

A few calls to InstallUtil::IsChromeSxSProcess are also replaced with
values in kBrandConstants or other more correct/clear values.

BUG=373987
Review-Url: https://codereview.chromium.org/2459583002
Cr-Commit-Position: refs/heads/master@{#451769}
Committed: https://chromium.googlesource.com/chromium/src/+/0f3aff7ddf7a686e87edff89a93a386f028be02b

[grt (UTC plus 2), 2016-10-31 13:48:26]

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

[commit-bot: I haz the power, 2016-10-31 13:48:35]

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), 2016-10-31 13:59:58]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-10-31 14:19:36]

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

[commit-bot: I haz the power, 2016-10-31 14:19:37]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[grt (UTC plus 2), 2016-11-01 10:20:23]

Patchset #1 (id:20001) has been deleted

[grt (UTC plus 2), 2016-11-01 10:20:29]

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

[commit-bot: I haz the power, 2016-11-01 10:20: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, 2016-11-01 11:34:28]

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

[commit-bot: I haz the power, 2016-11-01 11:34:29]

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

[grt (UTC plus 2), 2016-11-01 12:53:54]

Patchset #1 (id:40001) has been deleted

[grt (UTC plus 2), 2016-11-02 10:16:26]

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

[grt (UTC plus 2), 2016-11-02 10:16:27]

Hi Robert. Here's the first change bringing InstallDetails into setup. Following
this, I'll be able to kill things like the channel determination in
installer/util so that the one source of truth is the value computed in
install_static for both chrome (in chrome_elf and shared throughout the process)
and setup. Let me know if you would rather that I find another reviewer. Thanks.

[grt (UTC plus 2), 2016-11-02 10:24:50]

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

[commit-bot: I haz the power, 2016-11-02 10:25:00]

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-11-02 11:31:35]

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

[commit-bot: I haz the power, 2016-11-02 11:31:35]

Dry run: This issue passed the CQ dry run.

[robertshield, 2016-11-04 12:50:35]

lg, few thoughts

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
File chrome/installer/setup/brand_constants.h (right):

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/brand_constants.h:5: // Defines the struct used to
describe installer specific corresponding to each
Maybe: 

"Defines the struct describing installer details for each of a brand's install
modes; ..."

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/brand_constants.h:20: // The command-line switch used to
select this install mode.
Worth specifying who uses the switch?

"The command-line switch originally passed to the installer to select this
install mode?"

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/brand_constants.h:24: // Start Menu.
s/logos/start menu entries?

Maybe call it start_menu_suffix too? Unless it's also used elsewhere?

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/brand_constants.h:27: // True if this mode supported the
(now removed) app host and launcher.
if this mode ever supported?

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/install_worker.cc:1181: if
(GetBrandConstants().supported_app_launcher)
I don't have a suggestion, more of an observation: it's a little confusing from
a reader's perspective that some supported_XXX values are on the brand constants
and some are in the install details. 

Would it be possible to unify these somehow in a future CL?

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
File chrome/installer/setup/install_worker_unittest.cc (right):

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/install_worker_unittest.cc:482: // TODO(robertshield):
Set up some real expectations.
I don't know what this comment means anymore :-(

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
File chrome/installer/setup/setup_install_details.cc (right):

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/setup_install_details.cc:54: // The value is sussed out
in MasterPreferences initialization.
s/The value/In all three cases the value/

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/setup_install_details.cc:63: // The value is sussed out
in MasterPreferences initialization.
In both cases

[grt (UTC plus 2), 2016-11-08 13:02:13]

Thanks for the comments. Responses below.

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
File chrome/installer/setup/brand_constants.h (right):

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/brand_constants.h:5: // Defines the struct used to
describe installer specific corresponding to each
On 2016/11/04 12:50:35, robertshield_slow_reviews wrote:
> Maybe: 
> 
> "Defines the struct describing installer details for each of a brand's install
> modes; ..."

Done.

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/brand_constants.h:20: // The command-line switch used to
select this install mode.
On 2016/11/04 12:50:35, robertshield_slow_reviews wrote:
> Worth specifying who uses the switch?
> 
> "The command-line switch originally passed to the installer to select this
> install mode?"

Done.

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/brand_constants.h:24: // Start Menu.
On 2016/11/04 12:50:35, robertshield_slow_reviews wrote:
> s/logos/start menu entries?
> 
> Maybe call it start_menu_suffix too? Unless it's also used elsewhere?

It really is the suffix of the artwork files referenced in
chrome.VisualElementsManifest.xml, which is generated at install-time, and not
the entries in the start menu. I'll update the comment a bit.

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/brand_constants.h:27: // True if this mode supported the
(now removed) app host and launcher.
On 2016/11/04 12:50:35, robertshield_slow_reviews wrote:
> if this mode ever supported?

Done.

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/install_worker.cc:1181: if
(GetBrandConstants().supported_app_launcher)
On 2016/11/04 12:50:35, robertshield_slow_reviews wrote:
> I don't have a suggestion, more of an observation: it's a little confusing
from
> a reader's perspective that some supported_XXX values are on the brand
constants
> and some are in the install details. 
> 
> Would it be possible to unify these somehow in a future CL?

I wouldn't object to resolving it now somehow. Here's my dilemma:

There are three categories (maybe more) of constants that are brand and mode
specific:
- Those used in chrome_elf for the bare minimum needed to bootstrap crashpad.
- Those used in the product for all sorts of things (e.g., string identifiers).
- Those used in the installer.

Part of me wants these to be separate buckets so that the union of the three
isn't present in each binary. The downside to this is that there's this somewhat
odd partitioning with parallel structures in the three domains. Another part of
me wants them to be unified in one structure so that there's one and only one
home for the sauce that differs between brands/modes.

Do you have any thoughts? Is the confusion around multiple locations big enough
to justify lumping them all together?

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
File chrome/installer/setup/install_worker_unittest.cc (right):

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/install_worker_unittest.cc:482: // TODO(robertshield):
Set up some real expectations.
On 2016/11/04 12:50:35, robertshield_slow_reviews wrote:
> I don't know what this comment means anymore :-(

Currently, this test is a bit bogus. It basically tests that a few functions are
called some number of times in any order. It doesn't really assert that
AddInstallWorkItems does anything in particular. I debated removing it
altogether since test_installer.py is our one hope for ensuring that the
installer does the right thing. I decided that doing that removal now was out of
scope for this CL. Do you have any thoughts about either leaving this test here,
removing it, or making it more of a "real" test?

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
File chrome/installer/setup/setup_install_details.cc (right):

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/setup_install_details.cc:54: // The value is sussed out
in MasterPreferences initialization.
On 2016/11/04 12:50:35, robertshield_slow_reviews wrote:
> s/The value/In all three cases the value/

Done.

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/setup_install_details.cc:63: // The value is sussed out
in MasterPreferences initialization.
On 2016/11/04 12:50:35, robertshield_slow_reviews wrote:
> In both cases

Done.

[robertshield, 2016-11-08 22:03:40]

lgtm

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/install_worker.cc:1181: if
(GetBrandConstants().supported_app_launcher)
On 2016/11/08 13:02:13, grt (UTC plus 1) wrote:
> On 2016/11/04 12:50:35, robertshield_slow_reviews wrote:
> > I don't have a suggestion, more of an observation: it's a little confusing
> from
> > a reader's perspective that some supported_XXX values are on the brand
> constants
> > and some are in the install details. 
> > 
> > Would it be possible to unify these somehow in a future CL?
> 
> I wouldn't object to resolving it now somehow. Here's my dilemma:
> 
> There are three categories (maybe more) of constants that are brand and mode
> specific:
> - Those used in chrome_elf for the bare minimum needed to bootstrap crashpad.
> - Those used in the product for all sorts of things (e.g., string
identifiers).
> - Those used in the installer.
> 
> Part of me wants these to be separate buckets so that the union of the three
> isn't present in each binary. The downside to this is that there's this
somewhat
> odd partitioning with parallel structures in the three domains. Another part
of
> me wants them to be unified in one structure so that there's one and only one
> home for the sauce that differs between brands/modes.
> 
> Do you have any thoughts? Is the confusion around multiple locations big
enough
> to justify lumping them all together?

Thought about this for a bit and I'm not sure if the benefits of excluding the
unneeded state from the parts that don't need it really outweigh the cognitive
load needed to understand that there are three places where state is saved and
various combinations of those three are needed in different places. 

Doesn't need to be addressed here but on the whole I'd aim for unifying them in
the near future.

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
File chrome/installer/setup/install_worker_unittest.cc (right):

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/install_worker_unittest.cc:482: // TODO(robertshield):
Set up some real expectations.
On 2016/11/08 13:02:13, grt (UTC plus 1) wrote:
> On 2016/11/04 12:50:35, robertshield_slow_reviews wrote:
> > I don't know what this comment means anymore :-(
> 
> Currently, this test is a bit bogus. It basically tests that a few functions
are
> called some number of times in any order. It doesn't really assert that
> AddInstallWorkItems does anything in particular. I debated removing it
> altogether since test_installer.py is our one hope for ensuring that the
> installer does the right thing. I decided that doing that removal now was out
of
> scope for this CL. Do you have any thoughts about either leaving this test
here,
> removing it, or making it more of a "real" test?

Not really, I agree that is basically an extremely weak version of
test_installer.py that really only tests "does something rather than checking
that anything specific happens. 

I'd be fairly happy to kill it, though agree that is out of scope for this cl.

[grt (UTC plus 2), 2016-11-10 15:10:50]

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

[commit-bot: I haz the power, 2016-11-10 15:11:01]

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), 2016-11-10 15:18:03]

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/2459583002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/install_worker.cc:1181: if
(GetBrandConstants().supported_app_launcher)
On 2016/11/08 22:03:40, robertshield_slow_reviews wrote:
> On 2016/11/08 13:02:13, grt (UTC plus 1) wrote:
> > On 2016/11/04 12:50:35, robertshield_slow_reviews wrote:
> > > I don't have a suggestion, more of an observation: it's a little confusing
> > from
> > > a reader's perspective that some supported_XXX values are on the brand
> > constants
> > > and some are in the install details. 
> > > 
> > > Would it be possible to unify these somehow in a future CL?
> > 
> > I wouldn't object to resolving it now somehow. Here's my dilemma:
> > 
> > There are three categories (maybe more) of constants that are brand and mode
> > specific:
> > - Those used in chrome_elf for the bare minimum needed to bootstrap
crashpad.
> > - Those used in the product for all sorts of things (e.g., string
> identifiers).
> > - Those used in the installer.
> > 
> > Part of me wants these to be separate buckets so that the union of the three
> > isn't present in each binary. The downside to this is that there's this
> somewhat
> > odd partitioning with parallel structures in the three domains. Another part
> of
> > me wants them to be unified in one structure so that there's one and only
one
> > home for the sauce that differs between brands/modes.
> > 
> > Do you have any thoughts? Is the confusion around multiple locations big
> enough
> > to justify lumping them all together?
> 
> Thought about this for a bit and I'm not sure if the benefits of excluding the
> unneeded state from the parts that don't need it really outweigh the cognitive
> load needed to understand that there are three places where state is saved and
> various combinations of those three are needed in different places. 
> 
> Doesn't need to be addressed here but on the whole I'd aim for unifying them
in
> the near future.

Now is as good a time as any. WDYT?

[commit-bot: I haz the power, 2016-11-10 16:32:48]

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

[commit-bot: I haz the power, 2016-11-10 16:32:49]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2016-11-15 12:25:23]

Description was changed from

==========
Use InstallDetails in setup.

This change brings InstallDetails to the installer with the install mode
chosen on the basis of a command line switch. This makes InstallDetails
and various install_static APIs usable from within the installer.

This change also introduces kBrandConstants, an array of BrandConstants
structs corresponding to each of a brand's install modes (see
install_static/install_modes.h for information about these terms).

Finally, a few calls to InstallUtil::IsChromeSxSProcess are replaced
with values in kBrandConstants.

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

to

==========
Use InstallDetails in setup.

This change brings InstallDetails to the installer with the install mode
chosen on the basis of a command line switch. This makes InstallDetails
and various install_static APIs usable from within the installer.

Additionally, a few calls to InstallUtil::IsChromeSxSProcess are replaced
with values in kInstallModes.

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

[grt (UTC plus 2), 2017-01-03 14:19:30]

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

[grt (UTC plus 2), 2017-01-03 14:19:40]

Patchset #4 (id:120001) has been deleted

[commit-bot: I haz the power, 2017-01-03 14:19:41]

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

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

[commit-bot: I haz the power, 2017-01-03 15:40:03]

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

[grt (UTC plus 2), 2017-01-03 18:02:19]

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

[commit-bot: I haz the power, 2017-01-03 18:02:35]

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-03 19:00:26]

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

[commit-bot: I haz the power, 2017-01-03 19:00:27]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2017-01-06 16:02:34]

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

[commit-bot: I haz the power, 2017-01-06 16:03:05]

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-06 18:24:24]

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

[commit-bot: I haz the power, 2017-01-06 18:24:25]

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

[grt (UTC plus 2), 2017-02-10 14:50:50]

Description was changed from

==========
Use InstallDetails in setup.

This change brings InstallDetails to the installer with the install mode
chosen on the basis of a command line switch. This makes InstallDetails
and various install_static APIs usable from within the installer.

Additionally, a few calls to InstallUtil::IsChromeSxSProcess are replaced
with values in kInstallModes.

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

to

==========
Use InstallDetails in setup.

This change brings InstallDetails to the installer with the install mode
chosen on the basis of a command line switch. This makes InstallDetails
and various install_static APIs usable from within the installer.

A few calls to InstallUtil::IsChromeSxSProcess are also replaced with
values in kBrandConstants or other more correct/clear values.

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

[grt (UTC plus 2), 2017-02-10 14:53:07]

Patchset #8 (id:220001) has been deleted

[grt (UTC plus 2), 2017-02-10 14:53:20]

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

[commit-bot: I haz the power, 2017-02-10 14:53:46]

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-10 14:56:47]

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

[grt (UTC plus 2), 2017-02-10 14:57:07]

Patchset #8 (id:240001) has been deleted

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

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-10 14:57:27]

The CQ bit was unchecked by grt@chromium.org

[grt (UTC plus 2), 2017-02-10 14:57:51]

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

[commit-bot: I haz the power, 2017-02-10 14:58:16]

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-10 14:58:41]

Apologies for letting this sit for so long. I needed to get the multi-install
removal in before moving forward to this. That's out of the way now, so I'm now
unblocked to continue with this project. Would you be so kind as to take another
look?

[commit-bot: I haz the power, 2017-02-10 16:06:37]

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

[commit-bot: I haz the power, 2017-02-10 16:06:38]

Dry run: This issue passed the CQ dry run.

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

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

[commit-bot: I haz the power, 2017-02-14 11:50:58]

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-14 12:27:20]

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

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

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

[grt (UTC plus 2), 2017-02-14 12:39:37]

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

[commit-bot: I haz the power, 2017-02-14 12:39: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-02-14 13:33:13]

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

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

Dry run: This issue passed the CQ dry run.

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

gentle ping

[robertshield, 2017-02-20 19:11:16]

Looks awesome, apologies for the delay last week. Few thoughts below.

https://codereview.chromium.org/2459583002/diff/300001/chrome/install_static/...
File chrome/install_static/google_chrome_install_modes.cc (right):

https://codereview.chromium.org/2459583002/diff/300001/chrome/install_static/...
chrome/install_static/google_chrome_install_modes.cc:39: {
Add a comment describing this is the install mode for canary to match with line
26.

https://codereview.chromium.org/2459583002/diff/300001/chrome/install_static/...
File chrome/install_static/install_details.h (right):

https://codereview.chromium.org/2459583002/diff/300001/chrome/install_static/...
chrome/install_static/install_details.h:100: // True if the mode once supported
multi-install.
Maybe expand this to 

"True if the mode once supported multi-install, a legacy way of doing installs.
This still exists only to provide migration and cleanup for older installs."

https://codereview.chromium.org/2459583002/diff/300001/chrome/install_static/...
File chrome/install_static/install_modes.h (right):

https://codereview.chromium.org/2459583002/diff/300001/chrome/install_static/...
chrome/install_static/install_modes.h:19: // registry paths.
Can you go into a little more detail here (or elsewhere if there's somewhere
more appropriate) about:

* whether it is valid for a brand to have installed only a secondary mode or
only multiple secondary modes?
* any other practical differences in the way a primary mode installation is
treated vs secondary modes.

https://codereview.chromium.org/2459583002/diff/300001/chrome/installer/setup...
File chrome/installer/setup/install_unittest.cc (right):

https://codereview.chromium.org/2459583002/diff/300001/chrome/installer/setup...
chrome/installer/setup/install_unittest.cc:43:
std::tuple<install_static::InstallConstantIndex, const char*>> {
It would be cool if the parameters were described in more detail here at the
point of the test fixture declaration to avoid having to hunt for the test case
instantiations below.

https://codereview.chromium.org/2459583002/diff/300001/chrome/installer/setup...
File chrome/installer/setup/setup_install_details_unittest.cc (right):

https://codereview.chromium.org/2459583002/diff/300001/chrome/installer/setup...
chrome/installer/setup/setup_install_details_unittest.cc:36: #if
defined(GOOGLE_CHROME_BUILD)
Most of the interesting test cases here are under this define, so
double-checking: will this regularly run on waterfall / try bots that define
GOOGLE_CHROME_BUILD?

https://codereview.chromium.org/2459583002/diff/300001/chrome/installer/setup...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/2459583002/diff/300001/chrome/installer/setup...
chrome/installer/setup/setup_main.cc:1397: return
installer::SXS_OPTION_NOT_SUPPORTED;
Question about returning SXS_OPTION_NOT_SUPPORTED here because the check doesn't
also look for !install_details.is_primary_mode(). 

Should this whole block (1393-1406) be wrapped in an

if (!install_details.is_primary_mode()) {...}

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

Thanks. Responses below.

https://codereview.chromium.org/2459583002/diff/300001/chrome/install_static/...
File chrome/install_static/google_chrome_install_modes.cc (right):

https://codereview.chromium.org/2459583002/diff/300001/chrome/install_static/...
chrome/install_static/google_chrome_install_modes.cc:39: {
On 2017/02/20 19:11:16, robertshield wrote:
> Add a comment describing this is the install mode for canary to match with
line
> 26.

Done.

https://codereview.chromium.org/2459583002/diff/300001/chrome/install_static/...
File chrome/install_static/install_details.h (right):

https://codereview.chromium.org/2459583002/diff/300001/chrome/install_static/...
chrome/install_static/install_details.h:100: // True if the mode once supported
multi-install.
On 2017/02/20 19:11:16, robertshield wrote:
> Maybe expand this to 
> 
> "True if the mode once supported multi-install, a legacy way of doing
installs.
> This still exists only to provide migration and cleanup for older installs."

Done.

https://codereview.chromium.org/2459583002/diff/300001/chrome/install_static/...
File chrome/install_static/install_modes.h (right):

https://codereview.chromium.org/2459583002/diff/300001/chrome/install_static/...
chrome/install_static/install_modes.h:19: // registry paths.
On 2017/02/20 19:11:16, robertshield wrote:
> Can you go into a little more detail here (or elsewhere if there's somewhere
> more appropriate) about:
> 
> * whether it is valid for a brand to have installed only a secondary mode or
> only multiple secondary modes?
> * any other practical differences in the way a primary mode installation is
> treated vs secondary modes. 

Added some text. Let me know if you had something different in mind.

https://codereview.chromium.org/2459583002/diff/300001/chrome/installer/setup...
File chrome/installer/setup/install_unittest.cc (right):

https://codereview.chromium.org/2459583002/diff/300001/chrome/installer/setup...
chrome/installer/setup/install_unittest.cc:43:
std::tuple<install_static::InstallConstantIndex, const char*>> {
On 2017/02/20 19:11:16, robertshield wrote:
> It would be cool if the parameters were described in more detail here at the
> point of the test fixture declaration to avoid having to hunt for the test
case
> instantiations below. 

Done.

https://codereview.chromium.org/2459583002/diff/300001/chrome/installer/setup...
File chrome/installer/setup/setup_install_details_unittest.cc (right):

https://codereview.chromium.org/2459583002/diff/300001/chrome/installer/setup...
chrome/installer/setup/setup_install_details_unittest.cc:36: #if
defined(GOOGLE_CHROME_BUILD)
On 2017/02/20 19:11:16, robertshield wrote:
> Most of the interesting test cases here are under this define, so
> double-checking: will this regularly run on waterfall / try bots that define
> GOOGLE_CHROME_BUILD?

I've sent a query to MB owners about this. I don't see coverage on default
trybots or the CQ. It would be nice to fix this.

https://codereview.chromium.org/2459583002/diff/300001/chrome/installer/setup...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/2459583002/diff/300001/chrome/installer/setup...
chrome/installer/setup/setup_main.cc:1397: return
installer::SXS_OPTION_NOT_SUPPORTED;
On 2017/02/20 19:11:16, robertshield wrote:
> Question about returning SXS_OPTION_NOT_SUPPORTED here because the check
doesn't
> also look for !install_details.is_primary_mode(). 
> 
> Should this whole block (1393-1406) be wrapped in an
> 
> if (!install_details.is_primary_mode()) {...}

No. Blocking system-level installs is specific to SxS/canary, not general for
all secondary modes. The future dev and beta side-by-side install modes will
support system-level installation.

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

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

[commit-bot: I haz the power, 2017-02-21 13:38:17]

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

[robertshield, 2017-02-21 14:28:20]

LGTM, agree it would be nice to get GOOGLE_CHROME_BUILD tests running on the
waterfall.

https://codereview.chromium.org/2459583002/diff/300001/chrome/install_static/...
File chrome/install_static/install_modes.h (right):

https://codereview.chromium.org/2459583002/diff/300001/chrome/install_static/...
chrome/install_static/install_modes.h:19: // registry paths.
On 2017/02/21 13:21:15, grt (UTC plus 1) wrote:
> On 2017/02/20 19:11:16, robertshield wrote:
> > Can you go into a little more detail here (or elsewhere if there's somewhere
> > more appropriate) about:
> > 
> > * whether it is valid for a brand to have installed only a secondary mode or
> > only multiple secondary modes?
> > * any other practical differences in the way a primary mode installation is
> > treated vs secondary modes. 
> 
> Added some text. Let me know if you had something different in mind.

Looks great, thanks!

https://codereview.chromium.org/2459583002/diff/300001/chrome/installer/setup...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/2459583002/diff/300001/chrome/installer/setup...
chrome/installer/setup/setup_main.cc:1397: return
installer::SXS_OPTION_NOT_SUPPORTED;
On 2017/02/21 13:21:16, grt (UTC plus 1) wrote:
> On 2017/02/20 19:11:16, robertshield wrote:
> > Question about returning SXS_OPTION_NOT_SUPPORTED here because the check
> doesn't
> > also look for !install_details.is_primary_mode(). 
> > 
> > Should this whole block (1393-1406) be wrapped in an
> > 
> > if (!install_details.is_primary_mode()) {...}
> 
> No. Blocking system-level installs is specific to SxS/canary, not general for
> all secondary modes. The future dev and beta side-by-side install modes will
> support system-level installation.

Ok, sg. Consider adding a comment explaining the above here.

[robertshield, 2017-02-21 14:28:20]

LGTM, agree it would be nice to get GOOGLE_CHROME_BUILD tests running on the
waterfall.

https://codereview.chromium.org/2459583002/diff/300001/chrome/install_static/...
File chrome/install_static/install_modes.h (right):

https://codereview.chromium.org/2459583002/diff/300001/chrome/install_static/...
chrome/install_static/install_modes.h:19: // registry paths.
On 2017/02/21 13:21:15, grt (UTC plus 1) wrote:
> On 2017/02/20 19:11:16, robertshield wrote:
> > Can you go into a little more detail here (or elsewhere if there's somewhere
> > more appropriate) about:
> > 
> > * whether it is valid for a brand to have installed only a secondary mode or
> > only multiple secondary modes?
> > * any other practical differences in the way a primary mode installation is
> > treated vs secondary modes. 
> 
> Added some text. Let me know if you had something different in mind.

Looks great, thanks!

https://codereview.chromium.org/2459583002/diff/300001/chrome/installer/setup...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/2459583002/diff/300001/chrome/installer/setup...
chrome/installer/setup/setup_main.cc:1397: return
installer::SXS_OPTION_NOT_SUPPORTED;
On 2017/02/21 13:21:16, grt (UTC plus 1) wrote:
> On 2017/02/20 19:11:16, robertshield wrote:
> > Question about returning SXS_OPTION_NOT_SUPPORTED here because the check
> doesn't
> > also look for !install_details.is_primary_mode(). 
> > 
> > Should this whole block (1393-1406) be wrapped in an
> > 
> > if (!install_details.is_primary_mode()) {...}
> 
> No. Blocking system-level installs is specific to SxS/canary, not general for
> all secondary modes. The future dev and beta side-by-side install modes will
> support system-level installation.

Ok, sg. Consider adding a comment explaining the above here.

[grt (UTC plus 2), 2017-02-21 14:38:41]

The CQ bit was checked by grt@chromium.org

[grt (UTC plus 2), 2017-02-21 14:38:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertshield@chromium.org
Link to the patchset: https://codereview.chromium.org/2459583002/#ps360001
(title: "another doc comment")

[commit-bot: I haz the power, 2017-02-21 14:39:04]

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-21 14:39:09]

Thanks. Bombs away!

https://codereview.chromium.org/2459583002/diff/300001/chrome/installer/setup...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/2459583002/diff/300001/chrome/installer/setup...
chrome/installer/setup/setup_main.cc:1397: return
installer::SXS_OPTION_NOT_SUPPORTED;
On 2017/02/21 14:28:20, robertshield wrote:
> On 2017/02/21 13:21:16, grt (UTC plus 1) wrote:
> > On 2017/02/20 19:11:16, robertshield wrote:
> > > Question about returning SXS_OPTION_NOT_SUPPORTED here because the check
> > doesn't
> > > also look for !install_details.is_primary_mode(). 
> > > 
> > > Should this whole block (1393-1406) be wrapped in an
> > > 
> > > if (!install_details.is_primary_mode()) {...}
> > 
> > No. Blocking system-level installs is specific to SxS/canary, not general
for
> > all secondary modes. The future dev and beta side-by-side install modes will
> > support system-level installation.
> 
> Ok, sg. Consider adding a comment explaining the above here.

Done.

[commit-bot: I haz the power, 2017-02-21 15:52:32]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1487687921541900,
"parent_rev": "d0bce4262751f95ad57a02065b15ba4c561922b6", "commit_rev":
"0f3aff7ddf7a686e87edff89a93a386f028be02b"}

[commit-bot: I haz the power, 2017-02-21 15:53:24]

Description was changed from

==========
Use InstallDetails in setup.

This change brings InstallDetails to the installer with the install mode
chosen on the basis of a command line switch. This makes InstallDetails
and various install_static APIs usable from within the installer.

A few calls to InstallUtil::IsChromeSxSProcess are also replaced with
values in kBrandConstants or other more correct/clear values.

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

to

==========
Use InstallDetails in setup.

This change brings InstallDetails to the installer with the install mode
chosen on the basis of a command line switch. This makes InstallDetails
and various install_static APIs usable from within the installer.

A few calls to InstallUtil::IsChromeSxSProcess are also replaced with
values in kBrandConstants or other more correct/clear values.

BUG=373987

Review-Url: https://codereview.chromium.org/2459583002
Cr-Commit-Position: refs/heads/master@{#451769}
Committed:
https://chromium.googlesource.com/chromium/src/+/0f3aff7ddf7a686e87edff89a93a...
==========

[commit-bot: I haz the power, 2017-02-21 15:53:28]

Committed patchset #13 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/0f3aff7ddf7a686e87edff89a93a...

[grt (UTC plus 2), 2017-03-01 13:02:39]

On 2017/02/21 14:28:20, robertshield wrote:
> LGTM, agree it would be nice to get GOOGLE_CHROME_BUILD tests running on the
> waterfall.

FYI: install_static_unittests, installer_util_unittests, and setup_unittests are
now all running on the continuous Google Chrome builders, so we at least have
coverage of this Chrome-branded code. It's not quite as good as having it in the
trybots and CQ, but it's better than nothing.