Remove multi-install from install_static.

Remove multi-install from install_static.

BUG=577816
Review-Url: https://codereview.chromium.org/2601443002
Cr-Commit-Position: refs/heads/master@{#442874}
Committed: https://chromium.googlesource.com/chromium/src/+/8b090304788061c3bb4b2d4ca58d0461c01c646c

[grt (UTC plus 2), 2016-12-22 09:54:41]

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

[commit-bot: I haz the power, 2016-12-22 09:54: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, 2016-12-22 11:01:39]

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

[commit-bot: I haz the power, 2016-12-22 11:01:40]

Dry run: This issue passed the CQ dry run.

[huangs, 2016-12-27 20:04:21]

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

[huangs, 2016-12-27 20:04:23]

Initial comments. There may be more dead code to remove.

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/insta...
File chrome/install_static/install_constants.h (right):

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/insta...
chrome/install_static/install_constants.h:61: // True if this mode supported
multi-install.
Add "(legacy)"?

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/insta...
File chrome/install_static/install_modes.h (right):

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/insta...
chrome/install_static/install_modes.h:55: // The GUID with which the brand's
multi-install binaries were registered with
Add "(legacy)" for multi-install? Same below.

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/insta...
chrome/install_static/install_modes.h:76: std::wstring
GetBinariesClientStateKeyPath();
Looks like:
- GetBinariesClientStateKeyPath()
- GetBinariesClientStateMediumKeyPath()
- kBinariesPathName
become dead code after this CL? So maybe delete in this CL, or add TODO?

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/insta...
File chrome/install_static/install_util.cc (right):

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/insta...
chrome/install_static/install_util.cc:293: // strips off known modifiers (e.g.,
"-multi-full") to see if the empty string
Update comment re. "-multi-full"?

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/insta...
File chrome/install_static/install_util.h (right):

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/insta...
chrome/install_static/install_util.h:128: // TODO(grt): Remove |add_modifier|.
FYI Chromium code search shows that GetChromeChannelName() is only used by
install_static. But disregard if there's a deeper reason for the TODO.

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/produ...
File chrome/install_static/product_install_details_unittest.cc (right):

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/produ...
chrome/install_static/product_install_details_unittest.cc:258:
{L"-multi-chrome", L""},
Address these?  So maybe remove.  Alternatively, add "// Legacy", and add
"-multi-install" as test case?

[grt (UTC plus 2), 2017-01-02 12:38:35]

Patchset #2 (id:20001) has been deleted

[grt (UTC plus 2), 2017-01-02 12:58:11]

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

[commit-bot: I haz the power, 2017-01-02 12:58:15]

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-02 12:58:45]

Thank you!

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/insta...
File chrome/install_static/install_constants.h (right):

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/insta...
chrome/install_static/install_constants.h:61: // True if this mode supported
multi-install.
On 2016/12/27 20:04:22, huangs wrote:
> Add "(legacy)"?

Done.

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/insta...
File chrome/install_static/install_modes.h (right):

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/insta...
chrome/install_static/install_modes.h:55: // The GUID with which the brand's
multi-install binaries were registered with
On 2016/12/27 20:04:22, huangs wrote:
> Add "(legacy)" for multi-install? Same below.

Done.

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/insta...
chrome/install_static/install_modes.h:76: std::wstring
GetBinariesClientStateKeyPath();
On 2016/12/27 20:04:22, huangs wrote:
> Looks like:
> - GetBinariesClientStateKeyPath()
> - GetBinariesClientStateMediumKeyPath()
> - kBinariesPathName
> become dead code after this CL? So maybe delete in this CL, or add TODO?

Good catch. I left these in because I think I'll have uses for them in
install_util and setup once those components use install_static (which is on
hold until these multi-install CLs land).

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/insta...
File chrome/install_static/install_util.cc (right):

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/insta...
chrome/install_static/install_util.cc:293: // strips off known modifiers (e.g.,
"-multi-full") to see if the empty string
On 2016/12/27 20:04:22, huangs wrote:
> Update comment re. "-multi-full"?

Done.

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/insta...
File chrome/install_static/install_util.h (right):

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/insta...
chrome/install_static/install_util.h:128: // TODO(grt): Remove |add_modifier|.
On 2016/12/27 20:04:22, huangs wrote:
> FYI Chromium code search shows that GetChromeChannelName() is only used by
> install_static. But disregard if there's a deeper reason for the TODO.

TODO->Done!

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/produ...
File chrome/install_static/product_install_details_unittest.cc (right):

https://codereview.chromium.org/2601443002/diff/1/chrome/install_static/produ...
chrome/install_static/product_install_details_unittest.cc:258:
{L"-multi-chrome", L""},
On 2016/12/27 20:04:23, huangs wrote:
> Address these?  So maybe remove.  Alternatively, add "// Legacy", and add
> "-multi-install" as test case?

There will still be clients with "-multi" in their "ap" values, so these are
still valid test cases. Will add // Legacy.

[commit-bot: I haz the power, 2017-01-02 14:02:47]

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

[commit-bot: I haz the power, 2017-01-02 14:02:47]

Dry run: This issue passed the CQ dry run.

[huangs, 2017-01-03 05:35:14]

