Refactor chrome_launcher_support::GetAnyChromePath.

Refactor chrome_launcher_support::GetAnyChromePath.

Combined GetAnyChromePath and GetChromeSxSPath into one function that
takes a bool |is_sxs|. Avoids the need to have a wrapper function that
conditionally calls GetAnyChromePath or GetAnyChromeSxsPath.

BUG=428600
Committed: https://crrev.com/e1b07409f19d84853d4730e3ab5b862fd65838d0
Cr-Commit-Position: refs/heads/master@{#311384}

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

mgiuca@chromium.org changed reviewers:
+ grt@chromium.org, jackhou@chromium.org, vitalybuka@chromium.org

[Matt Giuca, 2014-10-30 02:54:51]

jackhou@chromium.org: chrome/app_installer.

grt@chromium.org: chrome/installer.

vitalybuka@chromium.org: cloud_print.

Thanks!

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
File chrome/installer/launcher_support/chrome_launcher_support.cc (right):

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
chrome/installer/launcher_support/chrome_launcher_support.cc:123: // non-canary
path.
Question for Jack: Is there a reason you prioritize user-level Chrome in
GetAnyChromeSxSPath (why is it inconsistent)?

https://codereview.chromium.org/685103004/diff/1/cloud_print/service/win/chro...
File cloud_print/service/win/chrome_launcher.cc (right):

https://codereview.chromium.org/685103004/diff/1/cloud_print/service/win/chro...
cloud_print/service/win/chrome_launcher.cc:202:
chrome_launcher_support::GetAnyChromePath(false);
Question for Vitaly: Is there a reason why cloud_print is not getting the SxS
(canary) path for Canary builds? Is it not supported?

Maybe in the future this will make it easier to launch a Canary Chrome.

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

app_installer lgtm

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
File chrome/installer/launcher_support/chrome_launcher_support.cc (right):

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
chrome/installer/launcher_support/chrome_launcher_support.cc:123: // non-canary
path.
On 2014/10/30 02:54:50, Matt Giuca wrote:
> Question for Jack: Is there a reason you prioritize user-level Chrome in
> GetAnyChromeSxSPath (why is it inconsistent)?

Canary is installed at user level by default.

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

I've also discovered in chrome/installer/gcapi/gcapi.cc this logic is
duplicated. GetGoogleChromePath is essentially the same as
chrome_launcher_support::GetAnyChromePath, but it does it in a round-about way
by doing its own internal check whether Chrome is installed in system or user
level, then directly calling GetChromePathForInstallationLevel. It can probably
be factored into just using GetAnyChromePath.

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
File chrome/installer/launcher_support/chrome_launcher_support.cc (right):

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
chrome/installer/launcher_support/chrome_launcher_support.cc:123: // non-canary
path.
Thanks, I will update this comment then.

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

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
File chrome/installer/launcher_support/chrome_launcher_support.cc (right):

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
chrome/installer/launcher_support/chrome_launcher_support.cc:123: // non-canary
path.
On 2014/10/30 03:59:22, jackhou1 wrote:
> On 2014/10/30 02:54:50, Matt Giuca wrote:
> > Question for Jack: Is there a reason you prioritize user-level Chrome in
> > GetAnyChromeSxSPath (why is it inconsistent)?
> 
> Canary is installed at user level by default.

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.

https://codereview.chromium.org/685103004/diff/1/cloud_print/service/win/chro...
File cloud_print/service/win/chrome_launcher.cc (right):

https://codereview.chromium.org/685103004/diff/1/cloud_print/service/win/chro...
cloud_print/service/win/chrome_launcher.cc:202:
chrome_launcher_support::GetAnyChromePath(false);
On 2014/10/30 02:54:50, Matt Giuca wrote:
> Question for Vitaly: Is there a reason why cloud_print is not getting the SxS
> (canary) path for Canary builds? Is it not supported?
> 
> Maybe in the future this will make it easier to launch a Canary Chrome.

The canary population is extremely small, and in most cases Canary can't be made
the default browser. I can see adding a fallback here to use canary if regular
Chrome isn't installed, but my gut tells me that non-canary should always be
preferred. The only exception to this would be if there's a very simple way to
determine that canary is actually the user's default browser when both are
installed. Since this is rarely the case, I wouldn't put a lot of effort into
coding it.

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

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
File chrome/installer/launcher_support/chrome_launcher_support.cc (right):

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
chrome/installer/launcher_support/chrome_launcher_support.cc:123: // non-canary
path.
On 2014/10/30 03:59:22, jackhou1 wrote:
> On 2014/10/30 02:54:50, Matt Giuca wrote:
> > Question for Jack: Is there a reason you prioritize user-level Chrome in
> > GetAnyChromeSxSPath (why is it inconsistent)?
> 
> Canary is installed at user level by default.

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

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
chrome/installer/launcher_support/chrome_launcher_support.cc:128: path =
GetChromePathForInstallationLevel(SYSTEM_LEVEL_INSTALLATION);
Could you fall back here to non-canary if canary is missing?

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
File chrome/installer/launcher_support/chrome_launcher_support.h (right):

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
chrome/installer/launcher_support/chrome_launcher_support.h:28: // If
|is_canary| is specified, gets the path to the SxS (Canary) version of
|is_canary| is true

https://codereview.chromium.org/685103004/diff/1/cloud_print/service/win/chro...
File cloud_print/service/win/chrome_launcher.cc (right):

https://codereview.chromium.org/685103004/diff/1/cloud_print/service/win/chro...
cloud_print/service/win/chrome_launcher.cc:202:
chrome_launcher_support::GetAnyChromePath(false);
On 2014/10/30 02:54:50, Matt Giuca wrote:
> Question for Vitaly: Is there a reason why cloud_print is not getting the SxS
> (canary) path for Canary builds? Is it not supported?
> 
> Maybe in the future this will make it easier to launch a Canary Chrome.

It's OK to try Canary first here and then fallback to regular one.

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

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
File chrome/installer/launcher_support/chrome_launcher_support.cc (right):

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
chrome/installer/launcher_support/chrome_launcher_support.cc:128: path =
GetChromePathForInstallationLevel(SYSTEM_LEVEL_INSTALLATION);
On 2014/10/30 16:46:17, Vitaly Buka wrote:
> Could you fall back here to non-canary if canary is missing?

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]

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
File chrome/installer/launcher_support/chrome_launcher_support.cc (right):

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
chrome/installer/launcher_support/chrome_launcher_support.cc:123: // non-canary
path.
On 2014/10/30 16:46:17, Vitaly Buka wrote:
> On 2014/10/30 03:59:22, jackhou1 wrote:
> > On 2014/10/30 02:54:50, Matt Giuca wrote:
> > > Question for Jack: Is there a reason you prioritize user-level Chrome in
> > > GetAnyChromeSxSPath (why is it inconsistent)?
> > 
> > Canary is installed at user level by default.
> 
> Yes, It's installed. Still, my guess, consistency makes sense here.
> If we start installing canary on system level, it will require no changes.

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.

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
chrome/installer/launcher_support/chrome_launcher_support.cc:128: path =
GetChromePathForInstallationLevel(SYSTEM_LEVEL_INSTALLATION);
On 2014/10/31 00:05:54, jackhou1 wrote:
> On 2014/10/30 16:46:17, Vitaly Buka wrote:
> > Could you fall back here to non-canary if canary is missing?
> 
> 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.

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 05:12:08]

