Force migrate all clients from multi-install back to single-install.

Force migrate all clients from multi-install back to single-install.

This is the first CL to remove multi-install from the codebase. With
this change, "--multi-install" is ignored on the command line and in
master preferences. All installs will henceforth be single-install. All
over-installs and updates will henceforth migrate an existing
multi-install back to single-install.

This CL also contains the following changes to mini_installer to report
state properly regardless of whether updates are being delivered via
Chrome's or the binaries' app guid:
- "-full" is added to both Chrome's "ap" value and, in case an existing
  multi-install Chrome is being updated, the binaries' "ap" value.
- Installer results are written both to Chrome's ClientState key and, as
  above, the binaries'.

Tangentially, this CL also cleans up some minor Chromium vs. Google
Chrome issues with the mini_installer and fixes function ordering
somewhat.

BUG=577816
R=gab@chromium.org

Committed: https://crrev.com/bc318bfb16955ab1c566d6582bab266ffb2e47a4
Cr-Commit-Position: refs/heads/master@{#438644}

[grt (UTC plus 2), 2016-11-18 21:04:38]

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

[commit-bot: I haz the power, 2016-11-18 21:05:40]

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-18 22:49:05]

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

[commit-bot: I haz the power, 2016-11-18 22:49:05]

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), 2016-11-21 11:19:23]

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

[commit-bot: I haz the power, 2016-11-21 11:19:36]

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-21 12:01:49]

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

[commit-bot: I haz the power, 2016-11-21 12:01:50]

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), 2016-11-21 13:12:04]

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

[commit-bot: I haz the power, 2016-11-21 13:12:21]

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-21 13:12:31]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-11-21 13:53:41]

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

[commit-bot: I haz the power, 2016-11-21 13:53:42]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2016-12-12 15:20:38]

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

[commit-bot: I haz the power, 2016-12-12 15:20:51]

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-12-12 15:34:41]

Description was changed from

==========
Force migrate all clients from multi-install back to single-install.

This is the first CL to remove multi-install from the codebase. With
this change, "--multi-install" is ignored on the command line. All
installs will henceforth be single-install. All over-installs and
updates will henceforth migrate an existing multi-install back to
single-install.

BUG=577816
==========

to

==========
Force migrate all clients from multi-install back to single-install.

This is the first CL to remove multi-install from the codebase. With
this change, "--multi-install" is ignored on the command line. All
installs will henceforth be single-install. All over-installs and
updates will henceforth migrate an existing multi-install back to
single-install.

This CL also contains the following changes to mini_installer to report
state properly regardless of whether updates are being delivered via
Chrome's or the binaries' app guid:
- "-full" is added to both Chrome's "ap" value and, in case an existing
  multi-install Chrome is being updated, the binaries' "ap" value.
- Installer results are written both to Chrome's ClientState key and, as
  above, the binaries'.

Tangentially, this CL also cleans up some minor Chromium vs. Google
Chrome issues with the mini_installer and fixes function ordering
somewhat.

BUG=577816
R=gab@chromium.org
==========

[grt (UTC plus 2), 2016-12-12 15:34:41]

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

[grt (UTC plus 2), 2016-12-12 15:34:48]

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

[commit-bot: I haz the power, 2016-12-12 15:35:04]

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-12-12 15:35:15]

Description was changed from

==========
Force migrate all clients from multi-install back to single-install.

This is the first CL to remove multi-install from the codebase. With
this change, "--multi-install" is ignored on the command line. All
installs will henceforth be single-install. All over-installs and
updates will henceforth migrate an existing multi-install back to
single-install.

This CL also contains the following changes to mini_installer to report
state properly regardless of whether updates are being delivered via
Chrome's or the binaries' app guid:
- "-full" is added to both Chrome's "ap" value and, in case an existing
  multi-install Chrome is being updated, the binaries' "ap" value.
- Installer results are written both to Chrome's ClientState key and, as
  above, the binaries'.

Tangentially, this CL also cleans up some minor Chromium vs. Google
Chrome issues with the mini_installer and fixes function ordering
somewhat.

BUG=577816
R=gab@chromium.org
==========

to

==========
Force migrate all clients from multi-install back to single-install.

