Remove support for non-browser products from InstallationState and ProductState.

Remove support for non-browser products from InstallationState and ProductState.

- InstallationState::GetProductState loses its distribution type parameter.
- ProductState::Initialize loses its distribution-related parameters.

This change also contains a minor simplification of GetChromeUninstallCmd.

BUG=577816
Review-Url: https://codereview.chromium.org/2618583005
Cr-Commit-Position: refs/heads/master@{#442684}
Committed: https://chromium.googlesource.com/chromium/src/+/2ff7705a8ca000902ea0b6ebed65f01b90a646a9

[grt (UTC plus 2), 2017-01-06 13:43:30]

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

[commit-bot: I haz the power, 2017-01-06 13:43:41]

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 13:46:01]

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

[commit-bot: I haz the power, 2017-01-06 13:46:02]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[grt (UTC plus 2), 2017-01-06 15:56:35]

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

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

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 16:35:30]

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

[commit-bot: I haz the power, 2017-01-06 16:35:31]

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_...)

[huangs, 2017-01-09 09:09:09]

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

[huangs, 2017-01-09 09:09:11]

Mostly NITs.  Maybe add reviewer for chrome_browser_main_win.cc sooner than
later?

https://codereview.chromium.org/2618583005/diff/40001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/2618583005/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main_win.cc:463: // TODO(tommi): Check if using
the default distribution is always the right
Is this TODO now irrelevant?

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/f...
File chrome/installer/util/fake_installation_state.h (right):

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/f...
chrome/installer/util/fake_installation_state.h:36: ProductState& target =
GetProduct(system_install);
May as well combine into 1 line.

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/f...
chrome/installer/util/fake_installation_state.h:41: ProductState&
GetProduct(bool system_install) {
For mutation, I think style requires ProductState* ? Alternatively, just move
into SetProductState() if it's the only caller.

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

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/h...
chrome/installer/util/helper.cc:39: // package's app guid rather than the
browser's.  If we can't read the app's
NIT: Extra space after ".".

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/i...
File chrome/installer/util/install_util.h (right):

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/i...
chrome/installer/util/install_util.h:47: // If system_install is true the
command is read from HKLM, otherwise
NIT: Unwrap; and use |system_install|. Also should mention that empty command
line is returned if nothing is found?

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/i...
File chrome/installer/util/installation_state.cc (right):

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/i...
chrome/installer/util/installation_state.cc:201: // Initializes |commands| from
the "Commands" subkey of |version_key|.
Seems this is only called from within the file?  Maybe move to local namespace
{} ?

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/i...
chrome/installer/util/installation_state.cc:211: kAccess) == ERROR_SUCCESS)
NIT: Not sure if we should use {}? I'll leave to your discretion.

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/i...
chrome/installer/util/installation_state.cc:219: user_chrome_.Initialize(false);
// TODO(grt): Do something with return value, or remove if not needed.

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/p...
File chrome/installer/util/product_state_unittest.cc (right):

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/p...
chrome/installer/util/product_state_unittest.cc:33: : system_install_(true),
Test looks broken: It skips user-level altogether. Add TODO to fix this?

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/p...
File chrome/installer/util/product_unittest.cc (right):

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/p...
chrome/installer/util/product_unittest.cc:64:
EXPECT_TRUE(machine_state.GetProductState(system_level) == NULL);
InstallationState::GetProductState() is changed to returns nullptr, but I
presume you want to keep NULL usage consistent throughout file?

[grt (UTC plus 2), 2017-01-09 10:10:02]

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

[grt (UTC plus 2), 2017-01-09 10:10:11]

Description was changed from

==========
Remove support for non-browser products from InstallationState and ProductState.

- InstallationState::GetProductState loses its distribution type parameter.
- ProductState::Initialize loses its distribution-related parameters.

This change also contains a minor simplification of GetChromeUninstallCmd.

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

to

==========
Remove support for non-browser products from InstallationState and ProductState.

- InstallationState::GetProductState loses its distribution type parameter.
- ProductState::Initialize loses its distribution-related parameters.

This change also contains a minor simplification of GetChromeUninstallCmd.

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

[grt (UTC plus 2), 2017-01-09 10:10:11]

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

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

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

Thanks, Sam.

+jochen for chrome/browser/chrome_browser_main_win.cc OWNERS review.

https://codereview.chromium.org/2618583005/diff/40001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/2618583005/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main_win.cc:463: // TODO(tommi): Check if using
the default distribution is always the right
On 2017/01/09 09:09:10, huangs wrote:
> Is this TODO now irrelevant?

Yes!

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/f...
File chrome/installer/util/fake_installation_state.h (right):

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/f...
chrome/installer/util/fake_installation_state.h:36: ProductState& target =
GetProduct(system_install);
On 2017/01/09 09:09:10, huangs wrote:
> May as well combine into 1 line.