Patchset #5 (id:80001) has been deleted

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

Hi guys, so sorry I haven't responded to this in months. Have been busy with a
launch and then lost my Windows machine !

PTAL.

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
File chrome/installer/launcher_support/chrome_launcher_support.cc (right):

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
chrome/installer/launcher_support/chrome_launcher_support.cc:123: // non-canary
path.
On 2014/10/31 03:29:00, grt wrote:
> On 2014/10/30 16:46:17, Vitaly Buka wrote:
> > On 2014/10/30 03:59:22, jackhou1 wrote:
> > > On 2014/10/30 02:54:50, Matt Giuca wrote:
> > > > Question for Jack: Is there a reason you prioritize user-level Chrome in
> > > > GetAnyChromeSxSPath (why is it inconsistent)?
> > > 
> > > Canary is installed at user level by default.
> > 
> > Yes, It's installed. Still, my guess, consistency makes sense here.
> > If we start installing canary on system level, it will require no changes.
> 
> 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.

Done.

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
chrome/installer/launcher_support/chrome_launcher_support.cc:128: path =
GetChromePathForInstallationLevel(SYSTEM_LEVEL_INSTALLATION);
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.)

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
File chrome/installer/launcher_support/chrome_launcher_support.h (right):

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
chrome/installer/launcher_support/chrome_launcher_support.h:28: // If
|is_canary| is specified, gets the path to the SxS (Canary) version of
On 2014/10/30 16:46:17, Vitaly Buka wrote:
> |is_canary| is true

