Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10451074:
Always suffix ChromeHTML entries on Windows for user-level installs. (Closed)
|
Created:
8 years, 6 months ago by gab Modified:
8 years, 6 months ago Reviewers:
grt (UTC plus 2) CC:
chromium-reviews, grt+watch_chromium.org, robertshield, chrome-win8-eng_google.com Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAlways suffix ChromeHTML entries on Windows for user-level installs.
This also adds the same suffixing to Chrome's appname for Default Programs registration (this suffix is not user-facing though as we don't suffix the actual string representing Chrome in the UI... obviously!)
Design doc: https://docs.google.com/a/chromium.org/document/d/1qmcV3uYBh3JwvXhYkI7asg0nN7KfVMWVOzND4p0jQ3E/edit
BUG=125362, 124013, 133173
TEST=http://goo.gl/ZZ7gE
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142634
Patch Set 1 #
Total comments: 3
Patch Set 2 : system level always unsuffixed and other nits #Patch Set 3 : dont change RemoveChromeLegacyRegistryKeys()'s behavior #
Total comments: 33
Patch Set 4 : fix InstallUtil::DeleteRegistryValue #
Total comments: 3
Patch Set 5 : some comments, others pending discussion #Patch Set 6 : better fix for InstallUtil::DeleteRegistryValue #
Total comments: 2
Patch Set 7 : nit #
Total comments: 31
Patch Set 8 : address comments + introduce ShellUtil::QuickIsChromeRegistered() #
Total comments: 25
Patch Set 9 : no UAC on uninstall if nothing in HKLM #
Total comments: 11
Patch Set 10 : addressed comments #
Total comments: 6
Patch Set 11 : nits #
Total comments: 2
Patch Set 12 : 1 space nit #Patch Set 13 : rebase on r142136 #Patch Set 14 : rebase on r142211 #
Messages
Total messages: 24 (0 generated)
Another patch will follow to suffix Chrome's appid, for now this puts everything in place for better suffixing in installer utils and uses it to always suffix ChromeHTML on user-level installs. Cheers, Gab
@aa: Can you address the comment below (not the entire CL). Thanks, Gab https://chromiumcodereview.appspot.com/10451074/diff/1/chrome/installer/setup... File chrome/installer/setup/uninstall.cc (left): https://chromiumcodereview.appspot.com/10451074/diff/1/chrome/installer/setup... chrome/installer/setup/uninstall.cc:681: if (roots[i] == HKEY_LOCAL_MACHINE && This was introduced in 2009 in http://src.chromium.org/viewvc/chrome/trunk/src/chrome/installer/setup/uninst... however the corresponding codereview (http://codereview.chromium.org/210007) does not have this line. It's not clear to me why the suffix is only applied for HKLM... can you comment on this section of the change please. We can actually probably remove this function entirely given it is doing some cleanup needed 2.5 years ago...
No idea, but I agree that we can remove old cleanup code.
https://chromiumcodereview.appspot.com/10451074/diff/1/chrome/installer/setup... File chrome/installer/setup/uninstall.cc (left): https://chromiumcodereview.appspot.com/10451074/diff/1/chrome/installer/setup... chrome/installer/setup/uninstall.cc:681: if (roots[i] == HKEY_LOCAL_MACHINE && Ok, thanks aa@, I'll remove it: https://chromiumcodereview.appspot.com/10454067/
https://chromiumcodereview.appspot.com/10451074/diff/1/chrome/installer/setup... File chrome/installer/setup/uninstall.cc (left): https://chromiumcodereview.appspot.com/10451074/diff/1/chrome/installer/setup... chrome/installer/setup/uninstall.cc:681: if (roots[i] == HKEY_LOCAL_MACHINE && On 2012/05/30 02:45:31, gab wrote: > Ok, thanks aa@, I'll remove it: https://chromiumcodereview.appspot.com/10454067/ Actually, as discussed with grt@, we won't remove this (ping me offline for details). I modified this CL to reflect exactly the current behavior for this function with my new set of calls.
Tiny update, see below. https://chromiumcodereview.appspot.com/10451074/diff/1008/chrome/installer/ut... File chrome/installer/util/install_util.cc (right): https://chromiumcodereview.appspot.com/10451074/diff/1008/chrome/installer/ut... chrome/installer/util/install_util.cc:390: RegKey key(reg_root, key_path.c_str(), KEY_ALL_ACCESS); This step was causing RemoveBadWindows8RegistrationIfNeeded() to leave HKEY_CURRENT_USER\Software\Classes\ChromeHTML behind after deleting HKEY_CURRENT_USER\Software\Classes\ChromeHTML\shell\open\command\DelegateExecute. The new code above fixes it.
Looks like a good direction. I need to stop for now. I'll make another full pass after these comments have been addressed. We can chat tomorrow. https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/browser/shel... File chrome/browser/shell_integration_win.cc (right): https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/browser/shel... chrome/browser/shell_integration_win.cc:77: prog_id += ShellUtil::GetCurrentInstallationSuffix(); It would be nice to abstract the whole idea of the suffix away from consumers of ShellUtil. How about adding these method to ShellUtil for use by shell_integration_win and others: static string16 GetProgId(BrowserDistribution*); static string16 GetApplicationName(BrowserDistribution*); https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/se... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/se... chrome/installer/setup/uninstall.cc:731: if (ShellUtil::IsInstallationPresentInHKLM(browser_dist, chrome_exe, The behavior implemented in IsInstallationPresentInHKLM is not desired here. Uninstall is a best-effort deal, so we don't want to leave everything behind if one expected value isn't in the registry. Instead, we just want to know if any system-level registrations need to be removed. The old code took some pains to search for registration pointing toward the current chrome.exe. https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/se... chrome/installer/setup/uninstall.cc:792: // previously installed with no suffix in HKCU (old suffix rules) and later How would this happen in practice? Would this happen if the user cancelled the UAC prompt the first time, and then accepted it a second time, or something? https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/se... chrome/installer/setup/uninstall.cc:807: if (installer_state.system_install() || !suffix.empty() || remove_all) { If this user has not made her Chrome the default (suffix is empty) but another user has (so there is an un-suffixed user-level Chrome registered in HKLM), this will delete that other users' registrations, will it not? Likewise, if there is a system-level Chrome registered and this user is doing an ordinary uninstall (not a self-destruct), then this will attempt to remove the system-level Chrome's registrations. The previous code was careful not to touch HKLM unless it was truly this user's Chrome that was registered there. https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... chrome/installer/util/shell_util.cc:411: // queries into HKCU in that case. Starting with Windows 8, Chrome's values go Is this statement about where Chrome's values go in Windows 8 true? I thought this was blocked on a perceived bug in Windows. https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... chrome/installer/util/shell_util.cc:679: const string16& chrome_exe) { nit: indentation https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... chrome/installer/util/shell_util.cc:692: // Assert that the Chrome registered there is |chrome_exe| (i.e. this user's String comparisons of paths make me uncomfortable. setup_util's ProgramCompare will do a more thorough check. Feel free to move it into installer_util so it can be reused. This function would be shorter using that, too. https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... chrome/installer/util/shell_util.cc:714: LOG(DFATAL) << "Failed to get username in " << __FUNCTION__ << "."; I think you can remove __FUNCTION__ since it doesn't add anything to the log message. The string is enough to find the line in the source code. Also, use PLOG rather than LOG so that the last error code is automagically logged. https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... chrome/installer/util/shell_util.cc:723: // Sets |suffix| to the current user's username on user-level installs. Regarding "to the current user's username": please mention that it's preceded by '.'. https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... chrome/installer/util/shell_util.cc:1000: if (!PathService::Get(base::FILE_EXE, &chrome_exe)) { In general, we should strive to not do this PathService and IsPerUserInstall stuff deep in utility functions. The main reasons are that they both potentially do real work (i.e., I/O) and they make testing very difficult. It looks like all callers in this file already have the path to chrome, so it should be easy to change the signature to take const FilePath& chrome_exe. The call to IsPerUserInstall could potentially stay if needed, since passing in the path is enough to make tests feasible. https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... chrome/installer/util/shell_util.cc:1009: !RegistryEntry::IsChromeRegisteredWithSuffix(tested_suffix)) { What's the motivation for looking at the ProgID rather than the client key (as is done by CurrentUserIsRegisteredWithNoSuffix)? https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... chrome/installer/util/shell_util.cc:1031: ShellUtil::RegisterChromeBrowser(dist, chrome_exe, L"", elevate_if_not_admin); Can anything good happen in MakeChromeDefault if RegisterChromeBrowser returns false? If not, how about an early exit here? https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... chrome/installer/util/shell_util.h:224: // Returns true if any of the possible Chrome registry keys are present in according to the implementation: "any" -> "all" https://chromiumcodereview.appspot.com/10451074/diff/1008/chrome/installer/ut... File chrome/installer/util/install_util.cc (right): https://chromiumcodereview.appspot.com/10451074/diff/1008/chrome/installer/ut... chrome/installer/util/install_util.cc:390: RegKey key(reg_root, key_path.c_str(), KEY_ALL_ACCESS); Please use something along the lines of: RegKey key; LONG result = key.Open(reg_root, key_path.c_str(), KEY_SET_VALUE); if (result == ERROR_SUCCESS) result = key.DeleteValue(value_name.c_str()); if (result != ERROR_SUCCESS && result != ERROR_FILE_NOT_FOUND) { LOG(ERROR) << yadda yadda; return false; } return true;
Addressed comments (some in code, some in words). Only skipped comment about RegKey as I need to try it out with regedit and maybe procmon and RDP just doesn't do it at that point... will get to it tomorrow. This is now based on top of the CL to move ProgramCompare so if you see weird diff between patchset 4-->5 keep that in mind ;)! Cheers, Gab https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/browser/shel... File chrome/browser/shell_integration_win.cc (right): https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/browser/shel... chrome/browser/shell_integration_win.cc:77: prog_id += ShellUtil::GetCurrentInstallationSuffix(); On 2012/05/30 20:37:40, grt wrote: > It would be nice to abstract the whole idea of the suffix away from consumers of > ShellUtil. How about adding these method to ShellUtil for use by > shell_integration_win and others: > > static string16 GetProgId(BrowserDistribution*); > static string16 GetApplicationName(BrowserDistribution*); Yea, I'm not sure functions in ShellUtil would serve that purpose though as the consumers could still call the functions from BrowserDistribution not knowing about our extra function. I think the best way to fix this is through the template design pattern in browser_distribution.h/cc: Have browser_distribution.h/cc define and implement two public functions: GetSuffixedAppId() GetSuffixedApplicationName() which are final (i.e. non-overridable, I used to do this all-the-time in Java, not sure about C++) Then move the current two functions to private. And have the implementation in browser_distribution.cc of GetSuffixedAppId()+GetSuffixedApplicationName() do the suffixing of the appid it gets from the privately overriden non-suffixed getters (i.e. all the actual browser distributions still return static strings and "do no work" as desired). https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/se... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/se... chrome/installer/setup/uninstall.cc:731: if (ShellUtil::IsInstallationPresentInHKLM(browser_dist, chrome_exe, On 2012/05/30 20:37:40, grt wrote: > The behavior implemented in IsInstallationPresentInHKLM is not desired here. > Uninstall is a best-effort deal, so we don't want to leave everything behind if > one expected value isn't in the registry. Instead, we just want to know if any > system-level registrations need to be removed. The old code took some pains to > search for registration pointing toward the current chrome.exe. Arg, this was the intent of IsInstallationPresentInHKLM all along, but I messed up when I refactored my CL into fewer calls (i.e. went from return true if ANY are present to ALL...) Fixed. https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/se... chrome/installer/setup/uninstall.cc:792: // previously installed with no suffix in HKCU (old suffix rules) and later On 2012/05/30 20:37:40, grt wrote: > How would this happen in practice? Would this happen if the user cancelled the > UAC prompt the first time, and then accepted it a second time, or something? Yea, if the user cancels the first UAC (now they'd get suffixed anyways, but before they didn't) and later makes chrome default through the UI and accepts UAC then (they will get suffixed, even in the current behavior its possible that they get suffixed then if we realize the unsuffixed install is taken already). https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/se... chrome/installer/setup/uninstall.cc:807: if (installer_state.system_install() || !suffix.empty() || remove_all) { On 2012/05/30 20:37:40, grt wrote: > If this user has not made her Chrome the default (suffix is empty) but another > user has (so there is an un-suffixed user-level Chrome registered in HKLM), this > will delete that other users' registrations, will it not? Likewise, if there is > a system-level Chrome registered and this user is doing an ordinary uninstall > (not a self-destruct), then this will attempt to remove the system-level > Chrome's registrations. The previous code was careful not to touch HKLM unless > it was truly this user's Chrome that was registered there. Ah right, good catch. I now in fact think that |remove_all| is superfluous (now and in the current code) as, if the system-level install is there, the current user no longer has the unsuffixed HKLM registration... I left it there anyways for now, to be safe as we don't EVER want to kill the new system-level install... (and |remove_all| is a cheap check which we already know the value of before doing I/O to check the current user's ownership status of the unsuffixed install I guess...) https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... chrome/installer/util/shell_util.cc:411: // queries into HKCU in that case. Starting with Windows 8, Chrome's values go On 2012/05/30 20:37:40, grt wrote: > Is this statement about where Chrome's values go in Windows 8 true? I thought > this was blocked on a perceived bug in Windows. Yes, in fact it was already checked in somehow, I guess I previously wrote that dreaming of a better future :)! I'll remove it for now as that's not the current behavior. https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... chrome/installer/util/shell_util.cc:679: const string16& chrome_exe) { On 2012/05/30 20:37:40, grt wrote: > nit: indentation Done. https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... chrome/installer/util/shell_util.cc:692: // Assert that the Chrome registered there is |chrome_exe| (i.e. this user's On 2012/05/30 20:37:40, grt wrote: > String comparisons of paths make me uncomfortable. setup_util's ProgramCompare > will do a more thorough check. Feel free to move it into installer_util so it > can be reused. This function would be shorter using that, too. This code was copied from the old AnotherUserHasDefaultBrowser() removed above. I agree that ProgramCompare is much better, I'll move it as requested (http://codereview.chromium.org/10446095/) and use it here. https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... chrome/installer/util/shell_util.cc:714: LOG(DFATAL) << "Failed to get username in " << __FUNCTION__ << "."; On 2012/05/30 20:37:40, grt wrote: > I think you can remove __FUNCTION__ since it doesn't add anything to the log > message. The string is enough to find the line in the source code. Also, use > PLOG rather than LOG so that the last error code is automagically logged. Ok, my thinking was that it's quite a generic string and this makes it specific, but I guess there still can't be more than a couple (if any) duplicate of this string in the code... https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... chrome/installer/util/shell_util.cc:723: // Sets |suffix| to the current user's username on user-level installs. On 2012/05/30 20:37:40, grt wrote: > Regarding "to the current user's username": please mention that it's preceded by > '.'. Done. https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... chrome/installer/util/shell_util.cc:1000: if (!PathService::Get(base::FILE_EXE, &chrome_exe)) { On 2012/05/30 20:37:40, grt wrote: > In general, we should strive to not do this PathService and IsPerUserInstall > stuff deep in utility functions. The main reasons are that they both > potentially do real work (i.e., I/O) and they make testing very difficult. It > looks like all callers in this file already have the path to chrome, so it > should be easy to change the signature to take const FilePath& chrome_exe. The > call to IsPerUserInstall could potentially stay if needed, since passing in the > path is enough to make tests feasible. Right, I don't like it either, the problem though is that this is not called at install-time (where we know chrome_exe ), but by various callers at run-time (that don't know chrome_exe). In the end though this is all just to make sure this is not a system-level install for a corner case described in the comment... I see other ways of doing this, but they aren't really better: - Singleton with chrome's install state - Caching of the suffix (I don't know if Chromium prefers CPU overload to memory overload (i.e. is caching usually a good thing for this kind of function?))... this addresses the "repetitive I/O" problem, but doesn't address the testing problem... https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... chrome/installer/util/shell_util.cc:1009: !RegistryEntry::IsChromeRegisteredWithSuffix(tested_suffix)) { On 2012/05/30 20:37:40, grt wrote: > What's the motivation for looking at the ProgID rather than the client key (as > is done by CurrentUserIsRegisteredWithNoSuffix)? Chrome can have only its ProgIds registered (if the user is not admin on Win7-). This function is trying to determine the suffix of the current Chrome for run-time behavior. CurrentUserIsRegisteredWithNoSuffix is trying to determine whether, at update-time, we should update the registry without a suffix. This call on the other hand can't be replaced by !RegistryEntry::IsChromeRegisteredWithSuffix(suffix), as we want to always suffix UNLESS a no suffix install IS present (i.e. not not suffix if no suffix install is present...). https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... chrome/installer/util/shell_util.cc:1031: ShellUtil::RegisterChromeBrowser(dist, chrome_exe, L"", elevate_if_not_admin); On 2012/05/30 20:37:40, grt wrote: > Can anything good happen in MakeChromeDefault if RegisterChromeBrowser returns > false? If not, how about an early exit here? Done. https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... chrome/installer/util/shell_util.h:224: // Returns true if any of the possible Chrome registry keys are present in On 2012/05/30 20:37:40, grt wrote: > according to the implementation: "any" -> "all" As mentionned on another comment, the intention is "any" and I messed up the implementation before sending the CL...
See comments below. Cheers, Gab https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/browser/shel... File chrome/browser/shell_integration_win.cc (right): https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/browser/shel... chrome/browser/shell_integration_win.cc:77: prog_id += ShellUtil::GetCurrentInstallationSuffix(); On 2012/05/31 06:36:01, gab wrote: > On 2012/05/30 20:37:40, grt wrote: > > It would be nice to abstract the whole idea of the suffix away from consumers > of > > ShellUtil. How about adding these method to ShellUtil for use by > > shell_integration_win and others: > > > > static string16 GetProgId(BrowserDistribution*); > > static string16 GetApplicationName(BrowserDistribution*); > > Yea, I'm not sure functions in ShellUtil would serve that purpose though as the > consumers could still call the functions from BrowserDistribution not knowing > about our extra function. > > I think the best way to fix this is through the template design pattern in > browser_distribution.h/cc: > > Have browser_distribution.h/cc define and implement two public functions: > GetSuffixedAppId() > GetSuffixedApplicationName() > which are final (i.e. non-overridable, I used to do this all-the-time in Java, > not sure about C++) > > Then move the current two functions to private. > > And have the implementation in browser_distribution.cc of > GetSuffixedAppId()+GetSuffixedApplicationName() do the suffixing of the appid it > gets from the privately overriden non-suffixed getters (i.e. all the actual > browser distributions still return static strings and "do no work" as desired). Ok, I added this in http://codereview.chromium.org/10446111/, which I'd like to checkin after this patch (I don't want to bloat up this CL with that change, but on the other hand it's easier to base it on top of the new suffixing code than making it work with the old one, checking it in, and adapt this CL to it after...) Let me know if that works for you. https://chromiumcodereview.appspot.com/10451074/diff/1008/chrome/installer/ut... File chrome/installer/util/install_util.cc (right): https://chromiumcodereview.appspot.com/10451074/diff/1008/chrome/installer/ut... chrome/installer/util/install_util.cc:390: RegKey key(reg_root, key_path.c_str(), KEY_ALL_ACCESS); On 2012/05/30 20:37:40, grt wrote: > Please use something along the lines of: > > RegKey key; > LONG result = key.Open(reg_root, key_path.c_str(), KEY_SET_VALUE); > if (result == ERROR_SUCCESS) > result = key.DeleteValue(value_name.c_str()); > if (result != ERROR_SUCCESS && result != ERROR_FILE_NOT_FOUND) { > LOG(ERROR) << yadda yadda; > return false; > } > return true; Done.
Apologies for the delay. https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/se... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/se... chrome/installer/setup/uninstall.cc:792: // previously installed with no suffix in HKCU (old suffix rules) and later On 2012/05/31 06:36:01, gab wrote: > On 2012/05/30 20:37:40, grt wrote: > > How would this happen in practice? Would this happen if the user cancelled > the > > UAC prompt the first time, and then accepted it a second time, or something? > > Yea, if the user cancels the first UAC (now they'd get suffixed anyways, but > before they didn't) and later makes chrome default through the UI and accepts > UAC then (they will get suffixed, even in the current behavior its possible that > they get suffixed then if we realize the unsuffixed install is taken already). Cool, thanks for the explanation. I think this is sufficiently non-obvious that it's worth capturing in code comments somewhere. What do you think? https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... chrome/installer/util/shell_util.cc:714: LOG(DFATAL) << "Failed to get username in " << __FUNCTION__ << "."; On 2012/05/31 06:36:01, gab wrote: > On 2012/05/30 20:37:40, grt wrote: > > I think you can remove __FUNCTION__ since it doesn't add anything to the log > > message. The string is enough to find the line in the source code. Also, use > > PLOG rather than LOG so that the last error code is automagically logged. > > Ok, my thinking was that it's quite a generic string and this makes it specific, > but I guess there still can't be more than a couple (if any) duplicate of this > string in the code... I find __FUNCTION__ can be really helpful in tests that generate lots of output. In this case, though I don't think it's needed since file and line are logged. https://chromiumcodereview.appspot.com/10451074/diff/1007/chrome/installer/ut... chrome/installer/util/shell_util.cc:1000: if (!PathService::Get(base::FILE_EXE, &chrome_exe)) { On 2012/05/31 06:36:01, gab wrote: > On 2012/05/30 20:37:40, grt wrote: > > In general, we should strive to not do this PathService and IsPerUserInstall > > stuff deep in utility functions. The main reasons are that they both > > potentially do real work (i.e., I/O) and they make testing very difficult. It > > looks like all callers in this file already have the path to chrome, so it > > should be easy to change the signature to take const FilePath& chrome_exe. > The > > call to IsPerUserInstall could potentially stay if needed, since passing in > the > > path is enough to make tests feasible. > > Right, I don't like it either, the problem though is that this is not called at > install-time (where we know chrome_exe ), but by various callers at run-time > (that don't know chrome_exe). > > In the end though this is all just to make sure this is not a system-level > install for a corner case described in the comment... > > I see other ways of doing this, but they aren't really better: > - Singleton with chrome's install state > - Caching of the suffix (I don't know if Chromium prefers CPU overload to memory > overload (i.e. is caching usually a good thing for this kind of function?))... > this addresses the "repetitive I/O" problem, but doesn't address the testing > problem... My suggestion was to change the function signature so that const FilePath& chrome_exe was passed in. This is simple for all callers in this file since they already have the path to chrome_exe. It's also simple for callers in uninstall.cc since they know the path to chrome.exe (and using PathService will actually give them the path to setup.exe anyway). Any other callers from within Chrome can use PathService themselves. https://chromiumcodereview.appspot.com/10451074/diff/9003/chrome/installer/ut... File chrome/installer/util/install_util.cc (right): https://chromiumcodereview.appspot.com/10451074/diff/9003/chrome/installer/ut... chrome/installer/util/install_util.cc:388: LOG(ERROR) << "Failed to delete registry value: " << value_name if (result == ERROR_SUCCESS) { result = key.DeleteValue(value_name.c_str()); } else if (result != ERROR_FILE_NOT_FOUND) { should be: if (result == ERROR_SUCCESS) result = key.DeleteValue(value_name.c_str()); if (result != ERROR_SUCCESS && result != ERROR_FILE_NOT_FOUND) { so that true is only returned on success, and so the log message is displayed for both failures. https://chromiumcodereview.appspot.com/10451074/diff/4023/chrome/installer/se... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/10451074/diff/4023/chrome/installer/se... chrome/installer/setup/uninstall.cc:731: if (ShellUtil::IsInstallationPresentInHKLM(browser_dist, chrome_exe, See comment on line 808 below. https://chromiumcodereview.appspot.com/10451074/diff/4023/chrome/installer/se... chrome/installer/setup/uninstall.cc:808: if (installer_state.system_install() || !suffix.empty() || I think you should revert the logic here. If I'm reading it correctly, your change will cause the self-destruct of a suffixed user-level Chrome to remove some of the registration bits for the system-level Chrome. Same comment applies to line 731 above. https://chromiumcodereview.appspot.com/10451074/diff/4023/chrome/installer/ut... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10451074/diff/4023/chrome/installer/ut... chrome/installer/util/shell_util.cc:80: LOOK_IN_HKLM = 1 << 1, consider adding LOOK_IN_HKCU_THEN_HKLM or something like that so that consumers don't need to OR the two values together when they want both. https://chromiumcodereview.appspot.com/10451074/diff/4023/chrome/installer/ut... chrome/installer/util/shell_util.cc:111: // registration of Chrome. Please explain in the comment why checking only the ProgID is sufficient. https://chromiumcodereview.appspot.com/10451074/diff/4023/chrome/installer/ut... chrome/installer/util/shell_util.cc:119: ShellUtil::kChromeHTMLProgIdDesc).ExistsInRegistry( Checking that the description matches seems fragile. How about either just checking for existence of the ProgID (simple), or checking that it points to the chrome.exe in question (more complex, but gives you the real right answer)? https://chromiumcodereview.appspot.com/10451074/diff/4023/chrome/installer/ut... chrome/installer/util/shell_util.cc:511: uint32 look_for_in) { nit: indentation https://chromiumcodereview.appspot.com/10451074/diff/4023/chrome/installer/ut... chrome/installer/util/shell_util.cc:660: const string16 installation_suffix = nit: use () rather than = https://chromiumcodereview.appspot.com/10451074/diff/4023/chrome/installer/ut... chrome/installer/util/shell_util.cc:689: PLOG(DFATAL) << "Failed to get username from Windows."; nit: since this log string ends up in the binary, consider just "GetUserName failed". PLOG will add ": " + system error message, so don't end the message with a '.' in this case. https://chromiumcodereview.appspot.com/10451074/diff/4023/chrome/installer/ut... chrome/installer/util/shell_util.cc:713: *suffix = L""; suffix->clear(); https://chromiumcodereview.appspot.com/10451074/diff/4023/chrome/installer/ut... chrome/installer/util/shell_util.cc:986: return L""; L"" -> string16() https://chromiumcodereview.appspot.com/10451074/diff/4023/chrome/installer/ut... chrome/installer/util/shell_util.cc:1008: InstallUtil::ProgramCompare comparator(registry_chrome_exe_path); PC's ctor takes chrome_exe and Evaluate takes a registry value containing a command line and pulls out the path to the exe. This function can be simplified to something along the lines of: bool ShellUtil::CurrentUserIsRegisteredWithNoSuffix( BrowserDistribution* dist, const string16& chrome_exe) { // Assert that HKLM\Software\Clients\StartMenuInternet\Google Chrome // points to |chrome_exe|. const string16 reg_key( RegistryEntry::GetBrowserClientKey(dist, string16()) .append(ShellUtil::kRegShellOpen)); RegKey key(HKEY_LOCAL_MACHINE, reg_key.c_str(), KEY_QUERY_VALUE); string16 value; if (key.ReadValue(L"", &value) == ERROR_SUCCESS) return InstallUtil::ProgramCompare(FilePath(chrome_exe)).Evaluate(value); return false; } https://chromiumcodereview.appspot.com/10451074/diff/4023/chrome/installer/ut... chrome/installer/util/shell_util.cc:1018: return AreAnyEntriesRegistered(entries, RegistryEntry::LOOK_IN_HKLM); This will return true for an unsuffixed user-level install when a system-level install is present. I think it's safer (and simpler) to use ProgramCompare to see if the open command under the StartMenuInternet key matches |chrome_exe|. One fairly simple and safe thing would be to resurrect most of what was CurrentUserHasDefaultBrowser in uninstall.cc with the following modifications: - have it pass over any keys that aren't equal to the browser distribution's ApplicationName or don't start with the bd's ApplicationName + '.'. - when it finds a match via its ProgramCompare instance, have it extract the suffix from the key name and return it via an out param. The suffix returned is the one that should then be used to delete the other HKLM registrations (but only if |remove_all| is set). https://chromiumcodereview.appspot.com/10451074/diff/4023/chrome/installer/ut... chrome/installer/util/shell_util.cc:1029: dist, chrome_exe, L"", elevate_if_not_admin)) { L"" -> string16() https://chromiumcodereview.appspot.com/10451074/diff/4023/chrome/installer/ut... chrome/installer/util/shell_util.cc:1211: !AreEntriesRegistered(entries, RegistryEntry::LOOK_IN_HKCU)) { there's no need to AddRegistryEntries if AreEntriesRegistered returns true. is this equivalent (with appropriate comments added)? if (!AreEntriesRegistered(entries, RegistryEntry::LOOK_IN_HKCU)) { if (!suffix.empty()) { STLDeleteElements(entries); RegistryEntry::GetProgIdEntries(dist, chrome_exe, suffix, &entries); } return AddRegistryEntries(HKEY_CURRENT_USER, entries); } return true; } https://chromiumcodereview.appspot.com/10451074/diff/4023/chrome/installer/ut... chrome/installer/util/shell_util.cc:1212: entries.clear(); entries.clear() -> STLDeleteElements(entries)
Comments addressed which gave me some insights into making an even cleaner patch I believe. PTA(nother!)L. Thanks, Gab http://codereview.chromium.org/10451074/diff/1007/chrome/installer/setup/unin... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/10451074/diff/1007/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:792: // previously installed with no suffix in HKCU (old suffix rules) and later On 2012/06/04 02:18:26, grt wrote: > On 2012/05/31 06:36:01, gab wrote: > > On 2012/05/30 20:37:40, grt wrote: > > > How would this happen in practice? Would this happen if the user cancelled > > the > > > UAC prompt the first time, and then accepted it a second time, or something? > > > > Yea, if the user cancels the first UAC (now they'd get suffixed anyways, but > > before they didn't) and later makes chrome default through the UI and accepts > > UAC then (they will get suffixed, even in the current behavior its possible > that > > they get suffixed then if we realize the unsuffixed install is taken already). > > Cool, thanks for the explanation. I think this is sufficiently non-obvious that > it's worth capturing in code comments somewhere. What do you think? Added clarification as to the example of when this can happen. Let me know if you think more details are needed. http://codereview.chromium.org/10451074/diff/1007/chrome/installer/util/shell... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10451074/diff/1007/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:714: LOG(DFATAL) << "Failed to get username in " << __FUNCTION__ << "."; On 2012/06/04 02:18:26, grt wrote: > On 2012/05/31 06:36:01, gab wrote: > > On 2012/05/30 20:37:40, grt wrote: > > > I think you can remove __FUNCTION__ since it doesn't add anything to the log > > > message. The string is enough to find the line in the source code. Also, > use > > > PLOG rather than LOG so that the last error code is automagically logged. > > > > Ok, my thinking was that it's quite a generic string and this makes it > specific, > > but I guess there still can't be more than a couple (if any) duplicate of this > > string in the code... > > I find __FUNCTION__ can be really helpful in tests that generate lots of output. > In this case, though I don't think it's needed since file and line are logged. Ah ok, hadn't realized file and line were logged, I agree then. Thanks for clarifying! http://codereview.chromium.org/10451074/diff/1007/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1000: if (!PathService::Get(base::FILE_EXE, &chrome_exe)) { On 2012/06/04 02:18:26, grt wrote: > On 2012/05/31 06:36:01, gab wrote: > > On 2012/05/30 20:37:40, grt wrote: > > > In general, we should strive to not do this PathService and IsPerUserInstall > > > stuff deep in utility functions. The main reasons are that they both > > > potentially do real work (i.e., I/O) and they make testing very difficult. > It > > > looks like all callers in this file already have the path to chrome, so it > > > should be easy to change the signature to take const FilePath& chrome_exe. > > The > > > call to IsPerUserInstall could potentially stay if needed, since passing in > > the > > > path is enough to make tests feasible. > > > > Right, I don't like it either, the problem though is that this is not called > at > > install-time (where we know chrome_exe ), but by various callers at run-time > > (that don't know chrome_exe). > > > > In the end though this is all just to make sure this is not a system-level > > install for a corner case described in the comment... > > > > I see other ways of doing this, but they aren't really better: > > - Singleton with chrome's install state > > - Caching of the suffix (I don't know if Chromium prefers CPU overload to > memory > > overload (i.e. is caching usually a good thing for this kind of function?))... > > this addresses the "repetitive I/O" problem, but doesn't address the testing > > problem... > > My suggestion was to change the function signature so that const FilePath& > chrome_exe was passed in. This is simple for all callers in this file since > they already have the path to chrome_exe. It's also simple for callers in > uninstall.cc since they know the path to chrome.exe (and using PathService will > actually give them the path to setup.exe anyway). Any other callers from within > Chrome can use PathService themselves. Ah ok yes, makes sense. Done. http://codereview.chromium.org/10451074/diff/9003/chrome/installer/util/insta... File chrome/installer/util/install_util.cc (right): http://codereview.chromium.org/10451074/diff/9003/chrome/installer/util/insta... chrome/installer/util/install_util.cc:388: LOG(ERROR) << "Failed to delete registry value: " << value_name On 2012/06/04 02:18:26, grt wrote: > if (result == ERROR_SUCCESS) { > result = key.DeleteValue(value_name.c_str()); > } else if (result != ERROR_FILE_NOT_FOUND) { > > should be: > > if (result == ERROR_SUCCESS) > result = key.DeleteValue(value_name.c_str()); > if (result != ERROR_SUCCESS && result != ERROR_FILE_NOT_FOUND) { > > so that true is only returned on success, and so the log message is displayed > for both failures. Indeed, somehow thought I was clever, but the first block modifies |result|...doh! Done. http://codereview.chromium.org/10451074/diff/4023/chrome/installer/setup/unin... File chrome/installer/setup/uninstall.cc (left): http://codereview.chromium.org/10451074/diff/4023/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:826: (!suffix.empty() || CurrentUserHasDefaultBrowser(installer_state)))) { This line of logic is essentially saying IsUserRegisteredWithDefaultPrograms? i.e. as before having a suffix automatically meant you had registered for default programs. This has now been replaced by QuickIsChromeRegistered(..., CONFIRM_SYSTEM_REGISTRATION) for both call sites. http://codereview.chromium.org/10451074/diff/4023/chrome/installer/setup/unin... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/10451074/diff/4023/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:731: if (ShellUtil::IsInstallationPresentInHKLM(browser_dist, chrome_exe, On 2012/06/04 02:18:26, grt wrote: > See comment on line 808 below. Done. http://codereview.chromium.org/10451074/diff/4023/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:808: if (installer_state.system_install() || !suffix.empty() || On 2012/06/04 02:18:26, grt wrote: > I think you should revert the logic here. If I'm reading it correctly, your > change will cause the self-destruct of a suffixed user-level Chrome to remove > some of the registration bits for the system-level Chrome. Same comment applies > to line 731 above. Done, I made QuickIsChromeRegistered() work for this scenario as well (I don't like to reinstantiate CurrentUserHasDefaultBrowser() as I feel it was duplicate code before). I don't think we need to check all entries in RegisteredApplications as we can only own the unsuffixed registration or the username suffixed one (|unique_suffix| in RegisterChromeBrowser is only the result of forwarding the suffix when elevating for registration (calling setup.exe with an arbitrary suffix is not otherwise supported (i.e. GetCurrentInstallationSuffix() will not find it at run-time))). I think this now has the same behavior as before, while making use of the new methods. http://codereview.chromium.org/10451074/diff/4023/chrome/installer/util/shell... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10451074/diff/4023/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:80: LOOK_IN_HKLM = 1 << 1, On 2012/06/04 02:18:26, grt wrote: > consider adding LOOK_IN_HKCU_THEN_HKLM or something like that so that consumers > don't need to OR the two values together when they want both. Done. http://codereview.chromium.org/10451074/diff/4023/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:111: // registration of Chrome. On 2012/06/04 02:18:26, grt wrote: > Please explain in the comment why checking only the ProgID is sufficient. Done in the new enum RegistrationConfirmationLevel in shell_util.h http://codereview.chromium.org/10451074/diff/4023/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:119: ShellUtil::kChromeHTMLProgIdDesc).ExistsInRegistry( On 2012/06/04 02:18:26, grt wrote: > Checking that the description matches seems fragile. How about either just > checking for existence of the ProgID (simple), or checking that it points to the > chrome.exe in question (more complex, but gives you the real right answer)? Done and merged in QuickChromeIsRegistered(). http://codereview.chromium.org/10451074/diff/4023/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:511: uint32 look_for_in) { On 2012/06/04 02:18:26, grt wrote: > nit: indentation Done. http://codereview.chromium.org/10451074/diff/4023/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:660: const string16 installation_suffix = On 2012/06/04 02:18:26, grt wrote: > nit: use () rather than = Done. http://codereview.chromium.org/10451074/diff/4023/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:689: PLOG(DFATAL) << "Failed to get username from Windows."; On 2012/06/04 02:18:26, grt wrote: > nit: since this log string ends up in the binary, consider just "GetUserName > failed". PLOG will add ": " + system error message, so don't end the message > with a '.' in this case. Done. http://codereview.chromium.org/10451074/diff/4023/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:713: *suffix = L""; On 2012/06/04 02:18:26, grt wrote: > suffix->clear(); Done. http://codereview.chromium.org/10451074/diff/4023/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:986: return L""; On 2012/06/04 02:18:26, grt wrote: > L"" -> string16() Done. http://codereview.chromium.org/10451074/diff/4023/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1008: InstallUtil::ProgramCompare comparator(registry_chrome_exe_path); On 2012/06/04 02:18:26, grt wrote: > PC's ctor takes chrome_exe and Evaluate takes a registry value containing a > command line and pulls out the path to the exe. This function can be simplified > to something along the lines of: > > bool ShellUtil::CurrentUserIsRegisteredWithNoSuffix( > BrowserDistribution* dist, > const string16& chrome_exe) { > // Assert that HKLM\Software\Clients\StartMenuInternet\Google Chrome > // points to |chrome_exe|. > const string16 reg_key( > RegistryEntry::GetBrowserClientKey(dist, string16()) > .append(ShellUtil::kRegShellOpen)); > RegKey key(HKEY_LOCAL_MACHINE, reg_key.c_str(), KEY_QUERY_VALUE); > string16 value; > if (key.ReadValue(L"", &value) == ERROR_SUCCESS) > return InstallUtil::ProgramCompare(FilePath(chrome_exe)).Evaluate(value); > return false; > } Changed to QuickIsChromeRegistered() and added logic to merge the prior RegistryEntry::IsChromeRegisteredWithSuffix(). I could also get rid of this and simply use an augmented IsChromeRegistered() here, but looking at all possible registry entries seems like overkill when a quick runtime check is needed (or a best effort uninstall check is needed)... let me know if you feel otherwise. http://codereview.chromium.org/10451074/diff/4023/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1018: return AreAnyEntriesRegistered(entries, RegistryEntry::LOOK_IN_HKLM); On 2012/06/04 02:18:26, grt wrote: > This will return true for an unsuffixed user-level install when a system-level > install is present. I think it's safer (and simpler) to use ProgramCompare to > see if the open command under the StartMenuInternet key matches |chrome_exe|. > > One fairly simple and safe thing would be to resurrect most of what was > CurrentUserHasDefaultBrowser in uninstall.cc with the following modifications: > - have it pass over any keys that aren't equal to the browser distribution's > ApplicationName or don't start with the bd's ApplicationName + '.'. > - when it finds a match via its ProgramCompare instance, have it extract the > suffix from the key name and return it via an out param. > The suffix returned is the one that should then be used to delete the other HKLM > registrations (but only if |remove_all| is set). Removed. http://codereview.chromium.org/10451074/diff/4023/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1029: dist, chrome_exe, L"", elevate_if_not_admin)) { On 2012/06/04 02:18:26, grt wrote: > L"" -> string16() Done. http://codereview.chromium.org/10451074/diff/4023/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1211: !AreEntriesRegistered(entries, RegistryEntry::LOOK_IN_HKCU)) { On 2012/06/04 02:18:26, grt wrote: > there's no need to AddRegistryEntries if AreEntriesRegistered returns true. is > this equivalent (with appropriate comments added)? > > if (!AreEntriesRegistered(entries, RegistryEntry::LOOK_IN_HKCU)) { > if (!suffix.empty()) { > STLDeleteElements(entries); > RegistryEntry::GetProgIdEntries(dist, chrome_exe, suffix, &entries); > } > return AddRegistryEntries(HKEY_CURRENT_USER, entries); > } > return true; > } Indeed :), the one nice thing with the previous logic is that !suffix.empty() is a faster check (if it were false, we're done), but given this will no longer really ever be true this logic is strictly better. Done. http://codereview.chromium.org/10451074/diff/4023/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1212: entries.clear(); On 2012/06/04 02:18:26, grt wrote: > entries.clear() -> STLDeleteElements(entries) Done. http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:977: // of Windows 8. This has yet to be implemented for Chrome, but it is still true that these values are valid in Win8 in HKCU, so it is correct to look there first anyways.
Fix a tiny thing so that with we don't pop UAC on uninstall if system entries ARE registered, but NOT in HKLM (i.e. ref upcoming HKCU patch). http://codereview.chromium.org/10451074/diff/25001/chrome/installer/util/shel... File chrome/installer/util/shell_util.h (right): http://codereview.chromium.org/10451074/diff/25001/chrome/installer/util/shel... chrome/installer/util/shell_util.h:239: CONFIRM_SYSTEM_REGISTRATION_IN_HKLM, Added this enum entry to allow uninstall.cc to check for system entries in HKLM only.
sending partial comments now. more to come. https://chromiumcodereview.appspot.com/10451074/diff/18001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10451074/diff/18001/chrome/installer/u... chrome/installer/util/shell_util.cc:81: LOOK_IN_HKCU_THEN_HKLM, this makes the logic worse in the func. i meant leaving the first two as they were and adding: LOOK_IN_HKCU_THEN_HKLM = LOOK_IN_HKCU | LOOK_IN_HKLM, https://chromiumcodereview.appspot.com/10451074/diff/18001/chrome/installer/u... chrome/installer/util/shell_util.cc:275: dist->GetAppShortCutName())); this seemingly trivial change will make IsChromeRegistered return false for all existing installs, will it not? does the test plan in your design doc include how to test for such breaking changes? https://chromiumcodereview.appspot.com/10451074/diff/18001/chrome/installer/u... chrome/installer/util/shell_util.cc:393: // registrations outside of HKCR on versions of Windows up to Win7, Chrome's "up to Win7" -> "prior to Win8" to reduce the ambiguity for Server 2008 R2, which could be considered past Win7 but is clearly before Win8. https://chromiumcodereview.appspot.com/10451074/diff/18001/chrome/installer/u... chrome/installer/util/shell_util.cc:402: RegistryStatus hkcu_status = DOES_NOT_EXIST; i think this shorter form is easier to read: RegistryStatus status = DOES_NOT_EXIST; if (look_in_hkcu) status = ...; if (status == DOES_NOT_EXIST && look_in_hklm) status = ...; return status == SAME_VALUE; i think it's also clean to do away with the look_in bools as well if you make look_for_in a bitfield again. https://chromiumcodereview.appspot.com/10451074/diff/18001/chrome/installer/u... chrome/installer/util/shell_util.cc:975: // User-level installs ProgId registrations can be found in HKCU (and values Note: this is only true for Chrome 20 onward. Any user-level install that was done prior to then has ProgId registrations in HKLM. Please make sure the design doc includes the history of what used to be where, as such configurations will need to be maintained (e.g., upgrades from pre-20 to post-20). https://chromiumcodereview.appspot.com/10451074/diff/18001/chrome/installer/u... chrome/installer/util/shell_util.cc:976: // there have priority over HKLM). The same is true for system entries as As of this CL, the system entries are always in HKLM. Please move this portion of the comment to the CL that changes that so that matters aren't confused here. In that other CL, I urge you to rename the "system entries" to something else to avoid confusion, as "system" implies system-wide, which means HKLM. https://chromiumcodereview.appspot.com/10451074/diff/18001/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/10451074/diff/18001/chrome/installer/u... chrome/installer/util/shell_util.h:226: // An enum used to tell CurrentUserIsRegisteredWithSuffix() which level of CurrentUserIsRegisteredWithSuffix -> QuickIsChromeRegistered https://chromiumcodereview.appspot.com/10451074/diff/18001/chrome/installer/u... chrome/installer/util/shell_util.h:228: enum RegistrationConfirmationLevel { chromium style: enums come before methods in a class. https://chromiumcodereview.appspot.com/10451074/diff/18001/chrome/installer/u... chrome/installer/util/shell_util.h:249: static bool QuickIsChromeRegistered( Looks like you can simplify things if you remove RegistrationConfirmationLevel and turn this into two functions that make the different checks. IsChromeBrowserRegistered() could be the name of the one declared here that checks the system registration. The variation that only checks the ProgID registration can be a private helper in the unnamed namespace that is used only by GetCurrentInstallationSuffix. https://chromiumcodereview.appspot.com/10451074/diff/25001/chrome/installer/s... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/10451074/diff/25001/chrome/installer/s... chrome/installer/setup/uninstall.cc:737: ShellUtil::QuickIsChromeRegistered( I don't like how this change puts more registration knowledge in uninstall.cc. What do you think about resurrecting something like ShellUtil::AdminNeededForRegistryCleanup (removed in r133333) that will be called from here with the necessary arguments. That way, uninstall.cc won't have to change in the future if/when there are other subtle changes to the HKCU/HKLM divide.
some more comments. i'll do my next pass after you've had a chance to process everything up to this point. cheers. http://codereview.chromium.org/10451074/diff/25001/chrome/browser/shell_integ... File chrome/browser/shell_integration_win.cc (right): http://codereview.chromium.org/10451074/diff/25001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:81: LOG(DFATAL) << "PathService::Get failed."; use plain-old NOTREACHED(); here since this is Chrome code. The release-mode logging here adds bytes to chrome.exe that no one will ever see. http://codereview.chromium.org/10451074/diff/25001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:112: LOG(DFATAL) << "PathService::Get failed."; NOTREACHED(); http://codereview.chromium.org/10451074/diff/25001/chrome/installer/setup/set... File chrome/installer/setup/setup_main.cc (right): http://codereview.chromium.org/10451074/diff/25001/chrome/installer/setup/set... chrome/installer/setup/setup_main.cc:729: chrome_exe); i don't think this |chrome_exe| is what you think it is. http://codereview.chromium.org/10451074/diff/25001/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/10451074/diff/25001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:801: // default through the UI)). egads! this comment looks like a lisp expression! :-)
Addressed comments, see below. Cheers, Gab http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:81: LOOK_IN_HKCU_THEN_HKLM, On 2012/06/08 16:24:13, grt wrote: > this makes the logic worse in the func. i meant leaving the first two as they > were and adding: > LOOK_IN_HKCU_THEN_HKLM = LOOK_IN_HKCU | LOOK_IN_HKLM, Ah ok, that's what I did originally, but felt it was slightly less consumer-friendly as it's a bitfield in which some flags have more than 1 bit and wasn't sure that was ok with Chromium-style. Done. http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:275: dist->GetAppShortCutName())); On 2012/06/08 16:24:13, grt wrote: > this seemingly trivial change will make IsChromeRegistered return false for all > existing installs, will it not? does the test plan in your design doc include > how to test for such breaking changes? IsChromeRegistered() returning false simply means registering again (for non-HKLM entries). My test matrix includes installing a new-style install over an old-style install so I think it's covered. http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:393: // registrations outside of HKCR on versions of Windows up to Win7, Chrome's On 2012/06/08 16:24:13, grt wrote: > "up to Win7" -> "prior to Win8" to reduce the ambiguity for Server 2008 R2, > which could be considered past Win7 but is clearly before Win8. Done. http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:402: RegistryStatus hkcu_status = DOES_NOT_EXIST; On 2012/06/08 16:24:13, grt wrote: > i think this shorter form is easier to read: > > RegistryStatus status = DOES_NOT_EXIST; > if (look_in_hkcu) > status = ...; > if (status == DOES_NOT_EXIST && look_in_hklm) > status = ...; > return status == SAME_VALUE; > > i think it's also clean to do away with the look_in bools as well if you make > look_for_in a bitfield again. Done. http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:975: // User-level installs ProgId registrations can be found in HKCU (and values On 2012/06/08 16:24:13, grt wrote: > Note: this is only true for Chrome 20 onward. Any user-level install that was > done prior to then has ProgId registrations in HKLM. Please make sure the > design doc includes the history of what used to be where, as such configurations > will need to be maintained (e.g., upgrades from pre-20 to post-20). Right, which is why I said "can be found" and not "are found". The old-style install section of the design doc also mentions this part for ProgIds. http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:976: // there have priority over HKLM). The same is true for system entries as On 2012/06/08 16:24:13, grt wrote: > As of this CL, the system entries are always in HKLM. Please move this portion > of the comment to the CL that changes that so that matters aren't confused here. > In that other CL, I urge you to rename the "system entries" to something else > to avoid confusion, as "system" implies system-wide, which means HKLM. I added it in this CL because it is still true that the values are *allowed* to be there and if they were would have priority over HKLM. This does not entail that they *are* there and simply falls through to the HKLM lookup if they aren't. I will make sure to rename SystemEntries in the HKCU CL. http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... File chrome/installer/util/shell_util.h (right): http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... chrome/installer/util/shell_util.h:226: // An enum used to tell CurrentUserIsRegisteredWithSuffix() which level of On 2012/06/08 16:24:13, grt wrote: > CurrentUserIsRegisteredWithSuffix -> QuickIsChromeRegistered Done. http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... chrome/installer/util/shell_util.h:228: enum RegistrationConfirmationLevel { On 2012/06/08 16:24:13, grt wrote: > chromium style: enums come before methods in a class. Done. http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... chrome/installer/util/shell_util.h:249: static bool QuickIsChromeRegistered( On 2012/06/08 16:24:13, grt wrote: > Looks like you can simplify things if you remove RegistrationConfirmationLevel > and turn this into two functions that make the different checks. > IsChromeBrowserRegistered() could be the name of the one declared here that > checks the system registration. The variation that only checks the ProgID > registration can be a private helper in the unnamed namespace that is used only > by GetCurrentInstallationSuffix. Thanks for pointing this out, it turns out that with all the rearrangements I've done this was now only called by uninstall.cc (apart from shell_util.cc itself), I made it a local function and made a new public function for the uninstall use as suggested in your uninstall.cc comments. The idea of a single function is that the code to ProgramCompare with HKCU preceding HKLM is common to both, only the path looked up is different; even if I was to split the function I would create a helper function that both functions call to make the final check (so that'd be 3 functions (maybe 4 given the special uninstall case) rather than 1 function + an enum). Given this is now a local function and the enum doesn't put burden on the public API anymore, I kept it as a single function for now, let me know if you think otherwise. http://codereview.chromium.org/10451074/diff/25001/chrome/browser/shell_integ... File chrome/browser/shell_integration_win.cc (right): http://codereview.chromium.org/10451074/diff/25001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:81: LOG(DFATAL) << "PathService::Get failed."; On 2012/06/08 17:51:46, grt wrote: > use plain-old NOTREACHED(); here since this is Chrome code. The release-mode > logging here adds bytes to chrome.exe that no one will ever see. Done. http://codereview.chromium.org/10451074/diff/25001/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:112: LOG(DFATAL) << "PathService::Get failed."; On 2012/06/08 17:51:46, grt wrote: > NOTREACHED(); Done. http://codereview.chromium.org/10451074/diff/25001/chrome/installer/setup/set... File chrome/installer/setup/setup_main.cc (right): http://codereview.chromium.org/10451074/diff/25001/chrome/installer/setup/set... chrome/installer/setup/setup_main.cc:729: chrome_exe); On 2012/06/08 17:51:46, grt wrote: > i don't think this |chrome_exe| is what you think it is. Ah right, it was quoted above, good catch, fixed... which leads us into the string16 dance... http://codereview.chromium.org/10451074/diff/25001/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/10451074/diff/25001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:737: ShellUtil::QuickIsChromeRegistered( On 2012/06/08 16:24:13, grt wrote: > I don't like how this change puts more registration knowledge in uninstall.cc. > What do you think about resurrecting something like > ShellUtil::AdminNeededForRegistryCleanup (removed in r133333) that will be > called from here with the necessary arguments. That way, uninstall.cc won't > have to change in the future if/when there are other subtle changes to the > HKCU/HKLM divide. Done (although I named the function QuickIsChromeRegisteredInHKLM() as it was not only used to determine elevation needs, but to determine the need to call DeleteChromeRegistrationKeys() and the original name didn't make sense as a condition there...). http://codereview.chromium.org/10451074/diff/25001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:801: // default through the UI)). On 2012/06/08 17:51:46, grt wrote: > egads! this comment looks like a lisp expression! :-) :), you're the one that asked for more details on the original comment ;)!
http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:275: dist->GetAppShortCutName())); On 2012/06/08 20:15:51, gab wrote: > On 2012/06/08 16:24:13, grt wrote: > > this seemingly trivial change will make IsChromeRegistered return false for > all > > existing installs, will it not? does the test plan in your design doc include > > how to test for such breaking changes? > > IsChromeRegistered() returning false simply means registering again (for > non-HKLM entries). Isn't it worse than that? What happens when a user tries to make Chrome default for a system-level install? Chrome is registered in HKLM by the installer, so a user should be able to make Chrome their default without UAC. I think a change that causes IsChromeRegistered() to return false will break this. > My test matrix includes installing a new-style install over an old-style install > so I think it's covered. To catch this, the test plan would have to include: 1) Install system-level old-style Chrome 2) Update to new-style Chrome 3) Visit chrome://chrome/settings/ and ensure that Chrome doesn't think it's the default browser 4) Click to make Chrome the default browser and ensure that Chrome is made the default without UAC http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:975: // User-level installs ProgId registrations can be found in HKCU (and values On 2012/06/08 20:15:51, gab wrote: > On 2012/06/08 16:24:13, grt wrote: > > Note: this is only true for Chrome 20 onward. Any user-level install that was > > done prior to then has ProgId registrations in HKLM. Please make sure the > > design doc includes the history of what used to be where, as such > configurations > > will need to be maintained (e.g., upgrades from pre-20 to post-20). > > Right, which is why I said "can be found" and not "are found". Ah, I see (note that "can be" is often used as a polite "are"). Would you mind making the comment more explicit? I know that three months from now I won't remember the subtlety here. http://codereview.chromium.org/10451074/diff/23003/chrome/installer/setup/set... File chrome/installer/setup/setup_main.cc (right): http://codereview.chromium.org/10451074/diff/23003/chrome/installer/setup/set... chrome/installer/setup/setup_main.cc:728: installer::RemoveChromeLegacyRegistryKeys(chrome->distribution(), since chrome_exe is set deep in conditionals up above, would you add a DCHECK_NE(chrome_exe, string16()); here just in case? it takes a bit of reading be convinced that it's always set when this is reached. http://codereview.chromium.org/10451074/diff/23003/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/10451074/diff/23003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:737: ShellUtil::QuickIsChromeRegisteredInHKLM(browser_dist, nit: Chromium style encourages wrapping function calls to save vertical space. either of the following would be preferred here: ShellUtil::QuickIsChromeRegisteredInHKLM(browser_dist, chrome_exe, suffix) && or ShellUtil::QuickIsChromeRegisteredInHKLM( browser_dist, chrome_exe, suffix) && http://codereview.chromium.org/10451074/diff/23003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:823: ShellUtil::QuickIsChromeRegisteredInHKLM(browser_dist, same wrapping nit here
http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:275: dist->GetAppShortCutName())); On 2012/06/11 17:28:57, grt wrote: > On 2012/06/08 20:15:51, gab wrote: > > On 2012/06/08 16:24:13, grt wrote: > > > this seemingly trivial change will make IsChromeRegistered return false for > > all > > > existing installs, will it not? does the test plan in your design doc > include > > > how to test for such breaking changes? > > > > IsChromeRegistered() returning false simply means registering again (for > > non-HKLM entries). > > Isn't it worse than that? What happens when a user tries to make Chrome default > for a system-level install? Chrome is registered in HKLM by the installer, so a > user should be able to make Chrome their default without UAC. I think a change > that causes IsChromeRegistered() to return false will break this. Ah, but the subtlety continues: RegisterChromeBrowser() is called on each update, so the new value will be written for an existing install. Is this correct? If so, I think this is safe.
Comments addressed :)! Cheers, Gab http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:275: dist->GetAppShortCutName())); On 2012/06/11 17:28:57, grt wrote: > On 2012/06/08 20:15:51, gab wrote: > > On 2012/06/08 16:24:13, grt wrote: > > > this seemingly trivial change will make IsChromeRegistered return false for > > all > > > existing installs, will it not? does the test plan in your design doc > include > > > how to test for such breaking changes? > > > > IsChromeRegistered() returning false simply means registering again (for > > non-HKLM entries). > > Isn't it worse than that? What happens when a user tries to make Chrome default > for a system-level install? Chrome is registered in HKLM by the installer, so a > user should be able to make Chrome their default without UAC. I think a change > that causes IsChromeRegistered() to return false will break this. > > > My test matrix includes installing a new-style install over an old-style > install > > so I think it's covered. > > To catch this, the test plan would have to include: > 1) Install system-level old-style Chrome > 2) Update to new-style Chrome > 3) Visit chrome://chrome/settings/ and ensure that Chrome doesn't think it's the > default browser > 4) Click to make Chrome the default browser and ensure that Chrome is made the > default without UAC Ack. http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:275: dist->GetAppShortCutName())); On 2012/06/11 17:49:07, grt wrote: > On 2012/06/11 17:28:57, grt wrote: > > On 2012/06/08 20:15:51, gab wrote: > > > On 2012/06/08 16:24:13, grt wrote: > > > > this seemingly trivial change will make IsChromeRegistered return false > for > > > all > > > > existing installs, will it not? does the test plan in your design doc > > include > > > > how to test for such breaking changes? > > > > > > IsChromeRegistered() returning false simply means registering again (for > > > non-HKLM entries). > > > > Isn't it worse than that? What happens when a user tries to make Chrome > default > > for a system-level install? Chrome is registered in HKLM by the installer, so > a > > user should be able to make Chrome their default without UAC. I think a > change > > that causes IsChromeRegistered() to return false will break this. > > Ah, but the subtlety continues: RegisterChromeBrowser() is called on each > update, so the new value will be written for an existing install. Is this > correct? If so, I think this is safe. As discussed this will be fine, the worst case is a UAC for user-level installs when the user makes Chrome default a second time (i.e. having done it the first time with the other registration). We decided it is OK to have this one-time UAC. We agreed that IsChromeRegistered() is essentially wrong, but fixing this is beyond the scope of this patched. http://codereview.chromium.org/10451074/diff/18001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:975: // User-level installs ProgId registrations can be found in HKCU (and values On 2012/06/11 17:28:57, grt wrote: > On 2012/06/08 20:15:51, gab wrote: > > On 2012/06/08 16:24:13, grt wrote: > > > Note: this is only true for Chrome 20 onward. Any user-level install that > was > > > done prior to then has ProgId registrations in HKLM. Please make sure the > > > design doc includes the history of what used to be where, as such > > configurations > > > will need to be maintained (e.g., upgrades from pre-20 to post-20). > > > > Right, which is why I said "can be found" and not "are found". > > Ah, I see (note that "can be" is often used as a polite "are"). Would you mind > making the comment more explicit? I know that three months from now I won't > remember the subtlety here. Done. http://codereview.chromium.org/10451074/diff/23003/chrome/installer/setup/set... File chrome/installer/setup/setup_main.cc (right): http://codereview.chromium.org/10451074/diff/23003/chrome/installer/setup/set... chrome/installer/setup/setup_main.cc:728: installer::RemoveChromeLegacyRegistryKeys(chrome->distribution(), On 2012/06/11 17:28:57, grt wrote: > since chrome_exe is set deep in conditionals up above, would you add a > DCHECK_NE(chrome_exe, string16()); here just in case? it takes a bit of reading > be convinced that it's always set when this is reached. Done. http://codereview.chromium.org/10451074/diff/23003/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/10451074/diff/23003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:737: ShellUtil::QuickIsChromeRegisteredInHKLM(browser_dist, On 2012/06/11 17:28:57, grt wrote: > nit: Chromium style encourages wrapping function calls to save vertical space. > either of the following would be preferred here: > ShellUtil::QuickIsChromeRegisteredInHKLM(browser_dist, chrome_exe, > suffix) && > or > ShellUtil::QuickIsChromeRegisteredInHKLM( > browser_dist, chrome_exe, suffix) && Done. http://codereview.chromium.org/10451074/diff/23003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:823: ShellUtil::QuickIsChromeRegisteredInHKLM(browser_dist, On 2012/06/11 17:28:57, grt wrote: > same wrapping nit here Done.
lgtm w/ 1 nit http://codereview.chromium.org/10451074/diff/18015/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/10451074/diff/18015/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:823: browser_dist, chrome_exe, suffix))) { nit: looks like this needs one more space to be 4-space indented from the line above
Nit fixed. Waiting for testing of all of: Always suffix ChromeHTML entries on Windows for user-level installs => this patch -- Abstract suffixing logic away from GetApplicationName => http://codereview.chromium.org/10446111/ -- Make all registrations in HKCU for user-level installs on Win8 => http://codereview.chromium.org/10399054/ -- Suffix Chrome's appid on user-level installs => http://codereview.chromium.org/10542031/ to be complete before committing to make sure they all go on the same build (and that we don't get any suffixed ChromeHTML+appname, but not appid... out there!) http://codereview.chromium.org/10451074/diff/18015/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/10451074/diff/18015/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:823: browser_dist, chrome_exe, suffix))) { On 2012/06/13 17:31:47, grt wrote: > nit: looks like this needs one more space to be 4-space indented from the line > above Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gab@chromium.org/10451074/31001
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gab@chromium.org/10451074/31001
Change committed as 142634 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