This is the first CL to remove multi-install from the codebase. With
this change, "--multi-install" is ignored on the command line and in
master preferences. All installs will henceforth be single-install. All
over-installs and updates will henceforth migrate an existing
multi-install back to single-install.

This CL also contains the following changes to mini_installer to report
state properly regardless of whether updates are being delivered via
Chrome's or the binaries' app guid:
- "-full" is added to both Chrome's "ap" value and, in case an existing
  multi-install Chrome is being updated, the binaries' "ap" value.
- Installer results are written both to Chrome's ClientState key and, as
  above, the binaries'.

Tangentially, this CL also cleans up some minor Chromium vs. Google
Chrome issues with the mini_installer and fixes function ordering
somewhat.

BUG=577816
R=gab@chromium.org
==========

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

Patchset #3 (id:60001) has been deleted

[grt (UTC plus 2), 2016-12-12 15:36:47]

Here's a fun one! PTAL. Feel free to loop in Robert if you think another pair of
eyes would be good. Thanks.

[commit-bot: I haz the power, 2016-12-12 16:32:53]

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

[commit-bot: I haz the power, 2016-12-12 16:32:54]

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

[gab, 2016-12-12 21:38:16]

Phew, that was quite a review (awesome btw!), lgtm overall but unless my
questions below spring up anything you hadn't thought of I can't say I have the
knowledge to be 100% confident about this change, though I am confident in your
knowledge and thoroughness and 100% excited about this happen =D!

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
File chrome/installer/mini_installer/chrome_appid.cc (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/chrome_appid.cc:15: const wchar_t kAppGuid[] =
L"";
Chromium has no appguid? Also, if we follow models in the rest of the installer,
this would typically be in a separate file and each file included in the right
build?

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
File chrome/installer/mini_installer/configuration.cc (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/configuration.cc:90: chrome_app_guid_ =
google_update::kSxSAppGuid;
Set this in above block on line 79

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/configuration.cc:143: bool
Configuration::IsUpdatingMultiChrome() const {
Writing my assumptions here: 

1) We update from multi:
 * --multi-install is present on uninstall cmd-line of Chrome product.
 * This returns true

2) We update from chrome but still need to cleanup old multi (we can't cleanup
right away as that would make the product disappear during update and GUpdate
wouldn't appreciate?)
 * --multi-install was stripped in (1)
 * This returns false

Correct? Can you point me at where the cleanup phase occurs?

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
File chrome/installer/mini_installer/configuration.h (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/configuration.h:24: ~Configuration();
Presence of virtual methods (even if protected and only for testing) dictates
that destructor should be virtual.

(or you could override the registry instead of mocking via virtual method)

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/configuration.h:51: // Retuns true if
--chrome-sxs is on the command line (Google Chrome only).
Why make this Google Chrome only? Chromium SxS exists, no?

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
File chrome/installer/mini_installer/configuration_test.cc (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/configuration_test.cc:42: using RegistryLocation
= Configuration::RegistryLocation;
Shouldn't this also be inherited?

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/configuration_test.cc:43: TestConfiguration() {}
= default;

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/configuration_test.cc:44: explicit
TestConfiguration(const wchar_t* command_line) : Configuration() {
Isn't ": Configuration()" implicit anyways?

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
File chrome/installer/mini_installer/mini_installer.cc (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/mini_installer.cc:108: // existing multi-install
Chrome is being updated.
Is this because the cleanup is two phase? i.e. first update (from multi) =
migration, second update (from chrome) = cleanup multi?

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
File chrome/installer/mini_installer/mini_installer_constants.cc (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/mini_installer_constants.cc:71: // No trailing
slash on this one because the app's GUID is not appended.
This \\ difference is kind of weird (I'd instead expect whoever appends to add
the \\ before doing so), but that was already the case for kClientStateKeyBase
so I guess it's fine.

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/setup/...
File chrome/installer/setup/installer_state.cc (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/setup/...
chrome/installer/setup/installer_state.cc:635: } else if
(is_migrating_to_single()) {
Wait... my assumptions are breaking down... so we can still be in
multi_install()? I thought we could now only be in is_migrating_to_single()
phase (once, when we were --multi-install and got an update).

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/setup/...
File chrome/installer/setup/installer_state_unittest.cc (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/setup/...
chrome/installer/setup/installer_state_unittest.cc:126: // check results for a
fresh install of single Chrome and the same for an
s/check/Check

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/setup/...
chrome/installer/setup/installer_state_unittest.cc:128: static constexpr const
wchar_t* kCommandLines[] = {
const wchar_t* const

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/setup/...
chrome/installer/setup/installer_state_unittest.cc:243:
EXPECT_EQ(InstallerState::SINGLE_INSTALL_OR_UPDATE,
Assuming we will get rid of the "Single" terminology in an eventual follow-up?

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/util/m...
File chrome/installer/util/master_preferences.cc (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/util/m...
chrome/installer/util/master_preferences.cc:167: '.' +
master_preferences::kMultiInstall,
Why bother stripping it? We're not stripping kChrome nor any other garbage. What
I'd like one day is to only copy a select set to Preferences instead of stomping
it all out there but that's another story.

Also, have we updated online documentation already?

[grt (UTC plus 2), 2016-12-13 09:16:27]

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

[commit-bot: I haz the power, 2016-12-13 09:16: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), 2016-12-13 09:20:38]

Thanks for the great review. PTAL.

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
File chrome/installer/mini_installer/chrome_appid.cc (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/chrome_appid.cc:15: const wchar_t kAppGuid[] =
L"";
On 2016/12/12 21:38:16, gab wrote:
> Chromium has no appguid?

No. Chromium doesn't integrate with Google Update, hence no appguid. Chromium
puts all "Clients" and "ClientState" values into the key "...\SOFTWARE\Chromium"
for the browser and "...\SOFTWARE\Chromium Binaries" for the binaries.

> Also, if we follow models in the rest of the installer,
> this would typically be in a separate file and each file included in the right
> build?

That's a world I'm moving to in https://crrev.com/432270 and others to come. I
hope that one day that stuff will be available in the mini_installer. For now,
though, we'll stick with mini_installer being a bit of a hack on its own. :-)

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
File chrome/installer/mini_installer/configuration.cc (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/configuration.cc:90: chrome_app_guid_ =
google_update::kSxSAppGuid;
On 2016/12/12 21:38:16, gab wrote:
> Set this in above block on line 79

Done.

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/configuration.cc:143: bool
Configuration::IsUpdatingMultiChrome() const {
On 2016/12/12 21:38:16, gab wrote:
> Writing my assumptions here: 
> 
> 1) We update from multi:
>  * --multi-install is present on uninstall cmd-line of Chrome product.
>  * This returns true
> 
> 2) We update from chrome but still need to cleanup old multi (we can't cleanup
> right away as that would make the product disappear during update and GUpdate
> wouldn't appreciate?)
>  * --multi-install was stripped in (1)
>  * This returns false
> 
> Correct? Can you point me at where the cleanup phase occurs?

This CL ensures that "--multi-install" on the command line or in
master_preferences is ignored, so it's no longer possible to initiate a multi
install or update. This means that UninstallArguments never contains
"--multi-install" when written in AddUninstallShortcutWorkItems.

And here's the piece that unconditionally deletes the Clients key for the
binaries when doing any single install or update:
https://cs.chromium.org/chromium/src/chrome/installer/setup/install_worker.cc...

So, while there may be multi-install Chrome already present on the machine, it
will be single-install by the time setup.exe exits (unless it exits due to an
error).

As far as the multi-to-single migration being delivered by Google Update via
Chrome's vs. the Binaries' app guids, both are supported. If the update is
delivered via the Binaries app guid, Google Update will think that the product
uninstalled itself during the update. This is the primary way that I monitor the
progress of the migration. :-) The relevant teams know not to scramble when the
particular error code for this spikes.

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
File chrome/installer/mini_installer/configuration.h (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/configuration.h:24: ~Configuration();
On 2016/12/12 21:38:16, gab wrote:
> Presence of virtual methods (even if protected and only for testing) dictates
> that destructor should be virtual.
> 
> (or you could override the registry instead of mocking via virtual method)

Switched to overriding the registry.

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/configuration.h:51: // Retuns true if
--chrome-sxs is on the command line (Google Chrome only).
On 2016/12/12 21:38:16, gab wrote:
> Why make this Google Chrome only? Chromium SxS exists, no?

Nope:
https://cs.chromium.org/chromium/src/chrome/installer/util/browser_distributi...

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
File chrome/installer/mini_installer/configuration_test.cc (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/configuration_test.cc:42: using RegistryLocation
= Configuration::RegistryLocation;
On 2016/12/12 21:38:16, gab wrote:
> Shouldn't this also be inherited?

This was making the type public for consumers of this class. It's gone now.

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/configuration_test.cc:43: TestConfiguration() {}
On 2016/12/12 21:38:16, gab wrote:
> = default;

Removed

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/configuration_test.cc:44: explicit
TestConfiguration(const wchar_t* command_line) : Configuration() {
On 2016/12/12 21:38:16, gab wrote:
> Isn't ": Configuration()" implicit anyways?

Yes. The original author must have liked being explicit. I've removed it since I
don't think it's general practice.

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
File chrome/installer/mini_installer/mini_installer.cc (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/mini_installer.cc:108: // existing multi-install
Chrome is being updated.
On 2016/12/12 21:38:16, gab wrote:
> Is this because the cleanup is two phase? i.e. first update (from multi) =
> migration, second update (from chrome) = cleanup multi?

No, it's single phase. This is so that the results (especially errors) are
written to both Chrome's and the Binaries' appguid so that Google Update will
see them regardless of which one it's paying attention to. In the case of
success, the value written to the Binaries' key will be ignored by Google Update
since it will simply report that the product disappeared during update (see
other comment).

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
File chrome/installer/mini_installer/mini_installer_constants.cc (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/mini_installer_constants.cc:71: // No trailing
slash on this one because the app's GUID is not appended.
On 2016/12/12 21:38:16, gab wrote:
> This \\ difference is kind of weird (I'd instead expect whoever appends to add
> the \\ before doing so), but that was already the case for kClientStateKeyBase
> so I guess it's fine.

Acknowledged.

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/setup/...
File chrome/installer/setup/installer_state.cc (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/setup/...
chrome/installer/setup/installer_state.cc:635: } else if
(is_migrating_to_single()) {
On 2016/12/12 21:38:16, gab wrote:
> Wait... my assumptions are breaking down... so we can still be in
> multi_install()? I thought we could now only be in is_migrating_to_single()
> phase (once, when we were --multi-install and got an update).

No, we can't be in multi_install anymore -- the block above will never be hit. I
am doing the code removal separately so that this CL is reasonable to review.

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/setup/...
File chrome/installer/setup/installer_state_unittest.cc (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/setup/...
chrome/installer/setup/installer_state_unittest.cc:126: // check results for a
fresh install of single Chrome and the same for an
On 2016/12/12 21:38:16, gab wrote:
> s/check/Check

Done.

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/setup/...
chrome/installer/setup/installer_state_unittest.cc:128: static constexpr const
wchar_t* kCommandLines[] = {
On 2016/12/12 21:38:16, gab wrote:
> const wchar_t* const

not needed. "constexpr const wchar_t*" reads like this: "kCommandLines is a
compile-time constant array containing wchar_t pointers to constant memory.

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/setup/...
chrome/installer/setup/installer_state_unittest.cc:243:
EXPECT_EQ(InstallerState::SINGLE_INSTALL_OR_UPDATE,
On 2016/12/12 21:38:16, gab wrote:
> Assuming we will get rid of the "Single" terminology in an eventual follow-up?

Yes, all cleanups will be in separate CLs.

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/util/m...
File chrome/installer/util/master_preferences.cc (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/util/m...
chrome/installer/util/master_preferences.cc:167: '.' +
master_preferences::kMultiInstall,
On 2016/12/12 21:38:16, gab wrote:
> Why bother stripping it?

This is an insurance policy against code that looks directly into the
process-wide MasterPreferences dict to check for multi-ness rather than querying
the InstallerState instance. I haven't found any in my various searches, but
that doesn't mean it's not there. :-)

Note that this does not edit the file on disk -- it just serves to keep the
state in-process consistent.

> We're not stripping kChrome nor any other garbage. What
> I'd like one day is to only copy a select set to Preferences instead of
stomping
> it all out there but that's another story.
> 
> Also, have we updated online documentation already?

I plan to make a pass on chromium.org sometime after this lands.

[commit-bot: I haz the power, 2016-12-13 09:25:43]

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

[commit-bot: I haz the power, 2016-12-13 09:25:44]

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-12-13 12:54:19]

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

[commit-bot: I haz the power, 2016-12-13 12:54:36]

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-12-13 14:33:06]

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

[commit-bot: I haz the power, 2016-12-13 14:33:07]

Dry run: This issue passed the CQ dry run.

[gab, 2016-12-14 16:33:38]

lgtm w/ nit and thoughts

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
File chrome/installer/mini_installer/configuration.h (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/configuration.h:51: // Retuns true if
--chrome-sxs is on the command line (Google Chrome only).
On 2016/12/13 09:20:37, grt (UTC plus 1) wrote:
> On 2016/12/12 21:38:16, gab wrote:
> > Why make this Google Chrome only? Chromium SxS exists, no?
> 
> Nope:
>
https://cs.chromium.org/chromium/src/chrome/installer/util/browser_distributi...

I see, thought it did because we have test_installer configs for SxS but just
realized they're all behind $SUPPORTS_SXS (which I can't figure out where we
ever set to True..?!).

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
File chrome/installer/mini_installer/mini_installer.cc (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/mini_installer.cc:108: // existing multi-install
Chrome is being updated.
On 2016/12/13 09:20:37, grt (UTC plus 1) wrote:
> On 2016/12/12 21:38:16, gab wrote:
> > Is this because the cleanup is two phase? i.e. first update (from multi) =
> > migration, second update (from chrome) = cleanup multi?
> 
> No, it's single phase. This is so that the results (especially errors) are
> written to both Chrome's and the Binaries' appguid so that Google Update will
> see them regardless of which one it's paying attention to. In the case of
> success, the value written to the Binaries' key will be ignored by Google
Update
> since it will simply report that the product disappeared during update (see
> other comment).

Aaaah, I see, that's what I hadn't realized : on error GU reports error on the
one it's watching, on success it reports success if updating product and
uninstall if updating binaries, yay!

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/util/m...
File chrome/installer/util/master_preferences.cc (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/util/m...
chrome/installer/util/master_preferences.cc:167: '.' +
master_preferences::kMultiInstall,
On 2016/12/13 09:20:37, grt (UTC plus 1) wrote:
> On 2016/12/12 21:38:16, gab wrote:
> > Why bother stripping it?
> 
> This is an insurance policy against code that looks directly into the
> process-wide MasterPreferences dict to check for multi-ness rather than
querying
> the InstallerState instance. I haven't found any in my various searches, but
> that doesn't mean it's not there. :-)

Got it, can you clarify the comment to that effect (i.e. to avoid this turning
into a justification to add stripping code for all-the-things).

> 
> Note that this does not edit the file on disk -- it just serves to keep the
> state in-process consistent.
> 
> > We're not stripping kChrome nor any other garbage. What
> > I'd like one day is to only copy a select set to Preferences instead of
> stomping
> > it all out there but that's another story.
> > 
> > Also, have we updated online documentation already?
> 
> I plan to make a pass on http://chromium.org sometime after this lands.

https://codereview.chromium.org/2507293005/diff/120001/chrome/installer/mini_...
File chrome/installer/mini_installer/configuration.cc (right):

https://codereview.chromium.org/2507293005/diff/120001/chrome/installer/mini_...
chrome/installer/mini_installer/configuration.cc:141: value.length() != 0 &&
Curious, why does it matter to check for pv here? Can the uninstall cmd-line
ever be present without a pv?

https://codereview.chromium.org/2507293005/diff/120001/chrome/installer/mini_...
File chrome/installer/mini_installer/configuration.h (right):

https://codereview.chromium.org/2507293005/diff/120001/chrome/installer/mini_...
chrome/installer/mini_installer/configuration.h:82: Configuration&
operator=(const Configuration&) = delete;
nit: the move constructor is implicitly deleted I think when there is an
out-of-line destructor

(and DISALLOW_COPY_AND_ASSIGN -- which is what would be used here if base/ was
allowed -- doesn't bother with disallowing move)

https://codereview.chromium.org/2507293005/diff/120001/chrome/installer/mini_...
File chrome/installer/mini_installer/configuration_test.cc (right):

https://codereview.chromium.org/2507293005/diff/120001/chrome/installer/mini_...
chrome/installer/mini_installer/configuration_test.cc:45: void Initialize(const
wchar_t* command_line) {
inline in constructor?

https://codereview.chromium.org/2507293005/diff/120001/chrome/installer/mini_...
chrome/installer/mini_installer/configuration_test.cc:46: Clear();
Probably don't need to Clear() a brand new Configuration?

[grt (UTC plus 2), 2016-12-14 20:38:45]

Thanks! Sending it up to the CQ. Say goodbye to multi-install!

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
File chrome/installer/mini_installer/configuration.h (right):

https://codereview.chromium.org/2507293005/diff/80001/chrome/installer/mini_i...
chrome/installer/mini_installer/configuration.h:51: // Retuns true if
--chrome-sxs is on the command line (Google Chrome only).
On 2016/12/14 16:33:38, gab wrote:
> On 2016/12/13 09:20:37, grt (UTC plus 1) wrote:
> > On 2016/12/12 21:38:16, gab wrote:
> > > Why make this Google Chrome only? Chromium SxS exists, no?
> > 
> > Nope:
> >
>
https://cs.chromium.org/chromium/src/chrome/installer/util/browser_distributi...
> 
> I see, thought it did because we have test_installer configs for SxS but just
> realized they're all behind $SUPPORTS_SXS (which I can't figure out where we
> ever set to True..?!).

It's magic!

(well, not really, but it isn't obvious when searching for it:
https://cs.chromium.org/chromium/src/chrome/test/mini_installer/variable_expa...
)

https://codereview.chromium.org/2507293005/diff/120001/chrome/installer/mini_...
File chrome/installer/mini_installer/configuration.cc (right):

https://codereview.chromium.org/2507293005/diff/120001/chrome/installer/mini_...
chrome/installer/mini_installer/configuration.cc:141: value.length() != 0 &&
On 2016/12/14 16:33:38, gab wrote:
> Curious, why does it matter to check for pv here?

"pv" is the critical registry value that, from Google Update's point of view,
means the product is installed. If "pv" does not exist, Chrome is not installed,
so the UninstallArguments should not be trusted. Google Update deletes an app's
ClientState key at some undetermined point after it notices that the app has
been uninstalled, so it's entirely possible that there could be stale data in
ClientState from a previous installation of Chrome.

> Can the uninstall cmd-line
> ever be present without a pv?

https://codereview.chromium.org/2507293005/diff/120001/chrome/installer/mini_...
File chrome/installer/mini_installer/configuration.h (right):

https://codereview.chromium.org/2507293005/diff/120001/chrome/installer/mini_...
chrome/installer/mini_installer/configuration.h:82: Configuration&
operator=(const Configuration&) = delete;
On 2016/12/14 16:33:38, gab wrote:
> nit: the move constructor is implicitly deleted I think when there is an
> out-of-line destructor

cool! the same seems to be true for the move assignment operator (C++11
12.8.20).

https://codereview.chromium.org/2507293005/diff/120001/chrome/installer/mini_...
File chrome/installer/mini_installer/configuration_test.cc (right):

https://codereview.chromium.org/2507293005/diff/120001/chrome/installer/mini_...
chrome/installer/mini_installer/configuration_test.cc:45: void Initialize(const
wchar_t* command_line) {
On 2016/12/14 16:33:38, gab wrote:
> inline in constructor?

Done.

https://codereview.chromium.org/2507293005/diff/120001/chrome/installer/mini_...
chrome/installer/mini_installer/configuration_test.cc:46: Clear();
On 2016/12/14 16:33:38, gab wrote:
> Probably don't need to Clear() a brand new Configuration?

Done.

[grt (UTC plus 2), 2016-12-14 20:38:54]

The CQ bit was checked by grt@chromium.org

[grt (UTC plus 2), 2016-12-14 20:38:55]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2507293005/#ps140001
(title: "gab comments")

[commit-bot: I haz the power, 2016-12-14 20:39:26]

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-12-14 22:00:29]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481747934766720,
"parent_rev": "ac943d09605aa2d94a6ab77ce1b249a732b01e10", "commit_rev":
"83c10938a784288ed8b8d616b5e9b673c70d952d"}

[commit-bot: I haz the power, 2016-12-14 22:01:04]

Description was changed from

==========
Force migrate all clients from multi-install back to single-install.

This is the first CL to remove multi-install from the codebase. With
this change, "--multi-install" is ignored on the command line and in
master preferences. All installs will henceforth be single-install. All
over-installs and updates will henceforth migrate an existing
multi-install back to single-install.

This CL also contains the following changes to mini_installer to report
state properly regardless of whether updates are being delivered via
Chrome's or the binaries' app guid:
- "-full" is added to both Chrome's "ap" value and, in case an existing
  multi-install Chrome is being updated, the binaries' "ap" value.
- Installer results are written both to Chrome's ClientState key and, as
  above, the binaries'.

Tangentially, this CL also cleans up some minor Chromium vs. Google
Chrome issues with the mini_installer and fixes function ordering
somewhat.

BUG=577816
R=gab@chromium.org
==========

to

==========
Force migrate all clients from multi-install back to single-install.

This is the first CL to remove multi-install from the codebase. With
this change, "--multi-install" is ignored on the command line and in
master preferences. All installs will henceforth be single-install. All
over-installs and updates will henceforth migrate an existing
multi-install back to single-install.

This CL also contains the following changes to mini_installer to report
state properly regardless of whether updates are being delivered via
Chrome's or the binaries' app guid:
- "-full" is added to both Chrome's "ap" value and, in case an existing
  multi-install Chrome is being updated, the binaries' "ap" value.
- Installer results are written both to Chrome's ClientState key and, as
  above, the binaries'.

Tangentially, this CL also cleans up some minor Chromium vs. Google
Chrome issues with the mini_installer and fixes function ordering
somewhat.

BUG=577816
R=gab@chromium.org

Review-Url: https://codereview.chromium.org/2507293005
==========

[commit-bot: I haz the power, 2016-12-14 22:01:05]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-12-14 22:04:48]

Description was changed from

==========
Force migrate all clients from multi-install back to single-install.

This is the first CL to remove multi-install from the codebase. With
this change, "--multi-install" is ignored on the command line and in
master preferences. All installs will henceforth be single-install. All
over-installs and updates will henceforth migrate an existing
multi-install back to single-install.

This CL also contains the following changes to mini_installer to report
state properly regardless of whether updates are being delivered via
Chrome's or the binaries' app guid:
- "-full" is added to both Chrome's "ap" value and, in case an existing
  multi-install Chrome is being updated, the binaries' "ap" value.
- Installer results are written both to Chrome's ClientState key and, as
  above, the binaries'.

Tangentially, this CL also cleans up some minor Chromium vs. Google
Chrome issues with the mini_installer and fixes function ordering
somewhat.

BUG=577816
R=gab@chromium.org

Review-Url: https://codereview.chromium.org/2507293005
==========

to

==========
Force migrate all clients from multi-install back to single-install.

This is the first CL to remove multi-install from the codebase. With
this change, "--multi-install" is ignored on the command line and in
master preferences. All installs will henceforth be single-install. All
over-installs and updates will henceforth migrate an existing
multi-install back to single-install.

This CL also contains the following changes to mini_installer to report
state properly regardless of whether updates are being delivered via
Chrome's or the binaries' app guid:
- "-full" is added to both Chrome's "ap" value and, in case an existing
  multi-install Chrome is being updated, the binaries' "ap" value.
- Installer results are written both to Chrome's ClientState key and, as
  above, the binaries'.

Tangentially, this CL also cleans up some minor Chromium vs. Google
Chrome issues with the mini_installer and fixes function ordering
somewhat.

BUG=577816
R=gab@chromium.org

Committed: https://crrev.com/bc318bfb16955ab1c566d6582bab266ffb2e47a4
Cr-Commit-Position: refs/heads/master@{#438644}
==========

[commit-bot: I haz the power, 2016-12-14 22:04:49]

Patchset 6 (id:??) landed as
https://crrev.com/bc318bfb16955ab1c566d6582bab266ffb2e47a4
Cr-Commit-Position: refs/heads/master@{#438644}