Done.

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/f...
chrome/installer/util/fake_installation_state.h:41: ProductState&
GetProduct(bool system_install) {
On 2017/01/09 09:09:10, huangs wrote:
> For mutation, I think style requires ProductState* ? Alternatively, just move
> into SetProductState() if it's the only caller.

Done.

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

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/h...
chrome/installer/util/helper.cc:39: // package's app guid rather than the
browser's.  If we can't read the app's
On 2017/01/09 09:09:10, huangs wrote:
> NIT: Extra space after ".".

Done.

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/i...
File chrome/installer/util/install_util.h (right):

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/i...
chrome/installer/util/install_util.h:47: // If system_install is true the
command is read from HKLM, otherwise
On 2017/01/09 09:09:10, huangs wrote:
> NIT: Unwrap; and use |system_install|. Also should mention that empty command
> line is returned if nothing is found?

Done.

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/i...
File chrome/installer/util/installation_state.cc (right):

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/i...
chrome/installer/util/installation_state.cc:201: // Initializes |commands| from
the "Commands" subkey of |version_key|.
On 2017/01/09 09:09:10, huangs wrote:
> Seems this is only called from within the file?  Maybe move to local namespace
> {} ?

Done.

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/i...
chrome/installer/util/installation_state.cc:211: kAccess) == ERROR_SUCCESS)
On 2017/01/09 09:09:10, huangs wrote:
> NIT: Not sure if we should use {}? I'll leave to your discretion.

I like them there. Thanks.

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/i...
chrome/installer/util/installation_state.cc:219: user_chrome_.Initialize(false);
On 2017/01/09 09:09:10, huangs wrote:
> // TODO(grt): Do something with return value, or remove if not needed.

Return value of ProductState::Initialize isn't used here, but is used by other
callers. Leaving as-is.

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/p...
File chrome/installer/util/product_state_unittest.cc (right):

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/p...
chrome/installer/util/product_state_unittest.cc:33: : system_install_(true),
On 2017/01/09 09:09:10, huangs wrote:
> Test looks broken: It skips user-level altogether. Add TODO to fix this?

Added coverage for user-level.

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/p...
File chrome/installer/util/product_unittest.cc (right):

https://codereview.chromium.org/2618583005/diff/40001/chrome/installer/util/p...
chrome/installer/util/product_unittest.cc:64:
EXPECT_TRUE(machine_state.GetProductState(system_level) == NULL);
On 2017/01/09 09:09:10, huangs wrote:
> InstallationState::GetProductState() is changed to returns nullptr, but I
> presume you want to keep NULL usage consistent throughout file?

NULL -> nullptr in this file

[commit-bot: I haz the power, 2017-01-09 10:12:02]

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

[commit-bot: I haz the power, 2017-01-09 10:12:02]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

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

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

[commit-bot: I haz the power, 2017-01-09 10:19:32]

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-09 10:57:46]

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

[commit-bot: I haz the power, 2017-01-09 10:57:47]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-01-09 13:09:34]

chrome_browser_main lgtm

[grt (UTC plus 2), 2017-01-10 10:40:50]

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:40: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, 2017-01-10 11:35:06]

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

[commit-bot: I haz the power, 2017-01-10 11:35:07]

Dry run: This issue passed the CQ dry run.

[huangs, 2017-01-10 18:30:46]

LGTM.

[grt (UTC plus 2), 2017-01-10 19:30:14]

The CQ bit was checked by grt@chromium.org

[grt (UTC plus 2), 2017-01-10 19:30:14]

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

[commit-bot: I haz the power, 2017-01-10 19:30:57]

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 19:46:29]

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

[commit-bot: I haz the power, 2017-01-10 19:46:32]

Failed to apply patch for chrome/installer/setup/uninstall.cc:
While running git apply --index -p1;
  error: patch failed: chrome/installer/setup/uninstall.cc:988
  error: chrome/installer/setup/uninstall.cc: patch does not apply

Patch:       chrome/installer/setup/uninstall.cc
Index: chrome/installer/setup/uninstall.cc
diff --git a/chrome/installer/setup/uninstall.cc
b/chrome/installer/setup/uninstall.cc
index
f19873d91caa56f45712564669d4a7927dce7a24..60747ead92942d76332884c9be2e96ebcb7b515e
100644
--- a/chrome/installer/setup/uninstall.cc
+++ b/chrome/installer/setup/uninstall.cc
@@ -988,8 +988,7 @@ InstallStatus UninstallProduct(const InstallationState&
original_state,
 
   // Get the state of the installed product (if any)
   const ProductState* product_state =
-      original_state.GetProductState(installer_state.system_install(),
-                                     browser_dist->GetType());
+      original_state.GetProductState(installer_state.system_install());
 
   // Delete shared registry keys as well (these require admin rights) if
   // remove_all option is specified.

[grt (UTC plus 2), 2017-01-10 19:59:02]

The CQ bit was checked by grt@chromium.org

[grt (UTC plus 2), 2017-01-10 19:59:02]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
huangs@chromium.org
Link to the patchset: https://codereview.chromium.org/2618583005/#ps140001
(title: "sync to position 442664")

[commit-bot: I haz the power, 2017-01-10 19:59:33]

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:09:39]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484078342301970,
"parent_rev": "44ec2a4fd5e1c052077575f84b5b5cbdb703cef6", "commit_rev":
"2ff7705a8ca000902ea0b6ebed65f01b90a646a9"}

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

Description was changed from

==========
Remove support for non-browser products from InstallationState and ProductState.

- InstallationState::GetProductState loses its distribution type parameter.
- ProductState::Initialize loses its distribution-related parameters.

This change also contains a minor simplification of GetChromeUninstallCmd.

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

to

==========
Remove support for non-browser products from InstallationState and ProductState.

- InstallationState::GetProductState loses its distribution type parameter.
- ProductState::Initialize loses its distribution-related parameters.

This change also contains a minor simplification of GetChromeUninstallCmd.

BUG=577816

Review-Url: https://codereview.chromium.org/2618583005
Cr-Commit-Position: refs/heads/master@{#442684}
Committed:
https://chromium.googlesource.com/chromium/src/+/2ff7705a8ca000902ea0b6ebed65...
==========

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

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