Remove uses of BrowserDistribution::CHROME_BINARIES in installer_util.

Remove uses of BrowserDistribution::CHROME_BINARIES in installer_util.

This CL also includes the following changes which trickle gently through
small corners of the codebase:

- Removes the |system-level| param from UpdateDidRunState.
- Removes GetChromeChannelAndModifiers; "-m", the "mutli-install"
  modifier, was the only one.
- Replaces installer::GetAppGuidForUpdates with
  BrowserDistribution::GetDistribution()->GetAppGuid(), since it is now
  correct in the absence of multi-install.

BUG=577816
Review-Url: https://codereview.chromium.org/2619323003
Cr-Commit-Position: refs/heads/master@{#442695}
Committed: https://chromium.googlesource.com/chromium/src/+/9074f30e1913ea6fe1244f43b38f9a9be21f889e

[grt (UTC plus 2), 2017-01-09 19:41:35]

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

[commit-bot: I haz the power, 2017-01-09 19:42:38]

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-01-09 20:18:17]

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

[commit-bot: I haz the power, 2017-01-09 20:18:54]

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-01-09 20:27:06]

Patchset #1 (id:1) has been deleted

[grt (UTC plus 2), 2017-01-09 20:27:27]

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

[grt (UTC plus 2), 2017-01-09 20:27:43]

Description was changed from

==========
Remove uses of BrowserDistribution::CHROME_BINARIES in installer_util.

This CL also replaces installer::GetAppGuidForUpdates with
BrowserDistribution::GetDistribution()->GetAppGuid(), since it is now
correct in the absence of multi-install.

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

to

==========
Remove uses of BrowserDistribution::CHROME_BINARIES in installer_util.

This CL also includes the following changes which trickle gently through
small corners of the codebase:

- Removes the |system-level| param from UpdateDidRunState.
- Removes GetChromeChannelAndModifiers; "-m", the "mutli-install"
  modifier, was the only one.
- Replaces installer::GetAppGuidForUpdates with
  BrowserDistribution::GetDistribution()->GetAppGuid(), since it is now
  correct in the absence of multi-install.

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

[commit-bot: I haz the power, 2017-01-09 20:27:52]

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-01-09 20:28:28]

Patchset #1 (id:20001) has been deleted

[Sébastien Marchand, 2017-01-09 20:41:22]

sebmarchand@chromium.org changed reviewers:
+ sebmarchand@chromium.org

[Sébastien Marchand, 2017-01-09 20:41:22]

lgtm with one question.

https://codereview.chromium.org/2619323003/diff/40001/chrome/common/channel_i...
File chrome/common/channel_info_win.cc (right):

https://codereview.chromium.org/2619323003/diff/40001/chrome/common/channel_i...
chrome/common/channel_info_win.cc:36: channel += L" SyzyASan";
Should this go in the "if (PathService::Get(base::FILE_MODULE, &module)" block?

[grt (UTC plus 2), 2017-01-09 20:49:51]

Thanks, Seb!

https://codereview.chromium.org/2619323003/diff/40001/chrome/common/channel_i...
File chrome/common/channel_info_win.cc (right):

https://codereview.chromium.org/2619323003/diff/40001/chrome/common/channel_i...
chrome/common/channel_info_win.cc:36: channel += L" SyzyASan";
On 2017/01/09 20:41:22, Sébastien Marchand wrote:
> Should this go in the "if (PathService::Get(base::FILE_MODULE, &module)"
block?

Hmm. Having it out here means that " SyzyAsan" will be appended unconditionally
-- regardless of whether or not GoogleUpdateSettings could be used. I think this
is the right thing. The fact that Asan is active is independent of the condition
above. Does that make sense to you?

[grt (UTC plus 2), 2017-01-09 20:51:20]

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

[grt (UTC plus 2), 2017-01-09 20:51:44]

+jochen for chrome/browser and chrome/common OWNERS review

[Sébastien Marchand, 2017-01-09 20:56:14]

https://codereview.chromium.org/2619323003/diff/40001/chrome/common/channel_i...
File chrome/common/channel_info_win.cc (right):