Done.

https://codereview.chromium.org/685103004/diff/1/cloud_print/service/win/chro...
File cloud_print/service/win/chrome_launcher.cc (right):

https://codereview.chromium.org/685103004/diff/1/cloud_print/service/win/chro...
cloud_print/service/win/chrome_launcher.cc:202:
chrome_launcher_support::GetAnyChromePath(false);
Echoing my statements in chrome_launcher_support.cc, I don't know why you would
want a canary cloud_print to use regular Chrome and vice versa.

Certainly the non-canary cloud_print should not be using canary Chrome. Why
would a canary cloud_print want to fall back to regular Chrome? (If you have
canary cloud_print, then surely you have canary Chrome installed.)

[grt (UTC plus 2), 2015-01-09 17:45:29]

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
File chrome/installer/launcher_support/chrome_launcher_support.cc (right):

https://codereview.chromium.org/685103004/diff/1/chrome/installer/launcher_su...
chrome/installer/launcher_support/chrome_launcher_support.cc:128: path =
GetChromePathForInstallationLevel(SYSTEM_LEVEL_INSTALLATION);
On 2015/01/09 06:17:32, Matt Giuca wrote:
> 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.)

SGTM

https://codereview.chromium.org/685103004/diff/1/cloud_print/service/win/chro...
File cloud_print/service/win/chrome_launcher.cc (right):

https://codereview.chromium.org/685103004/diff/1/cloud_print/service/win/chro...
cloud_print/service/win/chrome_launcher.cc:202:
chrome_launcher_support::GetAnyChromePath(false);
On 2015/01/09 06:17:32, Matt Giuca wrote:
> Echoing my statements in chrome_launcher_support.cc, I don't know why you
would
> want a canary cloud_print to use regular Chrome and vice versa.
> 
> Certainly the non-canary cloud_print should not be using canary Chrome. Why
> would a canary cloud_print want to fall back to regular Chrome? (If you have
> canary cloud_print, then surely you have canary Chrome installed.)

Is the only path to installing canary cloud print via SxS Chrome? If the two are
intertwined, then yeah, launch canary from canary and regular from regular.

https://codereview.chromium.org/685103004/diff/120001/chrome/installer/launch...
File chrome/installer/launcher_support/chrome_launcher_support.cc (right):

https://codereview.chromium.org/685103004/diff/120001/chrome/installer/launch...
chrome/installer/launcher_support/chrome_launcher_support.cc:137: if (is_canary)
{
i think you could simplify this by making GetSetupExeForInstallationLevel handle
chrome_sxs and doing away with GetChromeSxSPathForInstallationLevel altogether,
leaving you with something like
  path = GetChromePath(SYSTEM_LEVEL_INSTALLATION, chrome_sxs);
  if (path.empty())
    path = GetChromePath(USER_LEVEL_INSTALLATION, chrome_sxs);
and changing GetChromePathForInstallationLevel to:
  return GetChromePath(level, false /* chrome_sxs /*);

https://codereview.chromium.org/685103004/diff/120001/chrome/installer/launch...
File chrome/installer/launcher_support/chrome_launcher_support.h (right):

https://codereview.chromium.org/685103004/diff/120001/chrome/installer/launch...
chrome/installer/launcher_support/chrome_launcher_support.h:29: base::FilePath
GetAnyChromePath(bool is_canary);
Now that I think about it, |is_canary| is a bad name for this. Canary is the
name of the update channel (stable, beta, dev, canary) and not the name of the
type of install (normal or SxS). The relevant names in chrome/installer/util are
things like InstallUtil::IsChromeSxSProcess and GoogleChromeSxsDistribution.
Please change is_canary to chrome_sxs or something like that here and elsewhere.

https://codereview.chromium.org/685103004/diff/120001/cloud_print/service/win...
File cloud_print/service/win/chrome_launcher.cc (right):

https://codereview.chromium.org/685103004/diff/120001/cloud_print/service/win...
cloud_print/service/win/chrome_launcher.cc:202:
chrome_launcher_support::GetAnyChromePath(false);
(false /* chrome_sxs */);
here and below

https://codereview.chromium.org/685103004/diff/120001/cloud_print/service/win...
File cloud_print/service/win/service_listener.cc (right):

https://codereview.chromium.org/685103004/diff/120001/cloud_print/service/win...
cloud_print/service/win/service_listener.cc:40:
chrome_launcher_support::GetAnyChromePath(false).value());
there was discussion of canary cloud print elsewhere in the review comments.
should this somehow differentiate a canary cloud print service from a
non-canary?

