Fix -full fallback for diff updates (M56).

Fix -full fallback for diff updates (M56).

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

This is a partial merge of r438644.

BUG=686645
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2663003003
Cr-Commit-Position: refs/branch-heads/2924@{#889}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}
Committed: https://chromium.googlesource.com/chromium/src/+/7f55b083a84bb3fc58852c6d767f1ccdedcadee3

[grt (UTC plus 2), 2017-01-31 12:52:22]

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

[commit-bot: I haz the power, 2017-01-31 12:52:26]

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-31 12:52:27]

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

[commit-bot: I haz the power, 2017-01-31 12:52:28]

Dry run: CLs for remote refs other than refs/heads/master must contain
NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them

[grt (UTC plus 2), 2017-01-31 12:55:54]

Description was changed from

==========
Fix -full fallback for diff updates (M56).

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

This is a partial merge of r438644.

BUG=686645
==========

to

==========
Fix -full fallback for diff updates (M56).

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

This is a partial merge of r438644.

BUG=686645
NOTRY=true
NOPRESUBMIT=true
==========

[grt (UTC plus 2), 2017-01-31 12:56:01]

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

[commit-bot: I haz the power, 2017-01-31 12:56:06]

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-31 12:56:09]

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

[commit-bot: I haz the power, 2017-01-31 12:56:10]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2017-01-31 13:06:26]

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

[grt (UTC plus 2), 2017-01-31 13:06:26]

Hi Sam. Would you review this manual merge for me? It more-or-less brings the
mini_installer changes from https://codereview.chromium.org/2507293005/ into M56
to resolve a bug where "-full" isn't getting appended to the proper "ap" value.
It just so happens that I fixed the bug without knowing about it in
https://codereview.chromium.org/2507293005/ (M57) when I changed the strategy
for writing results into the registry. I would have asked Gab to do this review
(he reviewed the M57 CL), but he's away for a few days and I'd like to get this
ready sooner rather than later. Let me know if you'd like me to find someone
else for it. Thanks.

[huangs, 2017-01-31 17:40:04]