https://codereview.chromium.org/2619323003/diff/40001/chrome/common/channel_i...
chrome/common/channel_info_win.cc:36: channel += L" SyzyASan";
On 2017/01/09 20:49:51, grt (UTC plus 1) wrote:
> On 2017/01/09 20:41:22, Sébastien Marchand wrote:
> > Should this go in the "if (PathService::Get(base::FILE_MODULE, &module)"
> block?
> 
> Hmm. Having it out here means that " SyzyAsan" will be appended
unconditionally
> -- regardless of whether or not GoogleUpdateSettings could be used. I think
this
> is the right thing. The fact that Asan is active is independent of the
condition
> above. Does that make sense to you?

Ha yeah sorry, my question didn't really make sense :). Should we replace:
    channel += L" SyzyASan";
by:
    if (!channel.empty())
      channel += L' ';
    channel += 'SyzyAsan';

Otherwise we could end up in a situation where we have two spaces prefixing the
'SyzyAsan' tag, my OCD really don't like it. (this is beyond the scope of this
CL so I don't mind doing it in a separate CL if you want)

[commit-bot: I haz the power, 2017-01-09 21:03:13]

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

[commit-bot: I haz the power, 2017-01-09 21:03:13]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  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_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Georges Khalil, 2017-01-09 21:13:45]

georgesak@chromium.org changed reviewers:
+ georgesak@chromium.org

[Georges Khalil, 2017-01-09 21:13:46]

Fly by review, LGTM.

[Sébastien Marchand, 2017-01-09 21:19:39]