is special handling needed here in case GetAnyChromePath returns an empty path?

https://codereview.chromium.org/685103004/diff/120001/cloud_print/virtual_dri...
File cloud_print/virtual_driver/win/port_monitor/port_monitor.cc (right):

https://codereview.chromium.org/685103004/diff/120001/cloud_print/virtual_dri...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:304: return
ReadPathFromRegistry(HKEY_LOCAL_MACHINE, path_name);
are you aware that 64-bit builds will access a different key in the registry
compared to 32-bit builds? is this desired?

https://codereview.chromium.org/685103004/diff/120001/cloud_print/virtual_dri...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:311: return
chrome_launcher_support::GetAnyChromePath(false);
similar comment here

[Matt Giuca, 2015-01-12 06:54:26]

https://codereview.chromium.org/685103004/diff/1/cloud_print/service/win/chro...
File cloud_print/service/win/chrome_launcher.cc (right):

https://codereview.chromium.org/685103004/diff/1/cloud_print/service/win/chro...
cloud_print/service/win/chrome_launcher.cc:202:
chrome_launcher_support::GetAnyChromePath(false);
I don't know. Vitaly, can you answer this?

TBH I would prefer to keep the original functionality here (of launching the
regular Chrome only). This CL is a refactor, and should not be changing
functionality (especially since the author is completely unfamiliar with cloud
print). If indeed there is a bug here (canary cloud print only launches stable
Chrome), then that should have its own crbug and CL to fix.

https://codereview.chromium.org/685103004/diff/120001/chrome/installer/launch...
File chrome/installer/launcher_support/chrome_launcher_support.cc (right):

https://codereview.chromium.org/685103004/diff/120001/chrome/installer/launch...
chrome/installer/launcher_support/chrome_launcher_support.cc:137: if (is_canary)
{
I didn't quite follow your example, but I take the point... made
GetChromePathForInstallationLevel take a Boolean rather than have two separate
functions, and then this simplified the implementation of GetAnyChromePath to a
bare minimum.

https://codereview.chromium.org/685103004/diff/120001/chrome/installer/launch...
File chrome/installer/launcher_support/chrome_launcher_support.h (right):

https://codereview.chromium.org/685103004/diff/120001/chrome/installer/launch...
chrome/installer/launcher_support/chrome_launcher_support.h:29: base::FilePath
GetAnyChromePath(bool is_canary);
OK I wasn't aware of that distinction. Updated the names here and in app_shim.

https://codereview.chromium.org/685103004/diff/120001/cloud_print/service/win...
File cloud_print/service/win/chrome_launcher.cc (right):

https://codereview.chromium.org/685103004/diff/120001/cloud_print/service/win...
cloud_print/service/win/chrome_launcher.cc:202:
chrome_launcher_support::GetAnyChromePath(false);
On 2015/01/09 17:45:28, grt wrote:
> (false /* chrome_sxs */);
> here and below

Done.

https://codereview.chromium.org/685103004/diff/120001/cloud_print/service/win...
File cloud_print/service/win/service_listener.cc (right):

https://codereview.chromium.org/685103004/diff/120001/cloud_print/service/win...
cloud_print/service/win/service_listener.cc:40:
chrome_launcher_support::GetAnyChromePath(false).value());
On 2015/01/09 17:45:28, grt wrote:
> there was discussion of canary cloud print elsewhere in the review comments.
> should this somehow differentiate a canary cloud print service from a
> non-canary?
> 
> is special handling needed here in case GetAnyChromePath returns an empty
path?