LGTM with a few suggestions.

https://codereview.chromium.org/2601443002/diff/40001/chrome/install_static/i...
File chrome/install_static/install_details_unittest.cc (right):

https://codereview.chromium.org/2601443002/diff/40001/chrome/install_static/i...
chrome/install_static/install_details_unittest.cc:25:
constants.supported_multi_install = true;
Should there be a test for |supported_multi_install = false|?  Maybe add TODO?

https://codereview.chromium.org/2601443002/diff/40001/chrome/install_static/i...
File chrome/install_static/install_util.cc (right):

https://codereview.chromium.org/2601443002/diff/40001/chrome/install_static/i...
chrome/install_static/install_util.cc:512: return
InstallDetails::Get().channel();
Will we need to change Google Update to accommodate this?

https://codereview.chromium.org/2601443002/diff/40001/chrome/install_static/i...
File chrome/install_static/install_util_unittest.cc (right):

https://codereview.chromium.org/2601443002/diff/40001/chrome/install_static/i...
chrome/install_static/install_util_unittest.cc:418: // Stable user and system
levels.
s/Stable/Stable supports/  to match line 429 below?

https://codereview.chromium.org/2601443002/diff/40001/chrome/install_static/p...
File chrome/install_static/product_install_details.cc (right):

https://codereview.chromium.org/2601443002/diff/40001/chrome/install_static/p...
chrome/install_static/product_install_details.cc:8: #include <assert.h>
Can remove <assert.h>?

[grt (UTC plus 2), 2017-01-03 13:47:52]

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

[commit-bot: I haz the power, 2017-01-03 13:48:07]

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-03 13:57:00]

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

[commit-bot: I haz the power, 2017-01-03 13:57: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-03 14:47:59]

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

[commit-bot: I haz the power, 2017-01-03 14:48:00]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2017-01-03 15:47:57]

Many thanks.

https://codereview.chromium.org/2601443002/diff/40001/chrome/install_static/i...
File chrome/install_static/install_details_unittest.cc (right):

https://codereview.chromium.org/2601443002/diff/40001/chrome/install_static/i...
chrome/install_static/install_details_unittest.cc:25:
constants.supported_multi_install = true;
On 2017/01/03 05:35:14, huangs wrote:
> Should there be a test for |supported_multi_install = false|?  Maybe add TODO?

There's nothing in InstallDetails that depends on |supported_multi_install|, so
there's nothing to test here. I'll make sure there is test coverage in
https://codereview.chromium.org/2459583002/ (Use InstallDetails in setup) as I
rebase it on top of this.

https://codereview.chromium.org/2601443002/diff/40001/chrome/install_static/i...
File chrome/install_static/install_util.cc (right):

https://codereview.chromium.org/2601443002/diff/40001/chrome/install_static/i...
chrome/install_static/install_util.cc:512: return
InstallDetails::Get().channel();
On 2017/01/03 05:35:14, huangs wrote:
> Will we need to change Google Update to accommodate this?

No. This change is effectively just dead code removal since --multi-install at
install/update time is ignored as of r438644. Need-to-know teams have been
notified of that change. Thanks for asking!

https://codereview.chromium.org/2601443002/diff/40001/chrome/install_static/i...
File chrome/install_static/install_util_unittest.cc (right):

https://codereview.chromium.org/2601443002/diff/40001/chrome/install_static/i...
chrome/install_static/install_util_unittest.cc:418: // Stable user and system
levels.
On 2017/01/03 05:35:14, huangs wrote:
> s/Stable/Stable supports/  to match line 429 below?

Done.

https://codereview.chromium.org/2601443002/diff/40001/chrome/install_static/p...
File chrome/install_static/product_install_details.cc (right):

https://codereview.chromium.org/2601443002/diff/40001/chrome/install_static/p...
chrome/install_static/product_install_details.cc:8: #include <assert.h>
On 2017/01/03 05:35:14, huangs wrote:
> Can remove <assert.h>?

Done.

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

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:02: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-06 18:10:39]

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

[commit-bot: I haz the power, 2017-01-06 18:10:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[grt (UTC plus 2), 2017-01-11 11:12:02]

The CQ bit was checked by grt@chromium.org

[grt (UTC plus 2), 2017-01-11 11:12:02]

The patchset sent to the CQ was uploaded after l-g-t-m from huangs@chromium.org
Link to the patchset: https://codereview.chromium.org/2601443002/#ps140001
(title: "rebase onto position 442831")

[commit-bot: I haz the power, 2017-01-11 11:12:17]

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-11 11:55:41]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484133122280830,
"parent_rev": "cdf612491968c39e7d86847308e8a9225f19e7af", "commit_rev":
"8b090304788061c3bb4b2d4ca58d0461c01c646c"}

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

Description was changed from

==========
Remove multi-install from install_static.

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

to

==========
Remove multi-install from install_static.

BUG=577816

Review-Url: https://codereview.chromium.org/2601443002
Cr-Commit-Position: refs/heads/master@{#442874}
Committed:
https://chromium.googlesource.com/chromium/src/+/8b090304788061c3bb4b2d4ca58d...
==========

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

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/8b090304788061c3bb4b2d4ca58d...