Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10889028:
Install a user-level Start Menu shortcut for every user on system-installs through Active Setup (Closed)
|
Created:
8 years, 3 months ago by gab Modified:
8 years, 1 month ago CC:
chromium-reviews, grt+watch_chromium.org, grt (UTC plus 2), chrome-win8-eng_google.com Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionInstall a user-level Start Menu shortcut for every user on system-installs through Active Setup
Re-enable Active Setup
This is a hack to make this work while the bigger refactoring is under way (http://goo.gl/Az889).
BUG=132825
TEST=Ensure Start Menu tile appears for the installing user and for subsequent users logging in.
Ensure no duplicate in Start Menu on all versions of Windows.
Patch Set 1 #
Total comments: 16
Patch Set 2 : address comments #Patch Set 3 : comments + exit code + create shell notify #
Total comments: 31
Patch Set 4 : grt comments #
Total comments: 10
Patch Set 5 : merge with r153166 #Patch Set 6 : grt comments and better InstallStatus #Patch Set 7 : DFATAL and TODO #Patch Set 8 : initialization at top #Patch Set 9 : oops build failures! #Patch Set 10 : remove TODO for Notify #
Total comments: 1
Messages
Total messages: 19 (0 generated)
Good news, shadowing the global shortcut works on Win8 (yet to try other Windows). Weird news, I accidently tried a build without this change and it appears the global shortcut *was* showing up for the installing user and for another user... Maybe that's because I've used those shortcuts before... more experimentation is required. Another good news, the "Version" field of Active Setup doesn't seem to show up in the Active Setup dialog it just says "Google Chrome" (I happened to mess up something and have it stall on this dialog so I could read it.. usually it's too fast for the dialog to even show anyways).
Code lgtm, please test manually and ask QA to check it out when it's in. http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/install... File chrome/installer/setup/install_worker.cc (right): http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/install... chrome/installer/setup/install_worker.cc:1277: static const wchar_t* kActiveSetupVersion = L"23,0,0,0"; Please move this to the top of the file, also explain what Active Setup is in the comment.
http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/install.cc File chrome/installer/setup/install.cc (right): http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/install... chrome/installer/setup/install.cc:361: if (!file_util::PathExists(start_menu_user_level)) no need for this check, as CreateDirectory does it for you anyway. http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/install... chrome/installer/setup/install.cc:362: file_util::CreateDirectoryW(start_menu_user_level); CreateDirectoryW -> CreateDirectory http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/install... chrome/installer/setup/install.cc:375: LOG(WARNING) << "Creating shortcut at " << chrome_link.value() WARNING -> ERROR? http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/install.h File chrome/installer/setup/install.h (right): http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/install... chrome/installer/setup/install.h:56: // TODO(gab): This is a hack that is temporarily required for the Start Menu this is a doc comment for the function so that consumers know what it does and how to use it. i don't think mention of a refactor belongs here. http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/install... File chrome/installer/setup/install_worker.cc (right): http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/install... chrome/installer/setup/install_worker.cc:1258: CommandLine cmd(installer_state.GetInstallerDirectory(new_version). i think you should add --system-level to the command-line so that setup.exe knows it's running for a system-level install. this impacts how things like the InstallerState are initialized. i also think you should use product.AppendProductFlags so that --multi-install and --chrome are added if it's a multi-install. http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/install... chrome/installer/setup/install_worker.cc:1274: // that have already ran it, simply increase this number. ran -> run http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/setup_m... File chrome/installer/setup/setup_main.cc (right): http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/setup_m... chrome/installer/setup/setup_main.cc:1098: // Create user-level Start Menu shortcuts. i think it'd be more clear to refer to "per-user" Start Menu shortcuts rather than "user-level" ones since the latter term is ordinarily applied to a mode of installing Chrome as a whole.
Addressed comments, but I've been testing for the rest of the afternoon and it doesn't seem to work on a new machine (i.e. my usual test VM works, but has some leftover state). From a fresh 8400 box it doesn't work (i.e. new users don't have a tile although the all-users AND per-user shortcut are installed). I'm confused by what makes the tile appear now. It appears on the installing user, but I'm not clear what's different between the installing user and the one running setup.exe through Active Setup (he is non-admin... maybe that's it). In the end I'm not convinced this CL does any good as is... http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/install.cc File chrome/installer/setup/install.cc (right): http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/install... chrome/installer/setup/install.cc:361: if (!file_util::PathExists(start_menu_user_level)) On 2012/08/29 17:07:06, grt wrote: > no need for this check, as CreateDirectory does it for you anyway. Done. http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/install... chrome/installer/setup/install.cc:362: file_util::CreateDirectoryW(start_menu_user_level); On 2012/08/29 17:07:06, grt wrote: > CreateDirectoryW -> CreateDirectory Done. http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/install... chrome/installer/setup/install.cc:375: LOG(WARNING) << "Creating shortcut at " << chrome_link.value() On 2012/08/29 17:07:06, grt wrote: > WARNING -> ERROR? Done. http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/install.h File chrome/installer/setup/install.h (right): http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/install... chrome/installer/setup/install.h:56: // TODO(gab): This is a hack that is temporarily required for the Start Menu On 2012/08/29 17:07:06, grt wrote: > this is a doc comment for the function so that consumers know what it does and > how to use it. i don't think mention of a refactor belongs here. Done. http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/install... File chrome/installer/setup/install_worker.cc (right): http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/install... chrome/installer/setup/install_worker.cc:1258: CommandLine cmd(installer_state.GetInstallerDirectory(new_version). On 2012/08/29 17:07:06, grt wrote: > i think you should add --system-level to the command-line so that setup.exe > knows it's running for a system-level install. this impacts how things like the > InstallerState are initialized. i also think you should use > product.AppendProductFlags so that --multi-install and --chrome are added if > it's a multi-install. Done already :). http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/install... chrome/installer/setup/install_worker.cc:1274: // that have already ran it, simply increase this number. On 2012/08/29 17:07:06, grt wrote: > ran -> run Done. http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/install... chrome/installer/setup/install_worker.cc:1277: static const wchar_t* kActiveSetupVersion = L"23,0,0,0"; On 2012/08/29 16:59:30, robertshield wrote: > Please move this to the top of the file, also explain what Active Setup is in > the comment. Done. http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/setup_m... File chrome/installer/setup/setup_main.cc (right): http://codereview.chromium.org/10889028/diff/1/chrome/installer/setup/setup_m... chrome/installer/setup/setup_main.cc:1098: // Create user-level Start Menu shortcuts. On 2012/08/29 17:07:06, grt wrote: > i think it'd be more clear to refer to "per-user" Start Menu shortcuts rather > than "user-level" ones since the latter term is ordinarily applied to a mode of > installing Chrome as a whole. I agree, I'll start using this and update documentation as I see it in upcoming CLs.
Few more things. PTAL. Thanks, Gab
did it work? https://chromiumcodereview.appspot.com/10889028/diff/11001/base/file_util_win.cc File base/file_util_win.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/11001/base/file_util_win... base/file_util_win.cc:460: else add braces here and to the if side unless you remove the comment below https://chromiumcodereview.appspot.com/10889028/diff/11001/base/file_util_win... base/file_util_win.cc:461: // TODO(gab): Only use SHCNE_ASSOCCHANGED when the icon changed. is it even possible to put icon change detection logic down here? i don't think it is, in which case i don't think this TODO belongs. https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/install.cc:318: if (create_always && installer_state.system_install()) { nit: remove braces https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/install.cc:358: FilePath start_menu_user_level; start_menu_user_level -> user_start_menu or per_user_start_menu https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/install.cc:360: LOG(ERROR) << "Failed to get user-level start menu path."; i like DFATAL here since failure is extremely unexpected https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/install.cc:364: file_util::CreateDirectory(start_menu_user_level); y u no handle result? i suppose you could argue that the code below would error out properly if the directory didn't exist, so it's okay. please add a comment to that effect if that's your intention. https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/install.cc:378: << " failed."; nit: indentation (does this fit on the previous line? https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/install_worker.cc:52: // This is not the Chrome version. To have Active Setup (a method Chrome please reword the comment so that it begins by telling the reader what this constant is. follow this by an explanation of if/when it should change. something like: The version identifying the work done by setup.exe --configure-user-settings on user login by way of Active Setup. Increase this value if the work done in setup_main.cc's handling of kConfigureUserSettings changes and should be executed again for all users. https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/install_worker.cc:58: static const wchar_t* kActiveSetupVersion = L"23,0,0,0"; const wchar_t kActiveSetupVersion[] = L"23,0,0,0"; https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1271: Append(installer::kSetupExe)); please switch this to using BaseName of the current process. https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1288: string16(kActiveSetupVersion), true); remove string16() since it's implicit https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... File chrome/installer/setup/setup_main.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1096: please put a big honking notice here stating that kActiveSetupVersion in install_worker.cc must be increased if new/different work is added here and should be run for all users (see my comment in installer_worker.cc for more info). https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/uninstall.cc:307: PathService::Get(base::DIR_START_MENU, &shortcut_path); y u no handle return code?
Addressed comments. The current status is that the Active Setup + per-user shortcut + SCHNE_CREATE notification works for user that had logged in at least once before the install (but don't have to be logged in during the install), but not for those that are doing a fresh login post-install (even uninstalling and reinstalling or re-triggering Active Setup in some other ways doesn't help....?!?!). Further analysis is needed, but this is infinitely better then no Start Screen shortcut in all cases... https://chromiumcodereview.appspot.com/10889028/diff/11001/base/file_util_win.cc File base/file_util_win.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/11001/base/file_util_win... base/file_util_win.cc:460: else On 2012/09/01 03:20:28, grt wrote: > add braces here and to the if side unless you remove the comment below Done. https://chromiumcodereview.appspot.com/10889028/diff/11001/base/file_util_win... base/file_util_win.cc:461: // TODO(gab): Only use SHCNE_ASSOCCHANGED when the icon changed. On 2012/09/01 03:20:28, grt wrote: > is it even possible to put icon change detection logic down here? i don't think > it is, in which case i don't think this TODO belongs. My goal is to modify this function to check (for updates not creates) if the current state of the property it's about to update is the same as the current one (and if so don't update it). We would only need to notify the shell in the end if we actually did update something (we can special case the icon in the detection too since that requires the nuclear SHCNE_ASSOCCHANGED bit). This new notification mechanism will be particularly useful as we will install the shortcut per-user through Active Setup as the only way to update shortcuts post Chrome update will be at Active Setup (but that requires another login which could arguably happen much much after the update) or at browser launch (but we don't want a full desktop flash at every launch; hence the need to have a more confined shell notification mechanism when updating shortcuts). https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/install.cc:318: if (create_always && installer_state.system_install()) { On 2012/09/01 03:20:28, grt wrote: > nit: remove braces Done. https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/install.cc:358: FilePath start_menu_user_level; On 2012/09/01 03:20:28, grt wrote: > start_menu_user_level -> user_start_menu or per_user_start_menu Done. https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/install.cc:360: LOG(ERROR) << "Failed to get user-level start menu path."; On 2012/09/01 03:20:28, grt wrote: > i like DFATAL here since failure is extremely unexpected Sure, although this makes the code differ more from its copy pasted brother above. https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/install.cc:364: file_util::CreateDirectory(start_menu_user_level); On 2012/09/01 03:20:28, grt wrote: > y u no handle result? i suppose you could argue that the code below would error > out properly if the directory didn't exist, so it's okay. please add a comment > to that effect if that's your intention. Again, keeping the same behavior as its copy-pasted brother (and don't want to modify it yet) -- given we want to merge this in I think that not changing the current function's behavior and duplicating it as is here is the correct thing to do. https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/install.cc:378: << " failed."; On 2012/09/01 03:20:28, grt wrote: > nit: indentation (does this fit on the previous line? good eyes sir https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/install_worker.cc:52: // This is not the Chrome version. To have Active Setup (a method Chrome On 2012/09/01 03:20:28, grt wrote: > please reword the comment so that it begins by telling the reader what this > constant is. follow this by an explanation of if/when it should change. > something like: > > The version identifying the work done by setup.exe --configure-user-settings on > user login by way of Active Setup. Increase this value if the work done in > setup_main.cc's handling of kConfigureUserSettings changes and should be > executed again for all users. Done. https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/install_worker.cc:58: static const wchar_t* kActiveSetupVersion = L"23,0,0,0"; On 2012/09/01 03:20:28, grt wrote: > const wchar_t kActiveSetupVersion[] = L"23,0,0,0"; Done. https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1271: Append(installer::kSetupExe)); On 2012/09/01 03:20:28, grt wrote: > please switch this to using BaseName of the current process. Done. https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1288: string16(kActiveSetupVersion), true); On 2012/09/01 03:20:28, grt wrote: > remove string16() since it's implicit Done. https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... File chrome/installer/setup/setup_main.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1096: On 2012/09/01 03:20:28, grt wrote: > please put a big honking notice here stating that kActiveSetupVersion in > install_worker.cc must be increased if new/different work is added here and > should be run for all users (see my comment in installer_worker.cc for more > info). Done. https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/uninstall.cc:307: PathService::Get(base::DIR_START_MENU, &shortcut_path); On 2012/09/01 03:20:28, grt wrote: > y u no handle return code? same comment as in other files, staying close to behavior temporarily copied from above, but I agree that here it doesn't hurt to handle it. Done.
https://chromiumcodereview.appspot.com/10889028/diff/11001/base/file_util_win.cc File base/file_util_win.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/11001/base/file_util_win... base/file_util_win.cc:461: // TODO(gab): Only use SHCNE_ASSOCCHANGED when the icon changed. On 2012/09/01 22:28:40, gab wrote: > On 2012/09/01 03:20:28, grt wrote: > > is it even possible to put icon change detection logic down here? i don't > think > > it is, in which case i don't think this TODO belongs. > > My goal is to modify this function to check (for updates not creates) if the > current state of the property it's about to update is the same as the current > one (and if so don't update it). We would only need to notify the shell in the > end if we actually did update something (we can special case the icon in the > detection too since that requires the nuclear SHCNE_ASSOCCHANGED bit). The last time the icon changed, there was nothing visible to this function to indicate such (same file, same resource id). Let's discuss it in person with Robert, who wrestled with this the last time around. https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/install.cc:364: file_util::CreateDirectory(start_menu_user_level); On 2012/09/01 22:28:40, gab wrote: > On 2012/09/01 03:20:28, grt wrote: > > y u no handle result? i suppose you could argue that the code below would > error > > out properly if the directory didn't exist, so it's okay. please add a > comment > > to that effect if that's your intention. > > Again, keeping the same behavior as its copy-pasted brother (and don't want to > modify it yet) -- given we want to merge this in I think that not changing the > current function's behavior and duplicating it as is here is the correct thing > to do. On its own, this isn't a good enough reason. The merge isn't made more difficult by making small changes to non-obvious code to make it more obvious (either by adding a comment explaining the subtlety or by changing the logic to remove the subtlety). https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/11001/chrome/installer/s... chrome/installer/setup/uninstall.cc:307: PathService::Get(base::DIR_START_MENU, &shortcut_path); On 2012/09/01 22:28:40, gab wrote: > On 2012/09/01 03:20:28, grt wrote: > > y u no handle return code? > > same comment as in other files, staying close to behavior temporarily copied > from above, but I agree that here it doesn't hurt to handle it. It's actively harmful to not handle it. |shortcut_path| retains its previous value if PathService returns false. The code below adds to that and then does a recursive Delete. This happens as SYSTEM. We must never call Delete(foo, true) as SYSTEM unless we're certain what |foo| is. https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... chrome/installer/setup/install.cc:351: BrowserDistribution* browser_dist = product.distribution(); to protect against someone accidentally calling this in the future for user-level installs, how about adding: if (!installer_state.system_install()) { NOTREACHED(); return; } https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... chrome/installer/setup/install.cc:364: // as we still want to try to create the shortcut; if the directory is truly i find the "...as we still want to try..." part misleading since it implies that creating the shortcut might still work. there's no point in trying to create the shortcut if the directory couldn't be created. if you prefer to keep the logic (which is fine with me), please make this comment more like: // Ignore failures here since a failure to create the shortcut will be handled below. you can make it even more explicit by wrapping the next line in ignore_result(...) https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... chrome/installer/setup/install_worker.cc:56: const wchar_t* kActiveSetupVersion = L"23,0,0,0"; please make this an array rather than a pointer. const wchar_t* loses information (the length of the string). a std::string can potentially be initialized from an array without searching for the string terminator. i don't know for certain if MS's implementation does, but on principal i don't like the information loss here. https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... File chrome/installer/setup/setup_main.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1096: DCHECK(installer_state->system_install()); since it's legitimate for someone to inadvertently run this without --system-level, i think it's better to make this do the right thing in release builds. please change the logic so that ForceCreateUserLevelStartMenuShortcut isn't called in that case. one way would be to add " && installer_state->system_install()" to the condition on line 1106 (and maybe move the assignment of *exit_code up so it isn't set in that case).
PTAL. https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... chrome/installer/setup/install.cc:351: BrowserDistribution* browser_dist = product.distribution(); On 2012/09/02 14:18:19, grt wrote: > to protect against someone accidentally calling this in the future for > user-level installs, how about adding: > if (!installer_state.system_install()) { > NOTREACHED(); > return; > } Done. https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... chrome/installer/setup/install.cc:364: // as we still want to try to create the shortcut; if the directory is truly On 2012/09/02 14:18:19, grt wrote: > i find the "...as we still want to try..." part misleading since it implies that > creating the shortcut might still work. there's no point in trying to create > the shortcut if the directory couldn't be created. if you prefer to keep the > logic (which is fine with me), please make this comment more like: > > // Ignore failures here since a failure to create the shortcut will be handled > below. > > you can make it even more explicit by wrapping the next line in > ignore_result(...) Ended up deciding to wrap it in the if below. What I originally meant by "we still want to try" is that I guess it's possible the the CreateDirectory call fails, but the directory exists already. I also didn't want to add an extra return value to handle it... but adding it to the if seems like a good solution. https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... chrome/installer/setup/install_worker.cc:56: const wchar_t* kActiveSetupVersion = L"23,0,0,0"; On 2012/09/02 14:18:19, grt wrote: > please make this an array rather than a pointer. const wchar_t* loses > information (the length of the string). a std::string can potentially be > initialized from an array without searching for the string terminator. i don't > know for certain if MS's implementation does, but on principal i don't like the > information loss here. Agreed 100%. Done. https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... File chrome/installer/setup/setup_main.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1096: DCHECK(installer_state->system_install()); On 2012/09/02 14:18:19, grt wrote: > since it's legitimate for someone to inadvertently run this without > --system-level, i think it's better to make this do the right thing in release > builds. please change the logic so that ForceCreateUserLevelStartMenuShortcut > isn't called in that case. one way would be to add " && > installer_state->system_install()" to the condition on line 1106 (and maybe move > the assignment of *exit_code up so it isn't set in that case). Done and made InstallStatus better for this type of error. Imo this whole method is wrong in the way it works with |exit_code| and InstallStatuses. There should a local |status| at the top which everyone sets (and which is otherwise initialized to UNKNOWN_STATUS) at the end we could then DCHECK(!handled || status != UNKNOWN_STATUS) and only set the exit code then. It doesn't make sense that everyone has to set |exit_code| in the same way in each block; plus this would prevent someone implementing a new block from forgetting to set |exit_code| (as I originally did). If you agree with this plan I'll follow up with another CL.
On 2012/09/02 16:15:12, gab wrote: > PTAL. > > https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... > File chrome/installer/setup/install.cc (right): > > https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... > chrome/installer/setup/install.cc:351: BrowserDistribution* browser_dist = > product.distribution(); > On 2012/09/02 14:18:19, grt wrote: > > to protect against someone accidentally calling this in the future for > > user-level installs, how about adding: > > if (!installer_state.system_install()) { > > NOTREACHED(); > > return; > > } > > Done. > > https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... > chrome/installer/setup/install.cc:364: // as we still want to try to create the > shortcut; if the directory is truly > On 2012/09/02 14:18:19, grt wrote: > > i find the "...as we still want to try..." part misleading since it implies > that > > creating the shortcut might still work. there's no point in trying to create > > the shortcut if the directory couldn't be created. if you prefer to keep the > > logic (which is fine with me), please make this comment more like: > > > > // Ignore failures here since a failure to create the shortcut will be handled > > below. > > > > you can make it even more explicit by wrapping the next line in > > ignore_result(...) > > Ended up deciding to wrap it in the if below. What I originally meant by "we > still want to try" is that I guess it's possible the the CreateDirectory call > fails, but the directory exists already. I also didn't want to add an extra > return value to handle it... but adding it to the if seems like a good solution. > > https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... > File chrome/installer/setup/install_worker.cc (right): > > https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... > chrome/installer/setup/install_worker.cc:56: const wchar_t* kActiveSetupVersion > = L"23,0,0,0"; > On 2012/09/02 14:18:19, grt wrote: > > please make this an array rather than a pointer. const wchar_t* loses > > information (the length of the string). a std::string can potentially be > > initialized from an array without searching for the string terminator. i > don't > > know for certain if MS's implementation does, but on principal i don't like > the > > information loss here. > > Agreed 100%. Done. > > https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... > File chrome/installer/setup/setup_main.cc (right): > > https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... > chrome/installer/setup/setup_main.cc:1096: > DCHECK(installer_state->system_install()); > On 2012/09/02 14:18:19, grt wrote: > > since it's legitimate for someone to inadvertently run this without > > --system-level, i think it's better to make this do the right thing in release > > builds. please change the logic so that ForceCreateUserLevelStartMenuShortcut > > isn't called in that case. one way would be to add " && > > installer_state->system_install()" to the condition on line 1106 (and maybe > move > > the assignment of *exit_code up so it isn't set in that case). > > Done and made InstallStatus better for this type of error. > > Imo this whole method is wrong in the way it works with |exit_code| and > InstallStatuses. There should a local |status| at the top which everyone sets > (and which is otherwise initialized to UNKNOWN_STATUS) at the end we could then > DCHECK(!handled || status != UNKNOWN_STATUS) and only set the exit code then. > It doesn't make sense that everyone has to set |exit_code| in the same way in > each block; plus this would prevent someone implementing a new block from > forgetting to set |exit_code| (as I originally did). > > If you agree with this plan I'll follow up with another CL. Did this attempt wind up being subsumed into a different CL? Thanks guys!
On 2012/09/04 03:39:51, gideonwald wrote: > On 2012/09/02 16:15:12, gab wrote: > > PTAL. > > > > > https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... > > File chrome/installer/setup/install.cc (right): > > > > > https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... > > chrome/installer/setup/install.cc:351: BrowserDistribution* browser_dist = > > product.distribution(); > > On 2012/09/02 14:18:19, grt wrote: > > > to protect against someone accidentally calling this in the future for > > > user-level installs, how about adding: > > > if (!installer_state.system_install()) { > > > NOTREACHED(); > > > return; > > > } > > > > Done. > > > > > https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... > > chrome/installer/setup/install.cc:364: // as we still want to try to create > the > > shortcut; if the directory is truly > > On 2012/09/02 14:18:19, grt wrote: > > > i find the "...as we still want to try..." part misleading since it implies > > that > > > creating the shortcut might still work. there's no point in trying to > create > > > the shortcut if the directory couldn't be created. if you prefer to keep > the > > > logic (which is fine with me), please make this comment more like: > > > > > > // Ignore failures here since a failure to create the shortcut will be > handled > > > below. > > > > > > you can make it even more explicit by wrapping the next line in > > > ignore_result(...) > > > > Ended up deciding to wrap it in the if below. What I originally meant by "we > > still want to try" is that I guess it's possible the the CreateDirectory call > > fails, but the directory exists already. I also didn't want to add an extra > > return value to handle it... but adding it to the if seems like a good > solution. > > > > > https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... > > File chrome/installer/setup/install_worker.cc (right): > > > > > https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... > > chrome/installer/setup/install_worker.cc:56: const wchar_t* > kActiveSetupVersion > > = L"23,0,0,0"; > > On 2012/09/02 14:18:19, grt wrote: > > > please make this an array rather than a pointer. const wchar_t* loses > > > information (the length of the string). a std::string can potentially be > > > initialized from an array without searching for the string terminator. i > > don't > > > know for certain if MS's implementation does, but on principal i don't like > > the > > > information loss here. > > > > Agreed 100%. Done. > > > > > https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... > > File chrome/installer/setup/setup_main.cc (right): > > > > > https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... > > chrome/installer/setup/setup_main.cc:1096: > > DCHECK(installer_state->system_install()); > > On 2012/09/02 14:18:19, grt wrote: > > > since it's legitimate for someone to inadvertently run this without > > > --system-level, i think it's better to make this do the right thing in > release > > > builds. please change the logic so that > ForceCreateUserLevelStartMenuShortcut > > > isn't called in that case. one way would be to add " && > > > installer_state->system_install()" to the condition on line 1106 (and maybe > > move > > > the assignment of *exit_code up so it isn't set in that case). > > > > Done and made InstallStatus better for this type of error. > > > > Imo this whole method is wrong in the way it works with |exit_code| and > > InstallStatuses. There should a local |status| at the top which everyone sets > > (and which is otherwise initialized to UNKNOWN_STATUS) at the end we could > then > > DCHECK(!handled || status != UNKNOWN_STATUS) and only set the exit code then. > > It doesn't make sense that everyone has to set |exit_code| in the same way in > > each block; plus this would prevent someone implementing a new block from > > forgetting to set |exit_code| (as I originally did). > > > > If you agree with this plan I'll follow up with another CL. > > Did this attempt wind up being subsumed into a different CL? Thanks guys! Hmmm which attempt are you talking about? The refactoring suggestion (sounds like it, but I'm wondering why you'd care :)?!) or this actual start menu hack CL which is about to go in.
https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... File chrome/installer/setup/setup_main.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1096: DCHECK(installer_state->system_install()); On 2012/09/02 16:15:12, gab wrote: > On 2012/09/02 14:18:19, grt wrote: > > since it's legitimate for someone to inadvertently run this without > > --system-level, i think it's better to make this do the right thing in release > > builds. please change the logic so that ForceCreateUserLevelStartMenuShortcut > > isn't called in that case. one way would be to add " && > > installer_state->system_install()" to the condition on line 1106 (and maybe > move > > the assignment of *exit_code up so it isn't set in that case). > > Done and made InstallStatus better for this type of error. I don't think this new status code provides much of a signal on its own. If it's meant to be a general-purpose "something was called wrong" code, then seeing it on a dashboard may not tell us what was invoked (except for specific dashboards) or what the invalid state was. Do you think UNKNOWN_STATUS is sufficient for this? Also, I think it makes sense to LOG(DFATAL) any time this would be returned with enough info to know what was wrong so that release builds produce actionable (albeit local) info. > Imo this whole method is wrong in the way it works with |exit_code| and > InstallStatuses. There should a local |status| at the top which everyone sets > (and which is otherwise initialized to UNKNOWN_STATUS) at the end we could then > DCHECK(!handled || status != UNKNOWN_STATUS) and only set the exit code then. > It doesn't make sense that everyone has to set |exit_code| in the same way in > each block; plus this would prevent someone implementing a new block from > forgetting to set |exit_code| (as I originally did). > > If you agree with this plan I'll follow up with another CL. This certainly could be improved. Feel free to TODO it to yourself to take care of when it's time to pay down some debt.
Done. https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... File chrome/installer/setup/setup_main.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/23001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1096: DCHECK(installer_state->system_install()); On 2012/09/04 13:29:59, grt wrote: > On 2012/09/02 16:15:12, gab wrote: > > On 2012/09/02 14:18:19, grt wrote: > > > since it's legitimate for someone to inadvertently run this without > > > --system-level, i think it's better to make this do the right thing in > release > > > builds. please change the logic so that > ForceCreateUserLevelStartMenuShortcut > > > isn't called in that case. one way would be to add " && > > > installer_state->system_install()" to the condition on line 1106 (and maybe > > move > > > the assignment of *exit_code up so it isn't set in that case). > > > > Done and made InstallStatus better for this type of error. > > I don't think this new status code provides much of a signal on its own. If > it's meant to be a general-purpose "something was called wrong" code, then > seeing it on a dashboard may not tell us what was invoked (except for specific > dashboards) or what the invalid state was. Do you think UNKNOWN_STATUS is > sufficient for this? As discussed, we would rather never see UNKNOWN_STATUS in logs, on the flip side we also don't a new failure flag for a bunch of things that are never supposed to happen... so we introduce this new flag which should never happen, but at least it's not UNKNOWN_STATUS (which usually means we messed in the code and didn't handle a possible failure). Accompanied by DFATAL logs in Release builds we can trace back where things went unexpectingly should we ever get this error. > > Also, I think it makes sense to LOG(DFATAL) any time this would be returned with > enough info to know what was wrong so that release builds produce actionable > (albeit local) info. Done. > > > Imo this whole method is wrong in the way it works with |exit_code| and > > InstallStatuses. There should a local |status| at the top which everyone sets > > (and which is otherwise initialized to UNKNOWN_STATUS) at the end we could > then > > DCHECK(!handled || status != UNKNOWN_STATUS) and only set the exit code then. > > It doesn't make sense that everyone has to set |exit_code| in the same way in > > each block; plus this would prevent someone implementing a new block from > > forgetting to set |exit_code| (as I originally did). > > > > If you agree with this plan I'll follow up with another CL. > > This certainly could be improved. Feel free to TODO it to yourself to take care > of when it's time to pay down some debt. Done.
Oops build errors :)! PTAAHLL (Please take another and hopefully last look)! Cheers, Gab
https://chromiumcodereview.appspot.com/10889028/diff/11001/base/file_util_win.cc File base/file_util_win.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/11001/base/file_util_win... base/file_util_win.cc:461: // TODO(gab): Only use SHCNE_ASSOCCHANGED when the icon changed. On 2012/09/02 14:18:19, grt wrote: > On 2012/09/01 22:28:40, gab wrote: > > On 2012/09/01 03:20:28, grt wrote: > > > is it even possible to put icon change detection logic down here? i don't > > think > > > it is, in which case i don't think this TODO belongs. > > > > My goal is to modify this function to check (for updates not creates) if the > > current state of the property it's about to update is the same as the current > > one (and if so don't update it). We would only need to notify the shell in the > > end if we actually did update something (we can special case the icon in the > > detection too since that requires the nuclear SHCNE_ASSOCCHANGED bit). > > The last time the icon changed, there was nothing visible to this function to > indicate such (same file, same resource id). Let's discuss it in person with > Robert, who wrestled with this the last time around. As discussed, it isn't practical to detect the icon has changed, so please remove.
Done. Investigating how IE gets its shortcut out there some more. https://chromiumcodereview.appspot.com/10889028/diff/11001/base/file_util_win.cc File base/file_util_win.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/11001/base/file_util_win... base/file_util_win.cc:461: // TODO(gab): Only use SHCNE_ASSOCCHANGED when the icon changed. On 2012/09/04 18:44:35, grt wrote: > On 2012/09/02 14:18:19, grt wrote: > > On 2012/09/01 22:28:40, gab wrote: > > > On 2012/09/01 03:20:28, grt wrote: > > > > is it even possible to put icon change detection logic down here? i don't > > > think > > > > it is, in which case i don't think this TODO belongs. > > > > > > My goal is to modify this function to check (for updates not creates) if the > > > current state of the property it's about to update is the same as the > current > > > one (and if so don't update it). We would only need to notify the shell in > the > > > end if we actually did update something (we can special case the icon in the > > > detection too since that requires the nuclear SHCNE_ASSOCCHANGED bit). > > > > The last time the icon changed, there was nothing visible to this function to > > indicate such (same file, same resource id). Let's discuss it in person with > > Robert, who wrestled with this the last time around. > > As discussed, it isn't practical to detect the icon has changed, so please > remove. Done.
lgtm
drive-by https://chromiumcodereview.appspot.com/10889028/diff/11014/base/file_util_win.cc File base/file_util_win.cc (right): https://chromiumcodereview.appspot.com/10889028/diff/11014/base/file_util_win... base/file_util_win.cc:461: SHChangeNotify(SHCNE_ASSOCCHANGED, SHCNF_IDLIST, NULL, NULL); Is it not sufficient here to so SHCNE_UPDATEITEM with the path to the destination?
All relevants part of this CL were done as part of http://crbug.com/148539 Closing. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