Same as the discussion on chrome_launcher.cc: if there are bugs here, they
should be handled separately to this CL, which is purely a refactor.

https://codereview.chromium.org/685103004/diff/120001/cloud_print/virtual_dri...
File cloud_print/virtual_driver/win/port_monitor/port_monitor.cc (right):

https://codereview.chromium.org/685103004/diff/120001/cloud_print/virtual_dri...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:304: return
ReadPathFromRegistry(HKEY_LOCAL_MACHINE, path_name);
I have no idea... I'm not writing or modifying this code, just updating it to
match the new API (which should not change anything).

[Vitaly Buka (NO REVIEWS), 2015-01-12 08:24:43]

lgtm cloud_print/

https://codereview.chromium.org/685103004/diff/1/cloud_print/service/win/chro...
File cloud_print/service/win/chrome_launcher.cc (right):

https://codereview.chromium.org/685103004/diff/1/cloud_print/service/win/chro...
cloud_print/service/win/chrome_launcher.cc:202:
chrome_launcher_support::GetAnyChromePath(false);
sgtm.

There is no canary build of cloud print service and currently service always
uses non SxS Chrome. It would be nice to find a way to use SxS to testing or
troubleshooting, but you are right, it's a separate issue.

So lets keep just "false" here then.

On 2015/01/12 06:54:26, Matt Giuca wrote:
> I don't know. Vitaly, can you answer this?
> 
> TBH I would prefer to keep the original functionality here (of launching the
> regular Chrome only). This CL is a refactor, and should not be changing
> functionality (especially since the author is completely unfamiliar with cloud
> print). If indeed there is a bug here (canary cloud print only launches stable
> Chrome), then that should have its own crbug and CL to fix.

[grt (UTC plus 2), 2015-01-12 14:02:42]

lgtm w/ some comment nits. thanks.

https://codereview.chromium.org/685103004/diff/1/cloud_print/service/win/chro...
File cloud_print/service/win/chrome_launcher.cc (right):

https://codereview.chromium.org/685103004/diff/1/cloud_print/service/win/chro...
cloud_print/service/win/chrome_launcher.cc:202:
chrome_launcher_support::GetAnyChromePath(false);
On 2015/01/12 08:24:43, Vitaly Buka wrote:
> sgtm.
> 
> There is no canary build of cloud print service and currently service always
> uses non SxS Chrome. It would be nice to find a way to use SxS to testing or
> troubleshooting, but you are right, it's a separate issue.
> 
> So lets keep just "false" here then.
> 
> On 2015/01/12 06:54:26, Matt Giuca wrote:
> > I don't know. Vitaly, can you answer this?
> > 
> > TBH I would prefer to keep the original functionality here (of launching the
> > regular Chrome only). This CL is a refactor, and should not be changing
> > functionality (especially since the author is completely unfamiliar with
cloud
> > print). If indeed there is a bug here (canary cloud print only launches
stable
> > Chrome), then that should have its own crbug and CL to fix.

SGTM

https://codereview.chromium.org/685103004/diff/120001/chrome/installer/launch...
File chrome/installer/launcher_support/chrome_launcher_support.cc (right):

