Refactor chrome_launcher_support::GetAnyChromePath.

chrome/installer/launcher_support/chrome_launcher_support.cc

Index: chrome/installer/launcher_support/chrome_launcher_support.cc
diff --git a/chrome/installer/launcher_support/chrome_launcher_support.cc b/chrome/installer/launcher_support/chrome_launcher_support.cc
index 8d052a9b88a5efe36a9eed2a21e786147d93379f..7c53b7c858727bf15419d6e54152402d53483a55 100644
--- a/chrome/installer/launcher_support/chrome_launcher_support.cc
+++ b/chrome/installer/launcher_support/chrome_launcher_support.cc
@@ -116,20 +116,19 @@ base::FilePath GetChromePathForInstallationLevel(InstallationLevel level) {
       GetSetupExeForInstallationLevel(level), kChromeExe);
 }
 
-base::FilePath GetAnyChromePath() {
-  base::FilePath chrome_path;
-  if (chrome_path.empty())
-    chrome_path = GetChromePathForInstallationLevel(SYSTEM_LEVEL_INSTALLATION);
-  if (chrome_path.empty())
-    chrome_path = GetChromePathForInstallationLevel(USER_LEVEL_INSTALLATION);
-  return chrome_path;
-}
-
-base::FilePath GetAnyChromeSxSPath() {
-  base::FilePath path =
-      GetChromeSxSPathForInstallationLevel(USER_LEVEL_INSTALLATION);
-  if (path.empty())
-    path = GetChromeSxSPathForInstallationLevel(SYSTEM_LEVEL_INSTALLATION);
+base::FilePath GetAnyChromePath(bool is_canary) {
+  base::FilePath path;
+  if (is_canary) {
+    // TODO(mgiuca): Ask jackhou why this logic is reversed compared to the
+    // non-canary path.

[Matt Giuca, 2014/10/30 02:54:50]

Question for Jack: Is there a reason you prioritize user-level Chrome in
GetAnyChromeSxSPath (why is it inconsistent)?

[jackhou1, 2014/10/30 03:59:22]

Canary is installed at user level by default.

[Matt Giuca, 2014/10/30 04:22:58]

Thanks, I will update this comment then.

[grt (UTC plus 2), 2014/10/30 16:44:53]

Not only is it user-level by default, it's always user-level at the moment.
Since this is a deployment decision, however, it probably makes sense to not
make the code reliant on this. I think it would simplify to check system-level
and then user-level for both regular and canary.

[Vitaly Buka (NO REVIEWS), 2014/10/30 16:46:17]

Yes, It's installed. Still, my guess, consistency makes sense here.
If we start installing canary on system level, it will require no changes.

[grt (UTC plus 2), 2014/10/31 03:29:00]

Agreed. This is what I meant with my previous comment: treat canary and regular
Chrome the same in that system-level is preferred over user-level.

[Matt Giuca, 2015/01/09 06:17:32]

Done.

+    path = GetChromeSxSPathForInstallationLevel(USER_LEVEL_INSTALLATION);
+    if (path.empty())
+      path = GetChromeSxSPathForInstallationLevel(SYSTEM_LEVEL_INSTALLATION);
+  } else {
+    path = GetChromePathForInstallationLevel(SYSTEM_LEVEL_INSTALLATION);

[Vitaly Buka (NO REVIEWS), 2014/10/30 16:46:17]

Could you fall back here to non-canary if canary is missing?

[jackhou1, 2014/10/31 00:05:54]

For app_installer at least, it would be preferable that this either returns a
valid canary path, or fails. So I don't think we should silently fall back here.

[grt (UTC plus 2), 2014/10/31 03:29:00]

If it makes sense for some consumers to take whatever is available, while other
consumers specifically want either SxS or regular, then the |is_canary| could
become:

enum ChromePreference {
  GOOGLE_CHROME_ONLY,  // this is "is_canary == false"
  SXS_CHROME_ONLY,  // this is "is_canary == true"
  PREFER_GOOGLE_CHROME,  // this tries both
}

or something like that. for an even better user experience, PREFER_GOOGLE_CHROME
should be PREFER_USER_DEFAULT_CHROME, but see my previous comment about only
doing this if it's really easy to do.

[Matt Giuca, 2015/01/09 06:17:32]

I'd like to hold off on doing this unless there is a specific example of a
consumer that wants to fall back onto regular Chrome if canary is not installed.

I can't imagine a consumer that would: canary is effectively a separate product
to Chrome and the two should not interact at all. (If something wants to simply
"run the default web browser", like apps do, then that is another matter, but
that shouldn't be using this code path.)

[grt (UTC plus 2), 2015/01/09 17:45:28]

SGTM

+    if (path.empty())
+      path = GetChromePathForInstallationLevel(USER_LEVEL_INSTALLATION);
+  }
   return path;
 }