Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11413029:
Uninstall Chrome + has App Host + not App Launcher => Uninstall App Host. (Closed)
|
Created:
8 years, 1 month ago by huangs Modified:
8 years ago CC:
chromium-reviews, grt+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUninstall Chrome + has App Host + not App Launcher => Uninstall App Host.
BUG=151626, 160850
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151626
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fixing comments; cleanups; checking current level instead of forced user level. #
Total comments: 4
Patch Set 3 : Imitating similar code for Chrome Frame. #
Total comments: 6
Patch Set 4 : Refactoring code. #
Total comments: 8
Patch Set 5 : Fixing comments and spacing. #Messages
Total messages: 12 (0 generated)
Some known caveats are as follows: - After installation, chrome.exe and Chrome binaries linger. But this also happens with Chrome and no App Host stuff. - App shortcuts do not get deleted on uninstall. - This CL is only for user-level, i.e., it won't work for system-level. I think this issue should be bundled with the other problems involving system-level Chrome + App Host (i.e., cannot create app shortcuts; user-level App Host missing DLLs.) PTAL.
https://codereview.chromium.org/11413029/diff/1/chrome/installer/util/install... File chrome/installer/util/installer_state.cc (right): https://codereview.chromium.org/11413029/diff/1/chrome/installer/util/install... chrome/installer/util/installer_state.cc:140: bool system_level_app_host = false; I don't see the merit of this boolean variable, nor fixing it to false. I think it's OK to go ahead and look at the same level from which we are uninstalling Chrome. Do not uninstall the app host if this is a user-level uninstall and a system-level Chrome is present.
https://codereview.chromium.org/11413029/diff/1/chrome/installer/util/install... File chrome/installer/util/installer_state.cc (right): https://codereview.chromium.org/11413029/diff/1/chrome/installer/util/install... chrome/installer/util/installer_state.cc:136: // If Chrome is being uninstalled, and (non-App Launcher) App Host exists, suggest "Uninstall App Host if Chrome is being uninstalled and (non-App Launcher) App Host exists." https://codereview.chromium.org/11413029/diff/1/chrome/installer/util/install... chrome/installer/util/installer_state.cc:136: // If Chrome is being uninstalled, and (non-App Launcher) App Host exists, please consider if this logic belongs in the "if (is_uninstall && prefs.is_multi_install())" block below (i think it does). https://codereview.chromium.org/11413029/diff/1/chrome/installer/util/install... chrome/installer/util/installer_state.cc:144: !CommandLine(app_host_state->uninstall_command()) !app_host_state->uninstall_command().HasSwitch(...)
PTAL. https://chromiumcodereview.appspot.com/11413029/diff/1/chrome/installer/util/... File chrome/installer/util/installer_state.cc (right): https://chromiumcodereview.appspot.com/11413029/diff/1/chrome/installer/util/... chrome/installer/util/installer_state.cc:136: // If Chrome is being uninstalled, and (non-App Launcher) App Host exists, On 2012/11/16 04:25:17, grt wrote: > suggest "Uninstall App Host if Chrome is being uninstalled and (non-App > Launcher) App Host exists." Done. Added more comments regarding mix of system/user levels. https://chromiumcodereview.appspot.com/11413029/diff/1/chrome/installer/util/... chrome/installer/util/installer_state.cc:136: // If Chrome is being uninstalled, and (non-App Launcher) App Host exists, On 2012/11/16 04:25:17, grt wrote: > please consider if this logic belongs in the "if (is_uninstall && > prefs.is_multi_install())" block below (i think it does). Done, but with slight code duplication. https://chromiumcodereview.appspot.com/11413029/diff/1/chrome/installer/util/... chrome/installer/util/installer_state.cc:140: bool system_level_app_host = false; On 2012/11/16 02:59:19, erikwright wrote: > I don't see the merit of this boolean variable, nor fixing it to false. I think > it's OK to go ahead and look at the same level from which we are uninstalling > Chrome. > > Do not uninstall the app host if this is a user-level uninstall and a > system-level Chrome is present. Done. https://chromiumcodereview.appspot.com/11413029/diff/1/chrome/installer/util/... chrome/installer/util/installer_state.cc:144: !CommandLine(app_host_state->uninstall_command()) On 2012/11/16 04:25:17, grt wrote: > !app_host_state->uninstall_command().HasSwitch(...) Done. Also fixing same goof in install.cc
https://chromiumcodereview.appspot.com/11413029/diff/4001/chrome/installer/ut... File chrome/installer/util/installer_state.cc (right): https://chromiumcodereview.appspot.com/11413029/diff/4001/chrome/installer/ut... chrome/installer/util/installer_state.cc:122: Product* p = AddProductFromPreferences( reformatting like this makes it more difficult to review the changes. in general, please don't reformat code that isn't wildly inconsistent with the style guide unless you're touching it for other reasons. https://chromiumcodereview.appspot.com/11413029/diff/4001/chrome/installer/ut... chrome/installer/util/installer_state.cc:180: if (is_uninstall && prefs.install_chrome() - is_uninstall is tested on line 175, so please remove. - chrome being uninstalled is tested on line 194, so don't do that again here. instead, put this new code inside that conditional. - i suggest you follow the pattern here and use FindProduct(BrowserDistribution::CHROME_APP_HOST) instead of checking prefs.
PTAL. https://chromiumcodereview.appspot.com/11413029/diff/4001/chrome/installer/ut... File chrome/installer/util/installer_state.cc (right): https://chromiumcodereview.appspot.com/11413029/diff/4001/chrome/installer/ut... chrome/installer/util/installer_state.cc:122: Product* p = AddProductFromPreferences( Okay, will avoid in future. https://chromiumcodereview.appspot.com/11413029/diff/4001/chrome/installer/ut... chrome/installer/util/installer_state.cc:180: if (is_uninstall && prefs.install_chrome() On 2012/11/16 21:01:46, grt wrote: > - is_uninstall is tested on line 175, so please remove. > - chrome being uninstalled is tested on line 194, so don't do that again here. > instead, put this new code inside that conditional. > - i suggest you follow the pattern here and use > FindProduct(BrowserDistribution::CHROME_APP_HOST) instead of checking prefs. Done.
Comments and questions below. Cheers, Gab https://codereview.chromium.org/11413029/diff/2004/chrome/installer/util/inst... File chrome/installer/util/installer_state.cc (right): https://codereview.chromium.org/11413029/diff/2004/chrome/installer/util/inst... chrome/installer/util/installer_state.cc:180: if (chrome_frame_state != NULL && This is extremely similar to the code you added below (given it says the same after applying comments), you can avoid duplicating code without creating a new method (as creating a method would hinder readability here I feel) by doing something like (see similar implementation in shell_util.cc:GetProgIdEntries()): // Uninstall each product of type |type| listed below based on the presence or absence of |switch| in that product's uninstall command. const struct { BrowserDistribution::Type type; const char* switch, bool switch_expected; } conditional_additions[] = { // If Chrome Frame is installed in Ready Mode. Remove it along with Chrome. { BrowserDistribution::CHROME_FRAME, switches::kChromeFrameReadyMode, true }, // your comment about the app host.... { BrowserDistribution::CHROME_APP_HOST, switches::kChromeAppLauncher, false }, }; for (size_t i = 0; i < arraysize(conditional_additions); ++i) { // common implementation. } https://codereview.chromium.org/11413029/diff/2004/chrome/installer/util/inst... chrome/installer/util/installer_state.cc:191: const ProductState* app_host_state = machine_state.GetProductState( Rename variable to make it explicit this is system-level state, sys_app_host_state? Helps readability I think, I wasn't sure why this was the case until I looked at the code more (same for chrome_frame_state above please). https://codereview.chromium.org/11413029/diff/2004/chrome/installer/util/inst... chrome/installer/util/installer_state.cc:201: // When executed, orphaned app_host.exe prompts user for further action. What about when uninstalling user-level Chrome with no user-levle app launcher present?
PTAL. https://chromiumcodereview.appspot.com/11413029/diff/2004/chrome/installer/ut... File chrome/installer/util/installer_state.cc (right): https://chromiumcodereview.appspot.com/11413029/diff/2004/chrome/installer/ut... chrome/installer/util/installer_state.cc:180: if (chrome_frame_state != NULL && On 2012/11/16 22:20:26, gab wrote: > This is extremely similar to the code you added below (given it says the same > after applying comments), you can avoid duplicating code without creating a new > method (as creating a method would hinder readability here I feel) by doing > something like (see similar implementation in shell_util.cc:GetProgIdEntries()): > > // Uninstall each product of type |type| listed below based on the presence or > absence of |switch| in that product's uninstall command. > const struct { > BrowserDistribution::Type type; > const char* switch, > bool switch_expected; > } conditional_additions[] = { > // If Chrome Frame is installed in Ready Mode. Remove it along with Chrome. > { BrowserDistribution::CHROME_FRAME, switches::kChromeFrameReadyMode, true }, > // your comment about the app host.... > { BrowserDistribution::CHROME_APP_HOST, switches::kChromeAppLauncher, false }, > }; > > for (size_t i = 0; i < arraysize(conditional_additions); ++i) { > // common implementation. > } Done. https://chromiumcodereview.appspot.com/11413029/diff/2004/chrome/installer/ut... chrome/installer/util/installer_state.cc:191: const ProductState* app_host_state = machine_state.GetProductState( Disregarding, per F2F. https://chromiumcodereview.appspot.com/11413029/diff/2004/chrome/installer/ut... chrome/installer/util/installer_state.cc:201: // When executed, orphaned app_host.exe prompts user for further action. On 2012/11/16 22:20:26, gab wrote: > What about when uninstalling user-level Chrome with no user-levle app launcher > present? In this case, app_host_state will be NULL, and the block will be ignored.
LGTM w/ nits. https://chromiumcodereview.appspot.com/11413029/diff/8001/chrome/installer/ut... File chrome/installer/util/installer_state.cc (right): https://chromiumcodereview.appspot.com/11413029/diff/8001/chrome/installer/ut... chrome/installer/util/installer_state.cc:187: true }, 1) This "true" should be aligned with "BrowserDistribution" above. optional: 2) I would prefer/think it's more readable if each parameter was one it's own line (since it doesn't fit on one line), i.e.: { BrowserDistribution::CHROME_FRAME, switches::kChromeFrameReadyMode, true }, optional: 3) Or even with the brackets on their own line, but that kind of goes against the code-style of minimizing vertical space (while keeping readability). https://chromiumcodereview.appspot.com/11413029/diff/8001/chrome/installer/ut... chrome/installer/util/installer_state.cc:191: // app_host.exe is executed, it prompts user for further action. What about: // If the App Host is installed, but not the App Launcher. Remove it with Chrome. // Note however that for system-level Chrome uninstalls, any installed user-level // App Host will remain even if there is no App Launcher present (the orphaned // app_host.exe will prompt the user for further action when executed). https://chromiumcodereview.appspot.com/11413029/diff/8001/chrome/installer/ut... chrome/installer/util/installer_state.cc:193: false }, same as above. https://chromiumcodereview.appspot.com/11413029/diff/8001/chrome/installer/ut... chrome/installer/util/installer_state.cc:202: conditional_additions[i].switch_expected && nit: indent 4 more spaces.
https://chromiumcodereview.appspot.com/11413029/diff/8001/chrome/installer/ut... File chrome/installer/util/installer_state.cc (right): https://chromiumcodereview.appspot.com/11413029/diff/8001/chrome/installer/ut... chrome/installer/util/installer_state.cc:187: true }, On 2012/11/19 17:24:38, gab wrote: > 1) This "true" should be aligned with "BrowserDistribution" above. > > optional: 2) I would prefer/think it's more readable if each parameter was one > it's own line (since it doesn't fit on one line), i.e.: > { BrowserDistribution::CHROME_FRAME, > switches::kChromeFrameReadyMode, > true }, > > optional: 3) Or even with the brackets on their own line, but that kind of goes > against the code-style of minimizing vertical space (while keeping readability). Done. Did 1) and 2) only. https://chromiumcodereview.appspot.com/11413029/diff/8001/chrome/installer/ut... chrome/installer/util/installer_state.cc:191: // app_host.exe is executed, it prompts user for further action. On 2012/11/19 17:24:38, gab wrote: > What about: > > // If the App Host is installed, but not the App Launcher. Remove it with > Chrome. > // Note however that for system-level Chrome uninstalls, any installed > user-level > // App Host will remain even if there is no App Launcher present (the orphaned > // app_host.exe will prompt the user for further action when executed). Done. https://chromiumcodereview.appspot.com/11413029/diff/8001/chrome/installer/ut... chrome/installer/util/installer_state.cc:193: false }, On 2012/11/19 17:24:38, gab wrote: > same as above. Done. https://chromiumcodereview.appspot.com/11413029/diff/8001/chrome/installer/ut... chrome/installer/util/installer_state.cc:202: conditional_additions[i].switch_expected && On 2012/11/19 17:24:38, gab wrote: > nit: indent 4 more spaces. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/11413029/1007
Change committed as 168619 |