https://codereview.chromium.org/685103004/diff/120001/chrome/installer/launch...
chrome/installer/launcher_support/chrome_launcher_support.cc:137: if (is_canary)
{
On 2015/01/12 06:54:26, Matt Giuca wrote:
> I didn't quite follow your example, but I take the point... made
> GetChromePathForInstallationLevel take a Boolean rather than have two separate
> functions, and then this simplified the implementation of GetAnyChromePath to
a
> bare minimum.

Sorry if my proposal was confusing. I was suggesting a way that would leave
GetChromePathForInstallationLevel as-is so that its callers wouldn't need to
change. What you have is here is okay, though. I don't see a problem exposing
is_sxs there as well.

https://codereview.chromium.org/685103004/diff/140001/chrome/installer/launch...
File chrome/installer/launcher_support/chrome_launcher_support.h (right):

https://codereview.chromium.org/685103004/diff/140001/chrome/installer/launch...
chrome/installer/launcher_support/chrome_launcher_support.h:21: // also return
that of a single-install of Chrome if no multi-install exists.
add the note about |is_sxs| here as well

https://codereview.chromium.org/685103004/diff/140001/cloud_print/service/win...
File cloud_print/service/win/service_listener.cc (right):

https://codereview.chromium.org/685103004/diff/140001/cloud_print/service/win...
cloud_print/service/win/service_listener.cc:40:
chrome_launcher_support::GetAnyChromePath(false).value());
would you mind adding /* is_sxs */ here and in port_monitor.cc as well?

[Matt Giuca, 2015-01-13 08:22:00]

https://codereview.chromium.org/685103004/diff/120001/chrome/installer/launch...
File chrome/installer/launcher_support/chrome_launcher_support.cc (right):

https://codereview.chromium.org/685103004/diff/120001/chrome/installer/launch...
chrome/installer/launcher_support/chrome_launcher_support.cc:137: if (is_canary)
{
On 2015/01/12 14:02:41, grt wrote:
> On 2015/01/12 06:54:26, Matt Giuca wrote:
> > I didn't quite follow your example, but I take the point... made
> > GetChromePathForInstallationLevel take a Boolean rather than have two
separate
> > functions, and then this simplified the implementation of GetAnyChromePath
to
> a
> > bare minimum.
> 
> Sorry if my proposal was confusing. I was suggesting a way that would leave
> GetChromePathForInstallationLevel as-is so that its callers wouldn't need to
> change. What you have is here is okay, though. I don't see a problem exposing
> is_sxs there as well.

Acknowledged.

https://codereview.chromium.org/685103004/diff/140001/chrome/installer/launch...
File chrome/installer/launcher_support/chrome_launcher_support.h (right):

https://codereview.chromium.org/685103004/diff/140001/chrome/installer/launch...
chrome/installer/launcher_support/chrome_launcher_support.h:21: // also return
that of a single-install of Chrome if no multi-install exists.
On 2015/01/12 14:02:42, grt wrote:
> add the note about |is_sxs| here as well

Done.

https://codereview.chromium.org/685103004/diff/140001/cloud_print/service/win...
File cloud_print/service/win/service_listener.cc (right):

https://codereview.chromium.org/685103004/diff/140001/cloud_print/service/win...
cloud_print/service/win/service_listener.cc:40:
chrome_launcher_support::GetAnyChromePath(false).value());
On 2015/01/12 14:02:42, grt wrote:
> would you mind adding /* is_sxs */ here and in port_monitor.cc as well?

Done.

[Matt Giuca, 2015-01-13 08:25:53]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2015-01-13 08:26:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/685103004/180001

[commit-bot: I haz the power, 2015-01-13 08:48:48]

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

[commit-bot: I haz the power, 2015-01-13 08:48:48]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[grt (UTC plus 2), 2015-01-13 14:10:43]

https://codereview.chromium.org/685103004/diff/180001/chrome/app_installer/wi...
File chrome/app_installer/win/app_installer_main.cc (right):

https://codereview.chromium.org/685103004/diff/180001/chrome/app_installer/wi...
chrome/app_installer/win/app_installer_main.cc:23: #include
"chrome/installer/util/util_constants.h"
looks like this is missing:
#include "chrome/installer/launcher_support/chrome_launcher_support.h"

[Matt Giuca, 2015-01-14 00:26:58]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2015-01-14 00:28:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/685103004/200001

[commit-bot: I haz the power, 2015-01-14 01:20:03]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2015-01-14 01:21:51]

Patchset 10 (id:??) landed as
https://crrev.com/e1b07409f19d84853d4730e3ab5b862fd65838d0
Cr-Commit-Position: refs/heads/master@{#311384}