Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10960058:
Remove system-level Chrome ProgIDs from filetype associations at uninstall. (Closed)
|
Created:
8 years, 3 months ago by grt (UTC plus 2) Modified:
8 years, 2 months ago CC:
chromium-reviews, grt+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRemove system-level Chrome ProgIDs from filetype associations at uninstall.
BUG=149089
TEST=install system-level Chrome, make it the default, uninstall it, run One Note, click a link. IE should open.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158661
Patch Set 1 #Patch Set 2 : restore defaults #
Total comments: 2
Patch Set 3 : clarity for robertshield #
Total comments: 12
Patch Set 4 : thanks gab #Messages
Total messages: 11 (0 generated)
Gents, I'm sending this out for review even though I'm not terribly happy with it. As it turns out, IE never restores its filetype registration in HKLM. This means that even making IE the default doesn't bring Office apps back to life after a made-default-at-install system-level Chrome is uninstalled. Sigh. As far as I can tell, there's nothing we can look up in the registry to find the original settings, so I've taken the ugly path of baking them into the uninstall logic. I don't really like the way this looks, nor do I like the look of the other half-dozen ways I could think of. So please, if you'd be so kind, give me your thoughts.
Icky, but looks like it ought to work. One main question: do we still need to do this if on uninstall we successfully set some other browser, say FF, as default as a result of the uninstall prompt? https://codereview.chromium.org/10960058/diff/4001/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/10960058/diff/4001/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:659: const wchar_t* replacement = NULL; for clarity, replacement -> replacement_prog_id
Excellent point about setting another browser as default. chrome/browser/ui/views/uninstall_view.cc doesn't wait for the other browser's ReinstallCommand (http://msdn.microsoft.com/library/windows/desktop/cc144109.aspx#reinstall_com...) to complete. It should. I'll do this in a separate CL. https://chromiumcodereview.appspot.com/10960058/diff/4001/chrome/installer/se... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/10960058/diff/4001/chrome/installer/se... chrome/installer/setup/uninstall.cc:659: const wchar_t* replacement = NULL; On 2012/09/25 12:53:05, robertshield wrote: > for clarity, replacement -> replacement_prog_id Done.
Better than nothing... Comments below. Cheers, Gab https://codereview.chromium.org/10960058/diff/6009/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/10960058/diff/6009/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:645: root, (classes_path + *filetype).c_str(), L"", nit: Indent 1 more space here and below. https://codereview.chromium.org/10960058/diff/6009/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:647: cleared_assocs.push_back(*filetype); Do we want to do the replacement in HKCU as well or only HKLM? Seems to me in HKCU we can simply delete and let it fall back to HKLM default. https://codereview.chromium.org/10960058/diff/6009/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:678: key.WriteValue(NULL, replacement_prog_id) != ERROR_SUCCESS) { Here you use NULL and above (:645) you use L"" to represent the default value. I think I prefer NULL, but either way please choose one and use it consistently (maybe make a static const variable to indicate this is the Default value?... I know it took me some time to figure out what NULL/L"" meant in both scenarios (i.e. had to go lookup method definition in both cases)). https://codereview.chromium.org/10960058/diff/6009/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:682: } else if (replacement_prog_id) { I don't like to have to recheck this condition here, what about: if (replacement_prog_id) { if (key.Open(...replacement_prog_id) == ERROR_SUCCESS && (key.Open(...assoc) != ERROR_SUCCESS || key.WriteValue(...) != ERROR_SUCCESS)) { LOG(ERROR) ... } } else { LOG(WARNING) ... }
thanks, ptal. https://codereview.chromium.org/10960058/diff/6009/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/10960058/diff/6009/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:645: root, (classes_path + *filetype).c_str(), L"", On 2012/09/25 14:58:32, gab wrote: > nit: Indent 1 more space here and below. it's currently indented four spaces from the 'I' above. isn't this correct? https://codereview.chromium.org/10960058/diff/6009/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:647: cleared_assocs.push_back(*filetype); On 2012/09/25 14:58:32, gab wrote: > Do we want to do the replacement in HKCU as well or only HKLM? > > Seems to me in HKCU we can simply delete and let it fall back to HKLM default. only in HKLM. in HKCU we delete Chrome's value but do not replace it. https://codereview.chromium.org/10960058/diff/6009/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:678: key.WriteValue(NULL, replacement_prog_id) != ERROR_SUCCESS) { On 2012/09/25 14:58:32, gab wrote: > Here you use NULL and above (:645) you use L"" to represent the default value. > > I think I prefer NULL, but either way please choose one and use it consistently > (maybe make a static const variable to indicate this is the Default value?... I > know it took me some time to figure out what NULL/L"" meant in both scenarios > (i.e. had to go lookup method definition in both cases)). addressing by fixing DeleteRegistryValueIf in a separate change, stay tuned. https://codereview.chromium.org/10960058/diff/6009/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:682: } else if (replacement_prog_id) { On 2012/09/25 14:58:32, gab wrote: > I don't like to have to recheck this condition here, what about: > > if (replacement_prog_id) { > if (key.Open(...replacement_prog_id) == ERROR_SUCCESS && > (key.Open(...assoc) != ERROR_SUCCESS || > key.WriteValue(...) != ERROR_SUCCESS)) { > LOG(ERROR) ... > } > } else { > LOG(WARNING) ... > } rejiggered.
looks good, one more comment about the logic when deleting HKCU entries... I still think it's wrong. https://codereview.chromium.org/10960058/diff/6009/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/10960058/diff/6009/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:645: root, (classes_path + *filetype).c_str(), L"", On 2012/09/25 15:56:21, grt wrote: > On 2012/09/25 14:58:32, gab wrote: > > nit: Indent 1 more space here and below. > > it's currently indented four spaces from the 'I' above. isn't this correct? I can't count... https://codereview.chromium.org/10960058/diff/6009/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:647: cleared_assocs.push_back(*filetype); On 2012/09/25 15:56:21, grt wrote: > On 2012/09/25 14:58:32, gab wrote: > > Do we want to do the replacement in HKCU as well or only HKLM? > > > > Seems to me in HKCU we can simply delete and let it fall back to HKLM default. > > only in HKLM. in HKCU we delete Chrome's value but do not replace it. Right, hmmm well looks to me like this code populates |cleared_assocs| when deleting values in HKCU as well... no? In which case the code below swaps those values out in HKLM (which is actually even worse...).
Back to you, Gab! https://codereview.chromium.org/10960058/diff/6009/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/10960058/diff/6009/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:647: cleared_assocs.push_back(*filetype); On 2012/09/25 17:09:00, gab wrote: > On 2012/09/25 15:56:21, grt wrote: > > On 2012/09/25 14:58:32, gab wrote: > > > Do we want to do the replacement in HKCU as well or only HKLM? > > > > > > Seems to me in HKCU we can simply delete and let it fall back to HKLM > default. > > > > only in HKLM. in HKCU we delete Chrome's value but do not replace it. > > Right, hmmm well looks to me like this code populates |cleared_assocs| when > deleting values in HKCU as well... no? Correct. > In which case the code below swaps those values out in HKLM (which is actually > even worse...). I don't think so. The code below only does the replacing when the function is operating in HKLM; see line 656. Which one of us is missing something?! :-)
Duh! LGTM https://codereview.chromium.org/10960058/diff/6009/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/10960058/diff/6009/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:647: cleared_assocs.push_back(*filetype); On 2012/09/25 18:44:35, grt wrote: > On 2012/09/25 17:09:00, gab wrote: > > On 2012/09/25 15:56:21, grt wrote: > > > On 2012/09/25 14:58:32, gab wrote: > > > > Do we want to do the replacement in HKCU as well or only HKLM? > > > > > > > > Seems to me in HKCU we can simply delete and let it fall back to HKLM > > default. > > > > > > only in HKLM. in HKCU we delete Chrome's value but do not replace it. > > > > Right, hmmm well looks to me like this code populates |cleared_assocs| when > > deleting values in HKCU as well... no? > > Correct. > > > In which case the code below swaps those values out in HKLM (which is actually > > even worse...). > > I don't think so. The code below only does the replacing when the function is > operating in HKLM; see line 656. Which one of us is missing something?! :-) Indeed, my bad, read passed the "if (root == HKEY_LOCAL_MACHINE)" line and then the rest of the replacement block obviously has no further checks for this... optional: What do you think of also not populating |cleared_assocs| for no reason when operating in HKCU?
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grt@chromium.org/10960058/12009
Change committed as 158661 |
