Support migrating multi-install Chrome to single-install.

Support migrating multi-install Chrome to single-install.

When run without "--multi-install", an update will properly convert
multi-install Chrome back to single-install.

BUG=452585
R=robertshield@chromium.org

Committed: https://crrev.com/8f1179ecae3696cc287cfc4166b99468525b149b
Cr-Commit-Position: refs/heads/master@{#313916}

[grt (UTC plus 2), 2015-01-23 21:15:36]

Hi Robert. This does _almost_ everything to migrate multi-to-single. How ill
does this make you? It's not yet ready to commit, but I'd appreciate any early
comments you may have. Thanks.

[robertshield, 2015-01-23 22:45:31]

lg, train pulling in, so an initial set of questions :-) doesn't cause
particular illness, but need to think on it some more

https://codereview.chromium.org/869153004/diff/1/chrome/installer/setup/insta...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/869153004/diff/1/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:1096: // multi-install Chrome to
single-install.
any reason not to do this separately first, or does it have to be coupled to the
multi->single migration?

https://codereview.chromium.org/869153004/diff/1/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:1100: // This operation only applies to
single-installs.
nit: less ambiguous: This operation only applies when the this installer run is
updating or migrating to a single install.

Actually, that's too long.. anyway, maybe something to that effect?

https://codereview.chromium.org/869153004/diff/1/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:1357: // Ensure that the Clients key
for the binaries is gone for single installs.
Do we need to double check to make sure this doesn't remove the last Clients key
mid-way triggering Omaha self-destruct?

https://codereview.chromium.org/869153004/diff/1/chrome/installer/util/instal...
File chrome/installer/util/installation_validator.cc (right):