https://codereview.chromium.org/2619323003/diff/40001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/2619323003/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:427: bool
GoogleUpdateSettings::UpdateDidRunState(bool did_run, bool system_level) {
Oops, you forgot to remove the |bool system_level| argument.

[huangs, 2017-01-09 21:27:07]

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

[huangs, 2017-01-09 21:27:08]

Just NITs.

https://codereview.chromium.org/2619323003/diff/40001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/2619323003/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:11: #include <vector>
<vector> now unused?

https://codereview.chromium.org/2619323003/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:526: if
(value->SetMultiFailSuffix(false)) {
Should this be addressed? If we should leave this, maybe add brief comment on
why (e.g., clean up legacy)?

https://codereview.chromium.org/2619323003/diff/40001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings_unittest.cc (right):

https://codereview.chromium.org/2619323003/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings_unittest.cc:108: base::string16
ret_channel;
Combine with line below?

https://codereview.chromium.org/2619323003/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings_unittest.cc:1182: static
base::string16* chrome_version_key_;
Kinda weird that these are pointers, and we call delete explicitly? Add TODO to
cleanup?

https://codereview.chromium.org/2619323003/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings_unittest.cc:1262: const
base::string16* const reg_keys[] = {
Can simplify these 6 lines to

const base::string16& reg_key = GetParam().system_level() ?
    *chrome_state_medium_key_ : *chrome_state_key_;

https://codereview.chromium.org/2619323003/diff/40001/chrome/installer/util/h...
File chrome/installer/util/helper.h (right):

https://codereview.chromium.org/2619323003/diff/40001/chrome/installer/util/h...
chrome/installer/util/helper.h:13: class FilePath;
FilePath is returned by value below, so should replace forward-declaration with

#include "base/files/file_path.h"

? Also remove #include in .cc file.

[grt (UTC plus 2), 2017-01-10 08:43:56]

Thanks!

https://codereview.chromium.org/2619323003/diff/40001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/2619323003/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:11: #include <vector>
On 2017/01/09 21:27:08, huangs wrote:
> <vector> now unused?

You bet. <limits>, too.

https://codereview.chromium.org/2619323003/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:427: bool
GoogleUpdateSettings::UpdateDidRunState(bool did_run, bool system_level) {
On 2017/01/09 21:19:39, Sébastien Marchand wrote:
> Oops, you forgot to remove the |bool system_level| argument.

Doh!

https://codereview.chromium.org/2619323003/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:526: if
(value->SetMultiFailSuffix(false)) {
On 2017/01/09 21:27:08, huangs wrote:
> Should this be addressed? If we should leave this, maybe add brief comment on
> why (e.g., clean up legacy)?

Done.

https://codereview.chromium.org/2619323003/diff/40001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings_unittest.cc (right):

https://codereview.chromium.org/2619323003/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings_unittest.cc:108: base::string16
ret_channel;
On 2017/01/09 21:27:08, huangs wrote:
> Combine with line below?

Done.

https://codereview.chromium.org/2619323003/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings_unittest.cc:1182: static
base::string16* chrome_version_key_;
On 2017/01/09 21:27:08, huangs wrote:
> Kinda weird that these are pointers, and we call delete explicitly? Add TODO
to
> cleanup?

Indeed. Already fixed in https://codereview.chromium.org/2476573004/. :-) Will
add todo.

https://codereview.chromium.org/2619323003/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings_unittest.cc:1262: const
base::string16* const reg_keys[] = {
On 2017/01/09 21:27:08, huangs wrote:
> Can simplify these 6 lines to
> 
> const base::string16& reg_key = GetParam().system_level() ?
>     *chrome_state_medium_key_ : *chrome_state_key_;

Done.

https://codereview.chromium.org/2619323003/diff/40001/chrome/installer/util/h...
File chrome/installer/util/helper.h (right):

https://codereview.chromium.org/2619323003/diff/40001/chrome/installer/util/h...
chrome/installer/util/helper.h:13: class FilePath;
On 2017/01/09 21:27:08, huangs wrote:
> FilePath is returned by value below, so should replace forward-declaration
with
> 
> #include "base/files/file_path.h"
> 
> ? Also remove #include in .cc file.

Yes, indeed!

[jochen (gone - plz use gerrit), 2017-01-10 09:46:30]

lgtm

[grt (UTC plus 2), 2017-01-10 10:43:24]

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

[commit-bot: I haz the power, 2017-01-10 10:43:44]

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-10 11:56:20]

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

[commit-bot: I haz the power, 2017-01-10 11:56:21]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2017-01-10 21:25:44]

The CQ bit was checked by grt@chromium.org

[grt (UTC plus 2), 2017-01-10 21:25:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
sebmarchand@chromium.org, georgesak@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2619323003/#ps80001
(title: "sync to position 442533")

[commit-bot: I haz the power, 2017-01-10 21:26:08]

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-10 21:31:06]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484083544563040,
"parent_rev": "63f9112d35ca0514c9eab4b8bc538f277a6343b0", "commit_rev":
"9074f30e1913ea6fe1244f43b38f9a9be21f889e"}

[commit-bot: I haz the power, 2017-01-10 21:31:36]

Description was changed from

==========
Remove uses of BrowserDistribution::CHROME_BINARIES in installer_util.

This CL also includes the following changes which trickle gently through
small corners of the codebase:

- Removes the |system-level| param from UpdateDidRunState.
- Removes GetChromeChannelAndModifiers; "-m", the "mutli-install"
  modifier, was the only one.
- Replaces installer::GetAppGuidForUpdates with
  BrowserDistribution::GetDistribution()->GetAppGuid(), since it is now
  correct in the absence of multi-install.

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

to

==========
Remove uses of BrowserDistribution::CHROME_BINARIES in installer_util.

This CL also includes the following changes which trickle gently through
small corners of the codebase:

- Removes the |system-level| param from UpdateDidRunState.
- Removes GetChromeChannelAndModifiers; "-m", the "mutli-install"
  modifier, was the only one.
- Replaces installer::GetAppGuidForUpdates with
  BrowserDistribution::GetDistribution()->GetAppGuid(), since it is now
  correct in the absence of multi-install.

BUG=577816

Review-Url: https://codereview.chromium.org/2619323003
Cr-Commit-Position: refs/heads/master@{#442695}
Committed:
https://chromium.googlesource.com/chromium/src/+/9074f30e1913ea6fe1244f43b38f...
==========

[commit-bot: I haz the power, 2017-01-10 21:31:37]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9074f30e1913ea6fe1244f43b38f...