Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
200713003:
Add GetAnyChromeSxSPath. (Closed)
|
Created:
6 years, 9 months ago by jackhou1 Modified:
6 years, 9 months ago CC:
chromium-reviews, grt+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd GetAnyChromeSxSPath.
BUG=341353
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257937
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add GetAnyChromeSxSPath instead of checking command line. #
Total comments: 4
Patch Set 3 : Address comments #
Messages
Total messages: 20 (0 generated)
scottbyer, cloud_print is the main user of GetAnyChromePath, could you please check that this has no negative impact? grt, could you please review for OWNERS in chrome/installer?
lgtm
https://codereview.chromium.org/200713003/diff/1/chrome/installer/launcher_su... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://codereview.chromium.org/200713003/diff/1/chrome/installer/launcher_su... chrome/installer/launcher_support/chrome_launcher_support.cc:155: if (CommandLine::ForCurrentProcess()->HasSwitch(kChromeSxS)) from what context do you expect to call this? do callers necessarily have --chrome-sxs on their command line?
On 2014/03/17 21:26:51, grt wrote: > https://codereview.chromium.org/200713003/diff/1/chrome/installer/launcher_su... > File chrome/installer/launcher_support/chrome_launcher_support.cc (right): > > https://codereview.chromium.org/200713003/diff/1/chrome/installer/launcher_su... > chrome/installer/launcher_support/chrome_launcher_support.cc:155: if > (CommandLine::ForCurrentProcess()->HasSwitch(kChromeSxS)) > from what context do you expect to call this? do callers necessarily have > --chrome-sxs on their command line? GetAnyChromePath is used in app_installer. I pass --chrome-sxs directly to make it find and download Chrome Canary. It's currently used for testing, We could alternatively add a new method (GetAnyChromeSxSPath?) and check --chrome-sxs in app_installer_main.cc. WDYT?
On 2014/03/17 23:11:08, jackhou1 wrote: > On 2014/03/17 21:26:51, grt wrote: > > > https://codereview.chromium.org/200713003/diff/1/chrome/installer/launcher_su... > > File chrome/installer/launcher_support/chrome_launcher_support.cc (right): > > > > > https://codereview.chromium.org/200713003/diff/1/chrome/installer/launcher_su... > > chrome/installer/launcher_support/chrome_launcher_support.cc:155: if > > (CommandLine::ForCurrentProcess()->HasSwitch(kChromeSxS)) > > from what context do you expect to call this? do callers necessarily have > > --chrome-sxs on their command line? > > GetAnyChromePath is used in app_installer. I pass --chrome-sxs directly to make > it find and download Chrome Canary. It's currently used for testing, > > We could alternatively add a new method (GetAnyChromeSxSPath?) and check > --chrome-sxs in app_installer_main.cc. WDYT? Actually, given the pattern in this file with GetAnyAppHostPath, I think this is the better option, I'll change it and ping this CL.
On 2014/03/17 23:11:53, jackhou1 wrote: > On 2014/03/17 23:11:08, jackhou1 wrote: > > On 2014/03/17 21:26:51, grt wrote: > > > > > > https://codereview.chromium.org/200713003/diff/1/chrome/installer/launcher_su... > > > File chrome/installer/launcher_support/chrome_launcher_support.cc (right): > > > > > > > > > https://codereview.chromium.org/200713003/diff/1/chrome/installer/launcher_su... > > > chrome/installer/launcher_support/chrome_launcher_support.cc:155: if > > > (CommandLine::ForCurrentProcess()->HasSwitch(kChromeSxS)) > > > from what context do you expect to call this? do callers necessarily have > > > --chrome-sxs on their command line? > > > > GetAnyChromePath is used in app_installer. I pass --chrome-sxs directly to > make > > it find and download Chrome Canary. It's currently used for testing, > > > > We could alternatively add a new method (GetAnyChromeSxSPath?) and check > > --chrome-sxs in app_installer_main.cc. WDYT? > > Actually, given the pattern in this file with GetAnyAppHostPath, I think this is > the better option, I'll change it and ping this CL. PTAL
i like this much better. thanks. https://codereview.chromium.org/200713003/diff/20001/chrome/installer/launche... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://codereview.chromium.org/200713003/diff/20001/chrome/installer/launche... chrome/installer/launcher_support/chrome_launcher_support.cc:37: const wchar_t kSxSBrowserAppGuid[] = L"{4ea16ac7-fd5a-47c3-875b-dbf4a2008c20}"; nit: document this as "Coped from google_chrome_sxs_distribution.cc." since that's the authoritative definition of the value. https://codereview.chromium.org/200713003/diff/20001/chrome/installer/launche... File chrome/installer/launcher_support/chrome_launcher_support.h (right): https://codereview.chromium.org/200713003/diff/20001/chrome/installer/launche... chrome/installer/launcher_support/chrome_launcher_support.h:55: // Returns the path to an installed SxS chrome.exe, or an empty path. Prefers a currently, sxs is only installed at user level, so i suggest you switch this to check user level before system level. may as well leave the probe into system-level in place in case we support that in the future.
https://codereview.chromium.org/200713003/diff/20001/chrome/installer/launche... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://codereview.chromium.org/200713003/diff/20001/chrome/installer/launche... chrome/installer/launcher_support/chrome_launcher_support.cc:37: const wchar_t kSxSBrowserAppGuid[] = L"{4ea16ac7-fd5a-47c3-875b-dbf4a2008c20}"; On 2014/03/18 13:51:06, grt wrote: > nit: document this as "Coped from google_chrome_sxs_distribution.cc." since > that's the authoritative definition of the value. Done. https://codereview.chromium.org/200713003/diff/20001/chrome/installer/launche... File chrome/installer/launcher_support/chrome_launcher_support.h (right): https://codereview.chromium.org/200713003/diff/20001/chrome/installer/launche... chrome/installer/launcher_support/chrome_launcher_support.h:55: // Returns the path to an installed SxS chrome.exe, or an empty path. Prefers a On 2014/03/18 13:51:06, grt wrote: > currently, sxs is only installed at user level, so i suggest you switch this to > check user level before system level. may as well leave the probe into > system-level in place in case we support that in the future. Done.
lgtm
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/200713003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_clang_dbg
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/200713003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_clang_dbg
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/200713003/40001
Message was sent while issue was closed.
Change committed as 257937 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
