Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10546149:
Factor out logic to find chrome.exe from Omaha client state. (Closed)
|
Created:
8 years, 6 months ago by erikwright (departed) Modified:
8 years, 6 months ago CC:
chromium-reviews, grt+watch_chromium.org, pam+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFactor out logic to find chrome.exe from Omaha client state.
Besides these two clients, will also be used by the Chrome Application Host stub.
R=robertshield,abodenha
CC=grt
BUG=None
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=143423
Patch Set 1 #
Total comments: 35
Patch Set 2 : Respond to codereview comments. #Patch Set 3 : Move launcher_support under chrome/installer. #
Total comments: 4
Patch Set 4 : 'Indentation.' #Patch Set 5 : Remove fallback to Program Files. #Patch Set 6 : Use chrome from current dir in development. #
Messages
Total messages: 26 (0 generated)
LGTM vitalybuka@ should probably also take a look since he's been working on this area of the code most recently.
Vitaly, Albert suggested roping you in on this one.
http://codereview.chromium.org/10546149/diff/1/chrome/chrome.gyp File chrome/chrome.gyp (right): http://codereview.chromium.org/10546149/diff/1/chrome/chrome.gyp#newcode1076 chrome/chrome.gyp:1076: 'tools/launcher_support/chrome_launcher_support.cc', tools/ doesn't feel like the right place for this to me. isn't that area more for build tooling and the like? http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... File chrome/tools/launcher_support/chrome_launcher_support.cc (right): http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... chrome/tools/launcher_support/chrome_launcher_support.cc:35: FilePath chrome_path = GetChromePathForInstallationLevel( i've been told repeatedly that chromium prefers FilePath chrome_path(...) as per gotw#1. http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... chrome/tools/launcher_support/chrome_launcher_support.cc:43: chrome_path.Append(kGoogleChromeExePath); chrome_path = chrome_path.Append(kGoogleChromeExePath); http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... chrome/tools/launcher_support/chrome_launcher_support.cc:50: FilePath GetChromePathForInstallationLevel(InstallationLevel level) { will this ever be used by code that wishes to find a canary install? if so, canary is user-level only and has a different guid. http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... File chrome/tools/launcher_support/chrome_launcher_support.h (right): http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... chrome/tools/launcher_support/chrome_launcher_support.h:7: #pragma once http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... chrome/tools/launcher_support/chrome_launcher_support.h:8: class FilePath; since FilePath is used directly rather than just a pointer or reference to it, i think you must include base/file_path.h rather than forward decl'ing. http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... chrome/tools/launcher_support/chrome_launcher_support.h:10: namespace chrome_launcher_support { please document the declarations in this file. http://codereview.chromium.org/10546149/diff/1/cloud_print/service/service.gyp File cloud_print/service/service.gyp (right): http://codereview.chromium.org/10546149/diff/1/cloud_print/service/service.gy... cloud_print/service/service.gyp:35: }, ], unless you copied "}, ]," from somewhere, please check to see what's the most common. i've only seen "}],", but i haven't made an actual search for occurrences.
There is something in cloud_print\virtual_driver\win\port_monitor\port_monitor.cc but it's also use reg key for debug. Albert, do you mind if we switch to shared function? Also probably DEPS update required for this change. Checking DEPS on windows is broken, it probably would return SUCCESS there and fail on Linux. On 2012/06/13 17:44:02, erikwright wrote: > Vitaly, Albert suggested roping you in on this one.
http://codereview.chromium.org/10546149/diff/1/chrome/installer/gcapi/gcapi.cc File chrome/installer/gcapi/gcapi.cc (right): http://codereview.chromium.org/10546149/diff/1/chrome/installer/gcapi/gcapi.c... chrome/installer/gcapi/gcapi.cc:34: #include "chrome/tools/launcher_support/chrome_launcher_support.h" invalid order http://codereview.chromium.org/10546149/diff/1/cloud_print/service/service.gyp File cloud_print/service/service.gyp (right): http://codereview.chromium.org/10546149/diff/1/cloud_print/service/service.gy... cloud_print/service/service.gyp:31: ['OS=="win"', { What if you remove condition here? http://codereview.chromium.org/10546149/diff/1/cloud_print/service/win/cloud_... File cloud_print/service/win/cloud_print_service.cc (right): http://codereview.chromium.org/10546149/diff/1/cloud_print/service/win/cloud_... cloud_print/service/win/cloud_print_service.cc:190: return hr; you can put "using namespace chrome_launcher_support;"
nits http://codereview.chromium.org/10546149/diff/1/chrome/installer/gcapi/gcapi.cc File chrome/installer/gcapi/gcapi.cc (right): http://codereview.chromium.org/10546149/diff/1/chrome/installer/gcapi/gcapi.c... chrome/installer/gcapi/gcapi.cc:34: #include "chrome/tools/launcher_support/chrome_launcher_support.h" move this include down to line 39 http://codereview.chromium.org/10546149/diff/1/chrome/installer/gcapi/gcapi.c... chrome/installer/gcapi/gcapi.cc:377: FilePath chrome_exe_path = chrome_launcher_support::GetAnyChromePath(); prefer constructor syntax http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... File chrome/tools/launcher_support/chrome_launcher_support.cc (right): http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... chrome/tools/launcher_support/chrome_launcher_support.cc:18: // Consolidate them somehow. There are similarly worded TODO(robertshield)s elsewhere in the code base. Welcome to the club :-) http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... File chrome/tools/launcher_support/chrome_launcher_support.h (right): http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... chrome/tools/launcher_support/chrome_launcher_support.h:17: FilePath GetAnyChromePath(); could you add a comment to these methods (mainly the first one, which is somewhat confusingly named).
On Wed, Jun 13, 2012 at 11:01 AM, <vitalybuka@chromium.org> wrote: > There is something in > cloud_print\virtual_driver\**win\port_monitor\port_monitor.**cc > but it's also use reg key for debug. > > Albert, do you mind if we switch to shared function? > > I would prefer it. > Also probably DEPS update required for this change. Checking DEPS on > windows is > broken, it probably would return SUCCESS there and fail on Linux. > > > > On 2012/06/13 17:44:02, erikwright wrote: > >> Vitaly, Albert suggested roping you in on this one. >> > > > http://codereview.chromium.**org/10546149/<http://codereview.chromium.org/105... > -- Albert Bodenhamer | Software Engineer | abodenha@google.com | 650-253-7313
http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... File chrome/tools/launcher_support/chrome_launcher_support.cc (right): http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... chrome/tools/launcher_support/chrome_launcher_support.cc:18: // Consolidate them somehow. Please keep in mind that we'd like to avoid depend on large Chrome targets. On 2012/06/13 18:01:53, robertshield wrote: > There are similarly worded TODO(robertshield)s elsewhere in the code base. > Welcome to the club :-) http://codereview.chromium.org/10546149/diff/1/cloud_print/service/service.gyp File cloud_print/service/service.gyp (right): http://codereview.chromium.org/10546149/diff/1/cloud_print/service/service.gy... cloud_print/service/service.gyp:35: }, ], Please one per line here. On 2012/06/13 18:00:10, grt wrote: > unless you copied "}, ]," from somewhere, please check to see what's the most > common. i've only seen "}],", but i haven't made an actual search for > occurrences.
Patch set 2 responds to all comments (except the DEPS one). Patch set 3 only does a rename (and the DEPS one). Hopefully that is easy on the eyes. http://codereview.chromium.org/10546149/diff/1/chrome/chrome.gyp File chrome/chrome.gyp (right): http://codereview.chromium.org/10546149/diff/1/chrome/chrome.gyp#newcode1076 chrome/chrome.gyp:1076: 'tools/launcher_support/chrome_launcher_support.cc', On 2012/06/13 18:00:10, grt wrote: > tools/ doesn't feel like the right place for this to me. isn't that area more > for build tooling and the like? Done. http://codereview.chromium.org/10546149/diff/1/chrome/installer/gcapi/gcapi.cc File chrome/installer/gcapi/gcapi.cc (right): http://codereview.chromium.org/10546149/diff/1/chrome/installer/gcapi/gcapi.c... chrome/installer/gcapi/gcapi.cc:34: #include "chrome/tools/launcher_support/chrome_launcher_support.h" On 2012/06/13 18:01:36, Vitaly Buka wrote: > invalid order Done. http://codereview.chromium.org/10546149/diff/1/chrome/installer/gcapi/gcapi.c... chrome/installer/gcapi/gcapi.cc:34: #include "chrome/tools/launcher_support/chrome_launcher_support.h" On 2012/06/13 18:01:53, robertshield wrote: > move this include down to line 39 Done. http://codereview.chromium.org/10546149/diff/1/chrome/installer/gcapi/gcapi.c... chrome/installer/gcapi/gcapi.cc:377: FilePath chrome_exe_path = chrome_launcher_support::GetAnyChromePath(); On 2012/06/13 18:01:53, robertshield wrote: > prefer constructor syntax Done. http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... File chrome/tools/launcher_support/chrome_launcher_support.cc (right): http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... chrome/tools/launcher_support/chrome_launcher_support.cc:18: // Consolidate them somehow. On 2012/06/13 18:08:19, Vitaly Buka wrote: > Please keep in mind that we'd like to avoid depend on large Chrome targets. > On 2012/06/13 18:01:53, robertshield wrote: > > There are similarly worded TODO(robertshield)s elsewhere in the code base. > > Welcome to the club :-) > Absolutely. My client of this library is under a similar constraint. http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... chrome/tools/launcher_support/chrome_launcher_support.cc:35: FilePath chrome_path = GetChromePathForInstallationLevel( On 2012/06/13 18:00:10, grt wrote: > i've been told repeatedly that chromium prefers FilePath chrome_path(...) as per > gotw#1. Done. http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... chrome/tools/launcher_support/chrome_launcher_support.cc:43: chrome_path.Append(kGoogleChromeExePath); On 2012/06/13 18:00:10, grt wrote: > chrome_path = chrome_path.Append(kGoogleChromeExePath); Ouch! Perhaps that's a sign that I can just remove this case, since this is actually just moved from Cloud Print. Done. http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... chrome/tools/launcher_support/chrome_launcher_support.cc:50: FilePath GetChromePathForInstallationLevel(InstallationLevel level) { On 2012/06/13 18:00:10, grt wrote: > will this ever be used by code that wishes to find a canary install? if so, > canary is user-level only and has a different guid. I would say probably not. But I'm not sure how to enforce it or document it in a way that would catch such a mistake if it were made :( http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... File chrome/tools/launcher_support/chrome_launcher_support.h (right): http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... chrome/tools/launcher_support/chrome_launcher_support.h:7: On 2012/06/13 18:00:10, grt wrote: > #pragma once Done. http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... chrome/tools/launcher_support/chrome_launcher_support.h:8: class FilePath; On 2012/06/13 18:00:10, grt wrote: > since FilePath is used directly rather than just a pointer or reference to it, i > think you must include base/file_path.h rather than forward decl'ing. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Header... "How can we use a class Foo in a header file without access to its definition? ... We can declare (but not define) functions with arguments, and/or return values, of type Foo." http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... chrome/tools/launcher_support/chrome_launcher_support.h:10: namespace chrome_launcher_support { On 2012/06/13 18:00:10, grt wrote: > please document the declarations in this file. Done. http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... chrome/tools/launcher_support/chrome_launcher_support.h:17: FilePath GetAnyChromePath(); On 2012/06/13 18:01:53, robertshield wrote: > could you add a comment to these methods (mainly the first one, which is > somewhat confusingly named). Done. http://codereview.chromium.org/10546149/diff/1/cloud_print/service/service.gyp File cloud_print/service/service.gyp (right): http://codereview.chromium.org/10546149/diff/1/cloud_print/service/service.gy... cloud_print/service/service.gyp:31: ['OS=="win"', { On 2012/06/13 18:01:36, Vitaly Buka wrote: > What if you remove condition here? launcher_support is only built/meaningful on Windows. So cloud_print_service_lib would fail. Another option would be to make launcher_support be an empty lib on other platforms, but I think that's even uglier? http://codereview.chromium.org/10546149/diff/1/cloud_print/service/service.gy... cloud_print/service/service.gyp:35: }, ], On 2012/06/13 18:00:10, grt wrote: > unless you copied "}, ]," from somewhere, please check to see what's the most > common. i've only seen "}],", but i haven't made an actual search for > occurrences. Done. http://codereview.chromium.org/10546149/diff/1/cloud_print/service/service.gy... cloud_print/service/service.gyp:35: }, ], On 2012/06/13 18:08:19, Vitaly Buka wrote: > Please one per line here. > > On 2012/06/13 18:00:10, grt wrote: > > unless you copied "}, ]," from somewhere, please check to see what's the most > > common. i've only seen "}],", but i haven't made an actual search for > > occurrences. > Did you mean only one comma or only one close-bracket? 7/7 'conditions' blocks that I checked terminate with '}],'. http://codereview.chromium.org/10546149/diff/1/cloud_print/service/win/cloud_... File cloud_print/service/win/cloud_print_service.cc (right): http://codereview.chromium.org/10546149/diff/1/cloud_print/service/win/cloud_... cloud_print/service/win/cloud_print_service.cc:190: return hr; On 2012/06/13 18:01:36, Vitaly Buka wrote: > you can put > "using namespace chrome_launcher_support;" Done.
lgtm http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... File chrome/tools/launcher_support/chrome_launcher_support.cc (right): http://codereview.chromium.org/10546149/diff/1/chrome/tools/launcher_support/... chrome/tools/launcher_support/chrome_launcher_support.cc:43: chrome_path.Append(kGoogleChromeExePath); Yep. My mistake. But I still believe it might be useful. On 2012/06/13 19:43:43, erikwright wrote: > On 2012/06/13 18:00:10, grt wrote: > > chrome_path = chrome_path.Append(kGoogleChromeExePath); > > Ouch! Perhaps that's a sign that I can just remove this case, since this is > actually just moved from Cloud Print. > > Done. http://codereview.chromium.org/10546149/diff/1/cloud_print/service/service.gyp File cloud_print/service/service.gyp (right): http://codereview.chromium.org/10546149/diff/1/cloud_print/service/service.gy... cloud_print/service/service.gyp:35: }, ], Never mind. It's good now. On 2012/06/13 19:43:43, erikwright wrote: > On 2012/06/13 18:08:19, Vitaly Buka wrote: > > Please one per line here. > > > > On 2012/06/13 18:00:10, grt wrote: > > > unless you copied "}, ]," from somewhere, please check to see what's the > most > > > common. i've only seen "}],", but i haven't made an actual search for > > > occurrences. > > > > Did you mean only one comma or only one close-bracket? > 7/7 'conditions' blocks that I checked terminate with '}],'.
lgtm w/ a nit, although i'm not certain about the various gyp settings. can any other listed reviewers confirm the correctness of chrome_installer.gypi? http://codereview.chromium.org/10546149/diff/9002/chrome/chrome_installer.gypi File chrome/chrome_installer.gypi (right): http://codereview.chromium.org/10546149/diff/9002/chrome/chrome_installer.gyp... chrome/chrome_installer.gypi:178: '..', nit: de-indent two spaces here and the line below (i think)
lgtm http://codereview.chromium.org/10546149/diff/9002/chrome/chrome_installer.gypi File chrome/chrome_installer.gypi (right): http://codereview.chromium.org/10546149/diff/9002/chrome/chrome_installer.gyp... chrome/chrome_installer.gypi:178: '..', On 2012/06/13 21:15:51, grt wrote: > nit: de-indent two spaces here and the line below (i think) line 178 too?
misread. never mind :-) > line 178 too?
lgtm, question for Albert / Vitaly below re. the %PROGRAMFILES% fallback. http://codereview.chromium.org/10546149/diff/9002/chrome/installer/launcher_s... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): http://codereview.chromium.org/10546149/diff/9002/chrome/installer/launcher_s... chrome/installer/launcher_support/chrome_launcher_support.cc:42: if (PathService::Get(base::DIR_PROGRAM_FILES, &chrome_path)) { For any of the cloud print folk on this thread, this fallback to a non-Omaha-registered Chrome in %PROGRAMFILES% seems to have come from the cloud print launcher. Any idea in which circumstance it is likely to be used?
http://codereview.chromium.org/10546149/diff/9002/chrome/installer/launcher_s... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): http://codereview.chromium.org/10546149/diff/9002/chrome/installer/launcher_s... chrome/installer/launcher_support/chrome_launcher_support.cc:42: if (PathService::Get(base::DIR_PROGRAM_FILES, &chrome_path)) { It's just in case. I were convinced by reviewer that maybe some users will need this. Personally I used this for debuging. I have no strong opinion on this case. On 2012/06/13 21:42:21, robertshield wrote: > For any of the cloud print folk on this thread, this fallback to a > non-Omaha-registered Chrome in %PROGRAMFILES% seems to have come from the cloud > print launcher. Any idea in which circumstance it is likely to be used?
On Wed, Jun 13, 2012 at 2:52 PM, <vitalybuka@chromium.org> wrote: > > http://codereview.chromium.**org/10546149/diff/9002/chrome/** > installer/launcher_support/**chrome_launcher_support.cc<http://codereview.chromium.org/10546149/diff/9002/chrome/installer/launcher_support/chrome_launcher_support.cc> > File chrome/installer/launcher_**support/chrome_launcher_**support.cc > (right): > > http://codereview.chromium.**org/10546149/diff/9002/chrome/** > installer/launcher_support/**chrome_launcher_support.cc#**newcode42<http://codereview.chromium.org/10546149/diff/9002/chrome/installer/launcher_support/chrome_launcher_support.cc#newcode42> > chrome/installer/launcher_**support/chrome_launcher_**support.cc:42: if > (PathService::Get(base::DIR_**PROGRAM_FILES, &chrome_path)) { > It's just in case. I were convinced by reviewer that maybe some users > will need this. Personally I used this for debuging. > > I have no strong opinion on this case. > > It stems from advice I was given on chromium-dev when I was first working on the print driver that we should fall back to searching program files if all else failed. There weren't specific cases we were addressing. It's just a safety net. > On 2012/06/13 21:42:21, robertshield wrote: > >> For any of the cloud print folk on this thread, this fallback to a >> non-Omaha-registered Chrome in %PROGRAMFILES% seems to have come from >> > the cloud > >> print launcher. Any idea in which circumstance it is likely to be >> > used? > > http://codereview.chromium.**org/10546149/<http://codereview.chromium.org/105... > -- Albert Bodenhamer | Software Engineer | abodenha@google.com | 650-253-7313
Also omaha related code was copied from gcapi.cc, and it does not look like it official way to get chrome. I believe even without UninstallString chrome should work fine. So if we assume that this approach is supported way to get chrome.exe I have less motivation to keep base::DIR_PROGRAM_FILES approach On 2012/06/13 21:52:58, Vitaly Buka wrote: > http://codereview.chromium.org/10546149/diff/9002/chrome/installer/launcher_s... > File chrome/installer/launcher_support/chrome_launcher_support.cc (right): > > http://codereview.chromium.org/10546149/diff/9002/chrome/installer/launcher_s... > chrome/installer/launcher_support/chrome_launcher_support.cc:42: if > (PathService::Get(base::DIR_PROGRAM_FILES, &chrome_path)) { > It's just in case. I were convinced by reviewer that maybe some users will need > this. Personally I used this for debuging. > > I have no strong opinion on this case. > > On 2012/06/13 21:42:21, robertshield wrote: > > For any of the cloud print folk on this thread, this fallback to a > > non-Omaha-registered Chrome in %PROGRAMFILES% seems to have come from the > cloud > > print launcher. Any idea in which circumstance it is likely to be used?
On 2012/06/13 21:59:27, Vitaly Buka wrote: > Also omaha related code was copied from gcapi.cc, and it does not look like it > official way to get chrome. I believe even without UninstallString chrome should > work fine. > So if we assume that this approach is supported way to get chrome.exe I have > less motivation to keep base::DIR_PROGRAM_FILES approach > > On 2012/06/13 21:52:58, Vitaly Buka wrote: > > > http://codereview.chromium.org/10546149/diff/9002/chrome/installer/launcher_s... > > File chrome/installer/launcher_support/chrome_launcher_support.cc (right): > > > > > http://codereview.chromium.org/10546149/diff/9002/chrome/installer/launcher_s... > > chrome/installer/launcher_support/chrome_launcher_support.cc:42: if > > (PathService::Get(base::DIR_PROGRAM_FILES, &chrome_path)) { > > It's just in case. I were convinced by reviewer that maybe some users will > need > > this. Personally I used this for debuging. > > > > I have no strong opinion on this case. > > > > On 2012/06/13 21:42:21, robertshield wrote: > > > For any of the cloud print folk on this thread, this fallback to a > > > non-Omaha-registered Chrome in %PROGRAMFILES% seems to have come from the > > cloud > > > print launcher. Any idea in which circumstance it is likely to be used? I removed the Program Files support in my final patch. I can add it back if I have misunderstood. Vitaly, Robert, for development, in the bit that I'm building based on this I'm also looking in the same directory as my own executable. My executable runs from build/Debug, for example, so it will find chrome.exe there. I look there before looking for an installed Chrome. Would that also be appropriate for Cloud Print and GCAPI? Should I pull that logic into chrome_launcher_support so that it will look first in the same directory as the current executable and then for an installed Chrome?
On 2012/06/14 02:25:10, erikwright wrote: > On 2012/06/13 21:59:27, Vitaly Buka wrote: > > Also omaha related code was copied from gcapi.cc, and it does not look like it > > official way to get chrome. I believe even without UninstallString chrome > should > > work fine. > > So if we assume that this approach is supported way to get chrome.exe I have > > less motivation to keep base::DIR_PROGRAM_FILES approach > > > > On 2012/06/13 21:52:58, Vitaly Buka wrote: > > > > > > http://codereview.chromium.org/10546149/diff/9002/chrome/installer/launcher_s... > > > File chrome/installer/launcher_support/chrome_launcher_support.cc (right): > > > > > > > > > http://codereview.chromium.org/10546149/diff/9002/chrome/installer/launcher_s... > > > chrome/installer/launcher_support/chrome_launcher_support.cc:42: if > > > (PathService::Get(base::DIR_PROGRAM_FILES, &chrome_path)) { > > > It's just in case. I were convinced by reviewer that maybe some users will > > need > > > this. Personally I used this for debuging. > > > > > > I have no strong opinion on this case. > > > > > > On 2012/06/13 21:42:21, robertshield wrote: > > > > For any of the cloud print folk on this thread, this fallback to a > > > > non-Omaha-registered Chrome in %PROGRAMFILES% seems to have come from the > > > cloud > > > > print launcher. Any idea in which circumstance it is likely to be used? > > I removed the Program Files support in my final patch. I can add it back if I > have misunderstood. > > Vitaly, Robert, for development, in the bit that I'm building based on this I'm > also looking in the same directory as my own executable. My executable runs from > build/Debug, for example, so it will find chrome.exe there. I look there before > looking for an installed Chrome. > > Would that also be appropriate for Cloud Print and GCAPI? GCAPI is a special case grab-bag of assorted helper functions that gets integrated into third party products and whose behaviour I am loathe to touch unless needed. I'd prefer to minimize functional differences to GCAPI if at all possible. Should I pull that > logic into chrome_launcher_support so that it will look first in the same > directory as the current executable and then for an installed Chrome?
> Vitaly, Robert, for development, in the bit that I'm building based on this I'm > also looking in the same directory as my own executable. My executable runs from > build/Debug, for example, so it will find chrome.exe there. I look there before > looking for an installed Chrome. > Erik, what are you actually doing? How are you going to distribute and update your binary?
On 2012/06/14 02:25:10, erikwright wrote: > Vitaly, Robert, for development, in the bit that I'm building based on this I'm > also looking in the same directory as my own executable. My executable runs from > build/Debug, for example, so it will find chrome.exe there. I look there before > looking for an installed Chrome. I'm neither Vitaly nor Robert, so feel free to ignore this comment. This sounds like optimizing for the developer rather than for the user. Is there a way to make development easy (running the chrome.exe in build/Debug) without making every user's installation make an unnecessary check?
Anyone have any objections to this final revision? In non-official builds, look for chrome.exe in the same dir as the current executable. For development, this is very convenient. I manually tested this. It found my build/Debug/chrome.exe, and when I deleted that it found the installed chrome.exe.
lgtm On 2012/06/20 16:15:25, erikwright wrote: > Anyone have any objections to this final revision? > > In non-official builds, look for chrome.exe in the same dir as the current > executable. For development, this is very convenient. > > I manually tested this. It found my build/Debug/chrome.exe, and when I deleted > that it found the installed chrome.exe.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/10546149/19003
Change committed as 143423 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