https://codereview.chromium.org/869153004/diff/1/chrome/installer/util/instal...
chrome/installer/util/installation_validator.cc:396: if
(channel.IsAppLauncher()) {
isn't applauncher dead again?

https://codereview.chromium.org/869153004/diff/1/chrome/installer/util/instal...
File chrome/installer/util/installer_state.cc (right):

https://codereview.chromium.org/869153004/diff/1/chrome/installer/util/instal...
chrome/installer/util/installer_state.cc:733: bool modified = false;
why declare this up here and then |= it below wouldn't it be equivalent to keep
it declared on 737?

[grt (UTC plus 2), 2015-01-24 02:25:16]

Thanks. PTAL.

https://codereview.chromium.org/869153004/diff/1/chrome/installer/setup/insta...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/869153004/diff/1/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:1096: // multi-install Chrome to
single-install.
On 2015/01/23 22:45:30, robertshield wrote:
> any reason not to do this separately first, or does it have to be coupled to
the
> multi->single migration?

In the current world, usagestats is read from/written to only the binaries' key
for multi-install Chrome (and removed from Chrome's, if present) and read
from/written to only Chrome's key for single-install Chrome. I think it's
simpler to keep this logic, because we don't need to change the usagestats
handling in Chrome itself: multi-install still looks in the binaries' key,
single-install still looks in Chrome's. Happy to discuss further if you think
there's risk in this.

https://codereview.chromium.org/869153004/diff/1/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:1100: // This operation only applies to
single-installs.
On 2015/01/23 22:45:30, robertshield wrote:
> nit: less ambiguous: This operation only applies when the this installer run
is
> updating or migrating to a single install.
> 
> Actually, that's too long.. anyway, maybe something to that effect?

Reworded all comments here in the hopes that they are more clear. Let me know
what you think.

https://codereview.chromium.org/869153004/diff/1/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:1357: // Ensure that the Clients key
for the binaries is gone for single installs.
On 2015/01/23 22:45:30, robertshield wrote:
> Do we need to double check to make sure this doesn't remove the last Clients
key
> mid-way triggering Omaha self-destruct?

Line 1343 adds the Clients key for Chrome itself, so I don't think that's a
concern.

https://codereview.chromium.org/869153004/diff/1/chrome/installer/util/instal...
File chrome/installer/util/installation_validator.cc (right):

https://codereview.chromium.org/869153004/diff/1/chrome/installer/util/instal...
chrome/installer/util/installation_validator.cc:396: if
(channel.IsAppLauncher()) {
On 2015/01/23 22:45:30, robertshield wrote:
> isn't applauncher dead again?

huangs@ is removing heroic swaths of code in
https://codereview.chromium.org/811283002/. I'll hold off on landing this until
after his so that I pay the price of the merge conflicts. :-)

https://codereview.chromium.org/869153004/diff/1/chrome/installer/util/instal...
File chrome/installer/util/installer_state.cc (right):

https://codereview.chromium.org/869153004/diff/1/chrome/installer/util/instal...
chrome/installer/util/installer_state.cc:733: bool modified = false;
On 2015/01/23 22:45:30, robertshield wrote:
> why declare this up here and then |= it below wouldn't it be equivalent to
keep
> it declared on 737?

Yeah, that's better. This is an artifact of a previous hacking in here.

[grt (UTC plus 2), 2015-01-27 18:47:06]

It turns out the GP stuff I mentioned earlier today is okay. Please take another
look at this when you have a chance. huangs's change is still a prerequisite.

[robertshield, 2015-01-28 23:17:59]

LGTM

https://codereview.chromium.org/869153004/diff/20001/chrome/installer/setup/i...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/869153004/diff/20001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:1110: NOTREACHED();
Is it worth logging this (and maybe other, but mostly this) failure in this
function to the installer log to help with debugging in the case of user error
reports?

https://codereview.chromium.org/869153004/diff/20001/chrome/installer/util/in...
File chrome/installer/util/installation_validator.cc (right):

https://codereview.chromium.org/869153004/diff/20001/chrome/installer/util/in...
chrome/installer/util/installation_validator.cc:416: if (cf_state != NULL &&
chrome_state == NULL &&
can this whole check be removed given that CF has gone to the great git repo in
the sky? same question applies for other CF checks. If this is out of scope for
this CL please ignore this comment.

[grt (UTC plus 2), 2015-01-29 17:56:52]

Thanks!

https://codereview.chromium.org/869153004/diff/20001/chrome/installer/setup/i...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/869153004/diff/20001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:1110: NOTREACHED();
On 2015/01/28 23:17:59, robertshield wrote:
> Is it worth logging this (and maybe other, but mostly this) failure in this
> function to the installer log to help with debugging in the case of user error
> reports?

Excellent idea. Done.

https://codereview.chromium.org/869153004/diff/20001/chrome/installer/util/in...
File chrome/installer/util/installation_validator.cc (right):

https://codereview.chromium.org/869153004/diff/20001/chrome/installer/util/in...
chrome/installer/util/installation_validator.cc:416: if (cf_state != NULL &&
chrome_state == NULL &&
On 2015/01/28 23:17:59, robertshield wrote:
> can this whole check be removed given that CF has gone to the great git repo
in
> the sky? same question applies for other CF checks. If this is out of scope
for
> this CL please ignore this comment.

Yeah, there's more room for cleanup. I'll keep that to a separate CL. Maybe in
the grand "out with multi-install" bloodletting.

[grt (UTC plus 2), 2015-01-29 19:14:57]

Please take another look at chrome/installer/util/installation_validator.cc.
I've changed it a bit since your LGTM as a result of merge conflicts and the
fact that it shouldn't run for Chromium or canary installs. Thanks.

[robertshield, 2015-01-30 14:46:29]

The CQ bit was checked by robertshield@chromium.org

[robertshield, 2015-01-30 14:46:30]

lgtm

lgtm++

[commit-bot: I haz the power, 2015-01-30 14:47:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/869153004/80001

[commit-bot: I haz the power, 2015-01-30 14:49:41]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-30 14:50:30]

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