Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12094053:
[win] Add utility function to find out if chrome is installed. (Closed)
|
Created:
7 years, 10 months ago by benwells Modified:
7 years, 10 months ago CC:
chromium-reviews, grt+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[win] Add utility function to find out if chrome is installed.
The app launcher can be used with or without chrome installed. This function
lets the app launcher find out if chrome is installed.
BUG=159733
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180329
Patch Set 1 #Patch Set 2 : Use Clients key #Patch Set 3 : Tidy up #
Total comments: 14
Patch Set 4 : comments #
Total comments: 2
Patch Set 5 : Nit #
Total comments: 2
Messages
Total messages: 21 (0 generated)
erikwright: this is as we discussed a week or so ago. Note when I uninstall chrome dev channel, it leaves its state behind, so this doesn't actually work. Is there some state key I can check to see if chrome is still installed?
On 2013/01/30 08:06:35, benwells wrote: > erikwright: this is as we discussed a week or so ago. > > Note when I uninstall chrome dev channel, it leaves its state behind, so this > doesn't actually work. Is there some state key I can check to see if chrome is > still installed? See ProductState::Initialize in chrome/installer/util/installation_state.cc. Basically, look in the Clients (not ClientState) key under HKROOT/Software/Google/Update for google_update::kRegVersionField. The product is installed IFF the value is defined. I won't bother reviewing this for now as I think you'll need to rework it.
On 2013/01/30 14:53:48, erikwright wrote: > On 2013/01/30 08:06:35, benwells wrote: > > erikwright: this is as we discussed a week or so ago. > > > > Note when I uninstall chrome dev channel, it leaves its state behind, so this > > doesn't actually work. Is there some state key I can check to see if chrome is > > still installed? > > See ProductState::Initialize in chrome/installer/util/installation_state.cc. > > Basically, look in the Clients (not ClientState) key under > HKROOT/Software/Google/Update for google_update::kRegVersionField. The product > is installed IFF the value is defined. > > I won't bother reviewing this for now as I think you'll need to rework it. Hehe, I looked in Clients but was looking in HKCU not HKLM. Will have a new patch up soon....
Using clients key...
LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benwells@chromium.org/12094053/2003
Presubmit check for 12094053-2003 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/installer
+gab for owners review
This whole file is full of "#ifndef OFFICIAL_BUILD"s; not a fan, is this only to differentiate between developer builds and installed chrome? OFFICIAL_BUILD shouldn't be a basis for that imo... https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... chrome/installer/launcher_support/chrome_launcher_support.cc:89: KEY_QUERY_VALUE | KEY_WOW64_32KEY) == ERROR_SUCCESS; Will this stop working if Google Update becomes 64-bit?! https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... chrome/installer/launcher_support/chrome_launcher_support.cc:203: return true; We don't usually special case the official build (as that only calls for untested cases in developer builds). Is this to always return true in developer builds? If so, I would hope there is a better way. https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... File chrome/installer/launcher_support/chrome_launcher_support.h (right): https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... chrome/installer/launcher_support/chrome_launcher_support.h:57: bool IsChromeBrowserPresent(); Can you not use InstallerState::FindProduct() from installer_util here?
https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... chrome/installer/launcher_support/chrome_launcher_support.cc:88: return reg_key.Open(root_key, subkey.c_str(), the product is only installed if the Clients key has a "pv" value, so change this to: return reg_key.Open(...) == ERROR_SUCCESS && reg_key.HasValue("pv"); please update the comment at the top accordingly (and use a proper constant rather than using the string literal here). https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... chrome/installer/launcher_support/chrome_launcher_support.cc:89: KEY_QUERY_VALUE | KEY_WOW64_32KEY) == ERROR_SUCCESS; On 2013/01/31 15:09:45, gab wrote: > Will this stop working if Google Update becomes 64-bit?! Google Update only writes into the 32-bit region. KEY_WOW64_32KEY is needed since launcher support is built in both 32 and 64-bit modes. We're going to have to make broader changes when the rest of Chrome goes 64-bit.
https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... chrome/installer/launcher_support/chrome_launcher_support.cc:88: return reg_key.Open(root_key, subkey.c_str(), On 2013/01/31 15:16:27, grt wrote: > the product is only installed if the Clients key has a "pv" value, so change > this to: > return reg_key.Open(...) == ERROR_SUCCESS && reg_key.HasValue("pv"); > please update the comment at the top accordingly (and use a proper constant > rather than using the string literal here). The function should have a better name, too. Like IsProductInstalled or somesuch.
https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... chrome/installer/launcher_support/chrome_launcher_support.cc:203: return true; On 2013/01/31 15:09:45, gab wrote: > We don't usually special case the official build (as that only calls for > untested cases in developer builds). > > Is this to always return true in developer builds? If so, I would hope there is > a better way. This is for Chromium vs. Chrome. For Chromium builds, checking the Omaha state is irrelevant. https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... File chrome/installer/launcher_support/chrome_launcher_support.h (right): https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... chrome/installer/launcher_support/chrome_launcher_support.h:57: bool IsChromeBrowserPresent(); On 2013/01/31 15:09:45, gab wrote: > Can you not use InstallerState::FindProduct() from installer_util here? We don't have access to insaller_util here. Actually I would like to refactor installer_util so that it could essentially replace all of the code in this library.
https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... chrome/installer/launcher_support/chrome_launcher_support.cc:203: return true; On 2013/01/31 15:20:00, erikwright wrote: > On 2013/01/31 15:09:45, gab wrote: > > We don't usually special case the official build (as that only calls for > > untested cases in developer builds). > > > > Is this to always return true in developer builds? If so, I would hope there > is > > a better way. > > This is for Chromium vs. Chrome. > > For Chromium builds, checking the Omaha state is irrelevant. Ok, then why not use #if defined (GOOGLE_CHROME_BUILD)? Is the app_host even relevant in Chromium? https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... File chrome/installer/launcher_support/chrome_launcher_support.h (right): https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... chrome/installer/launcher_support/chrome_launcher_support.h:57: bool IsChromeBrowserPresent(); On 2013/01/31 15:20:00, erikwright wrote: > On 2013/01/31 15:09:45, gab wrote: > > Can you not use InstallerState::FindProduct() from installer_util here? > > We don't have access to insaller_util here. Actually I would like to refactor > installer_util so that it could essentially replace all of the code in this > library. OK, sgtm. Is this something you want to do "eventually" or instead of this CL? I'm not a fan of accumulating helpers here given they could all be replaced by existing code...
https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... chrome/installer/launcher_support/chrome_launcher_support.cc:88: return reg_key.Open(root_key, subkey.c_str(), On 2013/01/31 15:18:21, grt wrote: > On 2013/01/31 15:16:27, grt wrote: > > the product is only installed if the Clients key has a "pv" value, so change > > this to: > > return reg_key.Open(...) == ERROR_SUCCESS && reg_key.HasValue("pv"); > > please update the comment at the top accordingly (and use a proper constant > > rather than using the string literal here). > > The function should have a better name, too. Like IsProductInstalled or > somesuch. Done. https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... chrome/installer/launcher_support/chrome_launcher_support.cc:88: return reg_key.Open(root_key, subkey.c_str(), On 2013/01/31 15:16:27, grt wrote: > the product is only installed if the Clients key has a "pv" value, so change > this to: > return reg_key.Open(...) == ERROR_SUCCESS && reg_key.HasValue("pv"); > please update the comment at the top accordingly (and use a proper constant > rather than using the string literal here). Done. https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... chrome/installer/launcher_support/chrome_launcher_support.cc:203: return true; On 2013/01/31 18:05:40, gab wrote: > On 2013/01/31 15:20:00, erikwright wrote: > > On 2013/01/31 15:09:45, gab wrote: > > > We don't usually special case the official build (as that only calls for > > > untested cases in developer builds). > > > > > > Is this to always return true in developer builds? If so, I would hope there > > is > > > a better way. > > > > This is for Chromium vs. Chrome. > > > > For Chromium builds, checking the Omaha state is irrelevant. > > Ok, then why not use #if defined (GOOGLE_CHROME_BUILD)? > > Is the app_host even relevant in Chromium? I thought this was just to make dev's lives easier. I don't need it so I've removed it. https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... File chrome/installer/launcher_support/chrome_launcher_support.h (right): https://codereview.chromium.org/12094053/diff/2003/chrome/installer/launcher_... chrome/installer/launcher_support/chrome_launcher_support.h:57: bool IsChromeBrowserPresent(); On 2013/01/31 18:05:40, gab wrote: > On 2013/01/31 15:20:00, erikwright wrote: > > On 2013/01/31 15:09:45, gab wrote: > > > Can you not use InstallerState::FindProduct() from installer_util here? > > > > We don't have access to insaller_util here. Actually I would like to refactor > > installer_util so that it could essentially replace all of the code in this > > library. > > OK, sgtm. Is this something you want to do "eventually" or instead of this CL? > > I'm not a fan of accumulating helpers here given they could all be replaced by > existing code... I'd prefer not to have to refactor installer_util as part of this change.
lgtm for this change, but I'm really not a fan of this file; please highly consider refactoring this to re-use installer_util. Thanks, Gab https://codereview.chromium.org/12094053/diff/12001/chrome/installer/launcher... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://codereview.chromium.org/12094053/diff/12001/chrome/installer/launcher... chrome/installer/launcher_support/chrome_launcher_support.cc:91: reg_key.HasValue(kRegVersionField); nit: I think that to indent this aligned with reg_key.Open( above you need the return statement to be in ( ). Otherwise it should be indented as per line 205.
https://codereview.chromium.org/12094053/diff/12001/chrome/installer/launcher... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://codereview.chromium.org/12094053/diff/12001/chrome/installer/launcher... chrome/installer/launcher_support/chrome_launcher_support.cc:91: reg_key.HasValue(kRegVersionField); On 2013/02/01 02:00:33, gab wrote: > nit: I think that to indent this aligned with reg_key.Open( above you need the > return statement to be in ( ). > Otherwise it should be indented as per line 205. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benwells@chromium.org/12094053/9003
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benwells@chromium.org/12094053/9003
Message was sent while issue was closed.
Change committed as 180329
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/12094053/diff/9003/chrome/installer/la... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/12094053/diff/9003/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:91: reg_key.HasValue(kRegVersionField); I just thought of something... I think the installer doesn't drop these registry keys if Google Update is not installed... (i.e., this could be a pain for developers that don't have any Google products (drive, canary, etc.) on a test machine, but do have a local developer chrome installed which will not be detected by this...).
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/12094053/diff/9003/chrome/installer/la... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/12094053/diff/9003/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:91: reg_key.HasValue(kRegVersionField); On 2013/02/04 16:49:31, gab wrote: > I just thought of something... I think the installer doesn't drop these registry > keys if Google Update is not installed... (i.e., this could be a pain for > developers that don't have any Google products (drive, canary, etc.) on a test > machine, but do have a local developer chrome installed which will not be > detected by this...). Chrome's installer unconditionally writes "pv" (it doesn't check for the presence of Google Update), so I think it's okay. Good lookin' out. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