LGTM.  Oops I didn't see the "manual merge" part in the comment and did a full
review.  Please feel free to disregard comments or add TODO, etc.

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
File chrome/installer/mini_installer/configuration.cc (right):

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/configuration.cc:134: return
(OpenClientsKey(root, google_update::kAppGuid, KEY_QUERY_VALUE,
I'd prefer using |chrome_app_guid_| instead of google_update::kAppGuid, and do
the same below. Maybe add

DCHECK_EQ(google_update::kAppGuid, chrome_app_guid_);

at 131?

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
File chrome/installer/mini_installer/configuration_test.cc (right):

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/configuration_test.cc:13: #include
"base/test/test_reg_util_win.h"
#include "base/macros.h" for DISALLOW_COPY_AND_ASSIGN ?

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/configuration_test.cc:71: #endif
#endif  // defined(GOOGLE_CHROME_BUILD)

or

#endif  // GOOGLE_CHROME_BUILD

and below if you'd prefer this style?

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/configuration_test.cc:72: static constexpr const
wchar_t* kUninstallArguments[] = {
Would it be clearer to render |kUninstallArguments| from params?  I.e.:

  std::wstring uninstall_args = std::wstring(L"--uninstall") +
      (system_level ? L" --system-level" : L"") +
      (multi_install ? L" --multi-install --chrome" : L"");

...
  
  uninstall_args.c_str();

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/configuration_test.cc:78: ((system_level ? 0x02
: 0) | (multi_install ? 0x01 : 0));
NIT: Redundant outer ()?

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/configuration_test.cc:80: base::win::RegKey key;
Should this be base::win::RegKey or mini_installer's own RegKey? If former,
please add comment on why.

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/configuration_test.cc:172: #if
defined(GOOGLE_CHROME_BUILD)
Apply #if to the entire test, rather than leave empty test?

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/configuration_test.cc:177: #endif
Is it worth testing interaction with AddChromeRegistryState()?

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/configuration_test.cc:202: #endif
Is it worth testing interaction with AddChromeRegistryState()?

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
File chrome/installer/mini_installer/mini_installer.cc (right):

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:81: // binaries; otherwise,
returns false.
NIT: "opens the key for the binaries" at end: Maybe be more specific, e.g.,
"multi-install binaries"?

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:100: void
WriteInstallResults(const Configuration& configuration,
NIT: Rename to WriteInstallResultsToGoogleUpdate() to make it obvious in line
880 that this operation is only for Google Chrome?

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:109: for (int i = 0; i < 2;
++i) {
NIT: Maybe be move explicit?

  for (bool binaries : {false, true}) {
...

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:133: for (int i = 0; i < 2;
++i) {
NIT: Same as above.

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
File chrome/installer/mini_installer/mini_installer_constants.cc (right):

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer_constants.cc:62: const wchar_t
kClientsKeyBase[] = L"Software\\Google\\Update\\Clients\\";
NIT: Mention that the terminating "\\" is important?

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
File chrome/installer/mini_installer/regkey.h (right):

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/regkey.h:58: // Initializes |key| with the
desired |access| to |app_guid|'s Clients key.
I see this function in code search already; need to Sync?

Meanwhile, it looks like:
- configuration.cc is the only user of this.
- |app_guid| is only useful for GOOGLE_CHROME_BUILD.
- |app_guid| is always |google_update::kAppGuid|.

I'd recommend moving OpenClientKey() and OpenClientStateKey() to local
namespace{} inside configuration.cc, with hard-coded values. But this is beyond
scope of CL.

[grt (UTC plus 2), 2017-01-31 19:39:57]

Thanks for the thorough review. I'll land this as-is and address your comments
on ToT in a new CL.

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

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2017-01-31 19:40:53]

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-31 19:49:06]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1485891603205760, "parent_rev":
"fdacdd761288cc4be81ae5cecdda88413e36d106", "commit_rev":
"7f55b083a84bb3fc58852c6d767f1ccdedcadee3"}

[commit-bot: I haz the power, 2017-01-31 19:49:39]

Description was changed from

==========
Fix -full fallback for diff updates (M56).

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

This is a partial merge of r438644.

BUG=686645
NOTRY=true
NOPRESUBMIT=true
==========

to

==========
Fix -full fallback for diff updates (M56).

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

This is a partial merge of r438644.

BUG=686645
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2663003003
Cr-Commit-Position: refs/branch-heads/2924@{#889}
Cr-Branched-From:
3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}
Committed:
https://chromium.googlesource.com/chromium/src/+/7f55b083a84bb3fc58852c6d767f...
==========

[commit-bot: I haz the power, 2017-01-31 19:49:40]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/7f55b083a84bb3fc58852c6d767f...

[grt (UTC plus 2), 2017-02-02 08:17:16]

Please see https://codereview.chromium.org/2670133002/ for the cleanups. Thanks.

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
File chrome/installer/mini_installer/configuration.cc (right):

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/configuration.cc:134: return
(OpenClientsKey(root, google_update::kAppGuid, KEY_QUERY_VALUE,
On 2017/01/31 17:40:04, huangs wrote:
> I'd prefer using |chrome_app_guid_| instead of google_update::kAppGuid, and do
> the same below. Maybe add
> 
> DCHECK_EQ(google_update::kAppGuid, chrome_app_guid_);
> 
> at 131?

Done.

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
File chrome/installer/mini_installer/configuration_test.cc (right):

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/configuration_test.cc:13: #include
"base/test/test_reg_util_win.h"
On 2017/01/31 17:40:04, huangs wrote:
> #include "base/macros.h" for DISALLOW_COPY_AND_ASSIGN ?

Done.

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/configuration_test.cc:71: #endif
On 2017/01/31 17:40:04, huangs wrote:
> #endif  // defined(GOOGLE_CHROME_BUILD)
> 
> or
> 
> #endif  // GOOGLE_CHROME_BUILD
> 
> and below if you'd prefer this style?

Done here. For the ones below that are pretty much one-liners, I find the
comments noisy rather than helpful.

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/configuration_test.cc:72: static constexpr const
wchar_t* kUninstallArguments[] = {
On 2017/01/31 17:40:04, huangs wrote:
> Would it be clearer to render |kUninstallArguments| from params?  I.e.:
> 
>   std::wstring uninstall_args = std::wstring(L"--uninstall") +
>       (system_level ? L" --system-level" : L"") +
>       (multi_install ? L" --multi-install --chrome" : L"");
> 
> ...
>   
>   uninstall_args.c_str();
> 

Done.

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/configuration_test.cc:78: ((system_level ? 0x02
: 0) | (multi_install ? 0x01 : 0));
On 2017/01/31 17:40:04, huangs wrote:
> NIT: Redundant outer ()?

code removed

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/configuration_test.cc:80: base::win::RegKey key;
On 2017/01/31 17:40:04, huangs wrote:
> Should this be base::win::RegKey or mini_installer's own RegKey? If former,
> please add comment on why.

Done

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/configuration_test.cc:172: #if
defined(GOOGLE_CHROME_BUILD)
On 2017/01/31 17:40:04, huangs wrote:
> Apply #if to the entire test, rather than leave empty test?

Done.

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/configuration_test.cc:177: #endif
On 2017/01/31 17:40:04, huangs wrote:
> Is it worth testing interaction with AddChromeRegistryState()?

I'm not sure there is any interaction. chrome_app_guid()'s behavior is solely
dictated by the command line (and is documented as such).

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/configuration_test.cc:202: #endif
On 2017/01/31 17:40:04, huangs wrote:
> Is it worth testing interaction with AddChromeRegistryState()?

is_side_by_side() was bad, so I've removed it.

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
File chrome/installer/mini_installer/mini_installer.cc (right):

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:81: // binaries; otherwise,
returns false.
On 2017/01/31 17:40:04, huangs wrote:
> NIT: "opens the key for the binaries" at end: Maybe be more specific, e.g.,
> "multi-install binaries"?

Done.

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:100: void
WriteInstallResults(const Configuration& configuration,
On 2017/01/31 17:40:04, huangs wrote:
> NIT: Rename to WriteInstallResultsToGoogleUpdate() to make it obvious in line
> 880 that this operation is only for Google Chrome?

I've added a TODO above the #if above for where I want this to go in the future.

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:109: for (int i = 0; i < 2;
++i) {
On 2017/01/31 17:40:04, huangs wrote:
> NIT: Maybe be move explicit?
> 
>   for (bool binaries : {false, true}) {
> ...

Awesome. I wasn't sure this was permitted by the style guide, but I see it done
elsewhere so it must be.

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
File chrome/installer/mini_installer/mini_installer_constants.cc (right):

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer_constants.cc:62: const wchar_t
kClientsKeyBase[] = L"Software\\Google\\Update\\Clients\\";
On 2017/01/31 17:40:04, huangs wrote:
> NIT: Mention that the terminating "\\" is important?

Done.

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
File chrome/installer/mini_installer/regkey.h (right):

https://codereview.chromium.org/2663003003/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/regkey.h:58: // Initializes |key| with the
desired |access| to |app_guid|'s Clients key.
On 2017/01/31 17:40:04, huangs wrote:
> I see this function in code search already; need to Sync?
> 
> Meanwhile, it looks like:
> - configuration.cc is the only user of this.
> - |app_guid| is only useful for GOOGLE_CHROME_BUILD.
> - |app_guid| is always |google_update::kAppGuid|.
> 
> I'd recommend moving OpenClientKey() and OpenClientStateKey() to local
> namespace{} inside configuration.cc, with hard-coded values. But this is
beyond
> scope of CL.

Yeah, I don't really like this either. OpenClientStateKey is used outside of
configuration.cc, so I'm going to leave these as-is for now.