Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14287008:
Refactoring installer shortcut deletion; adding dedicated shortcut update feature. (Closed)
|
Created:
7 years, 8 months ago by huangs Modified:
7 years, 7 months ago CC:
chromium-reviews, grt+watch_chromium.org Base URL:
http://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRefactoring installer shortcut deletion; adding dedicated shortcut update feature.
In ShellUtil, there were 3 shortcut removal routines. Changes:
RemoveShortcut() => Renamed to RemoveShortcuts(), also ridding |shortcut_name| parameter (removed 1 caller that was redundant to start with).
RemoveTaskbarShortcuts() => Absorbed by RemoveShortcut()
RemoveStartScreenShortcuts() => Absorbed by RemoveShortcut()
We want to also add an "update" feature to enable batch shortcut udpate, e.g., change all shortcuts that point to app_host.exe to point to chrome.exe instead, with the proper icon changes.
To unify both operations, we create a BatchShortcutAction() that takes callbacks to perform specific operations (removal / update). ShellUtil::GetShortcutPath() is also refactored to accept more locations. The added routine in ShellUtil is:
UpdateShortcuts()
BUG=234876
R=gab@chromium.org, grt@chromium.org, sky@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198514
Patch Set 1 : #
Total comments: 28
Patch Set 2 : Code cleanup; restructured BatchShortcutOperation; making RemoveShortcut() remove all by default. #Patch Set 3 : More refactoring; single visitor BatchShortcutAction(); using callbacks for operations. #Patch Set 4 : Adding tests for cookie migration code; using SHORTCUT_REPLACE_EXISTING now. #
Total comments: 36
Patch Set 5 : Removed redundant routines; cleaning up interfaces, including fixing callers. #
Total comments: 38
Patch Set 6 : Removing name_filter; removing general directory operations; renaming 'migrate' to 'retarget'; code… #
Total comments: 44
Patch Set 7 : Comment fixes; reduced RemoveShortcuts() to a single interface. #
Total comments: 14
Patch Set 8 : Comment fixes; cleanups. #
Total comments: 16
Patch Set 9 : Adding ShellUtil::ShortcutLocationIsExpected(); clearnup on error checks. #
Total comments: 12
Patch Set 10 : Renaming to ShortcutLocationIsSupported(); cleanups. #
Total comments: 2
Patch Set 11 : Comment fix. #Patch Set 12 : Removing small change to base_paths_win.cc; save it for later. #Patch Set 13 : Generaling 'Retarget' to 'Update' via base::win::ShortcutProperties. #
Total comments: 15
Patch Set 14 : Comment fixes; some refactoring in ShellUtil unittest. #
Total comments: 10
Patch Set 15 : Reverting ShellUtil unittest changes; bit more fixing. #
Total comments: 2
Patch Set 16 : Nits. #
Total comments: 3
Patch Set 17 : Sync. #
Total comments: 4
Patch Set 18 : Comment fixes. #Patch Set 19 : Sync; fixing .git/config to allow CQ to commit. #
Total comments: 2
Messages
Total messages: 44 (0 generated)
This is a preliminary CL, to see if you think the approach is viable. PTAL.
In general I think it would be cleaner, shorter, and more adaptable if instead of being a class that is subclassed for various purposes, this was simply a Run method taking a Callback for which there is only one unbound parameter (the path to the shortcut to act upon). The Run method would then do the same logic and call the Callback for each shortcut (same principle as ActOnShortcut(), but doesn't require a full class definition for each implementation). More specific comments below written as I was making up my mind about this (some of them might be voided by my meta-comment above). Cheers! Gab https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1175: ShellUtil::GetBrowserModelId(dist, nit: wrap |dist| on the line below. The rule is either you wrap to the paran or 4 spaces, but never a mix of both. https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1189: class BatchShortcutOperation { I think it would be nicer, less verbose, and easier to specialize if instead of a full class.......... https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1204: // |name| can specify the name for a single shortcut (without ".lnk" suffix), s/suffix/extension https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1204: // |name| can specify the name for a single shortcut (without ".lnk" suffix), I don't like relying on the extension being present or not, often adds burden on the callers either way. How about simply checking for it and adjusting if required (see https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/u...). I could be convinced otherwise.. https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1238: if (shortcut_path.Extension() == installer::kLnkExt) { You are already only enumerating files with the .lnk extension. https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1259: const string16* name) { If |name| is changed to a pattern (as suggested in another comment in shell_util.h), I don't think we need 2 methods (i.e., I think we can fold Run() in this method). https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1272: if (!PathService::Get(base::DIR_TASKBAR_PINS, &taskbar_pins_path) || Feels like we should augment GetShortcutPath() instead of adding an extra method here. https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1278: return Run(taskbar_pins_path, kAllFiles, target_exe); Taskbar pins are special though, they have to be explicitly unpinned via base::win::TaskbarUnpinShortcutLink(), only deleting the file confuses Windows. base::win::TaskbarUnpinShortcutLink() can either be called on the shortcut that was originally pinned or on the pin itself (in fact I think it resolves the appid of the "unpinned shortcut" and unpins the shortcut with a matching appid). https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1308: bool delete_folder) (bool, bool, bool, bool, bool, bool, bool) interfaces are hard to read :). Consider using a struct so that it is easier to see what is being called from the caller code without having to check the method's signature. https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1318: if (!base::win::TaskbarUnpinShortcutLink(shortcut_path.value().c_str())) { Did you check who uses RemoveShortcut(), is it still worth it for this method to perform the unpinning or should that be done separately? https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:2048: BatchShortcutDelete op(true, true, delete_folder); // Unpon and delete-file. nit: Unpon :) https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:2067: base::FilePath folder(GetAppShortcutsFolder(dist, target_exe)); This could probably also go in GetShortcutPath() as an extra ShortcutLocation, making the code even simpler. https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.h:516: const string16* shortcut_name); I like the idea of using a pattern here instead of using NULL to mean all shortcuts. https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.h:529: // Traverses through instsalles shortcut(s) in a similar fashion as nit: instsalles ?!
Implemented *some* of the suggested changes, since I'm planning some bigger change. Uploading now as intermediate step. https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1175: ShellUtil::GetBrowserModelId(dist, On 2013/04/24 21:41:46, gab wrote: > nit: wrap |dist| on the line below. > > The rule is either you wrap to the paran or 4 spaces, but never a mix of both. Done. https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1189: class BatchShortcutOperation { On 2013/04/24 21:41:46, gab wrote: > I think it would be nicer, less verbose, and easier to specialize if instead of > a full class.......... I think it's important to group it into a class, but using a different method now. https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1204: // |name| can specify the name for a single shortcut (without ".lnk" suffix), On 2013/04/24 21:41:46, gab wrote: > s/suffix/extension Done. https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1204: // |name| can specify the name for a single shortcut (without ".lnk" suffix), Done; adding ".lnk" if missing (exception is empty string, which specifies "only act on folder"). https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1238: if (shortcut_path.Extension() == installer::kLnkExt) { Changed to DCHECK_EQ(). https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1259: const string16* name) { I think I'll get rid of the ActOnFolder() stuff; will get back to you. https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1272: if (!PathService::Get(base::DIR_TASKBAR_PINS, &taskbar_pins_path) || Might be solved by removing ActOnFolder(). https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1278: return Run(taskbar_pins_path, kAllFiles, target_exe); Per earlier discussion, having a dedicated unpin operation would probably solve this. https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1308: bool delete_folder) Adding accessors to explicitly set flags. But this may change once I remove ActOnFolder(). https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1318: if (!base::win::TaskbarUnpinShortcutLink(shortcut_path.value().c_str())) { There a routine that avoids calling this BECAUSE of unpinning. But this should be a separate refactoring task! https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:2048: BatchShortcutDelete op(true, true, delete_folder); // Unpon and delete-file. On 2013/04/24 21:41:46, gab wrote: > nit: Unpon :) Done. :) https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:2067: base::FilePath folder(GetAppShortcutsFolder(dist, target_exe)); Will do this next. Can I remove / redo the messages below? https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.h:516: const string16* shortcut_name); Changing interface: The old routine is now RemoveShortcutWithName(), and by default RemoveShortcut() removes all shortcuts (made into an adaptor). This way we can keep the details w.r.t. "*" vs. NULL internal to ShellUtil. https://codereview.chromium.org/14287008/diff/3001/chrome/installer/util/shel... chrome/installer/util/shell_util.h:529: // Traverses through instsalles shortcut(s) in a similar fashion as On 2013/04/24 21:41:46, gab wrote: > nit: instsalles ?! installed. Done.
Also changed interfaces somewhat, and updated callers. PTAL.
Before, SHORTCUT_UPDATE_EXISTING creates a weird failure where name change was unsuccessful, BUT if I peer into the .lnk file in the debugger then it works?! Should be solved now. Code complete, pending requests by review, PTAL.
Ping.
Comments below, haven't looked at the tests yet, will wait for this set of comments to be adressed first. Thanks! Gab https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/s... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/s... chrome/installer/setup/uninstall.cc:350: if (!ShellUtil::RemoveShortcutWithName( If all shortcuts pointing to |target_exe| are deleted by RemoveShortcut(ShellUtil::SHORTCUT_LOCATION_DESKTOP, ...) this call is no longer needed :)! https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... File chrome/installer/util/shell_util.cc (left): https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1303: system_shortcut_path.empty()) { GetShortcutPath() is already designed to only return true if the path is not empty(), remove all these extra checks you added. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1164: base::FilePath GetAppShortcutsFolder(BrowserDistribution* dist, I feel it would be better to have GetAppShortcutsFolder() use the same style as GetShortcutPath() (i.e. return bool and return the path as out-param). This would avoid returning empty paths here on failure and would integrate more cleanly in GetShortcutPath(). https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1184: const wchar_t kAllFiles[] = L"*"; Rename kAllShortcuts and add the extension (if you can help it via a constant I think it's better if the shortcut code doesn't have to fix this by adding the extension). https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1235: // |name_filter|, and have |target_exe| as the target. Remove comma (it is only an enumeration of 2 things). https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1237: // |opFolder| on |shortcut_folder|. What about: // Applies |shortcut_operation| on each matching shortcuts and then applies |folder_operation| on |shortcut_folder|. ? https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1241: bool BatchShortcutAction(const FileOperationCallback& opShortcut, Use underscore casing for variable names, not Camel case. I suggest shortcut_operation and folder_operation. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1251: LOG(ERROR) << "Cannot find path at location " << location; This is more an WARNING, then an ERROR imo. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1261: if (!EndsWith(pattern, installer::kLnkExt, false)) You just added installer::kLnkExt above so it will always end with the extension... I think you want: string16 pattern = name_filter; and then test for the extension and add it if necessary. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1264: file_util::FileEnumerator::FILES, pattern); either indent parameters at the paranthesis or 4 spaces in, not both. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1271: if (!opShortcut.Run(shortcut_path)) { Merge this if with the previous one via "&&" (since the previous if itself doesn't do anything else but nest this one). https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1374: case SHORTCUT_LOCATION_TASKBAR_PINS: I feel this should fail if (base::win::GetVersion() >= base::win::VERSION_WIN7)) to be consistent with the new comments in the ShortcutLocation interface... At the same time this probably doesn't matter since we should only ever use this location to read/replace shortcut (all creations of pinned shortcuts are done via the ShellAPI). It also should really be up to PathService::Get() to fail for base::DIR_TASKBAR_PINS under Win7, but it doesn't right now (https://code.google.com/p/chromium/codesearch#chromium/src/base/base_paths_wi.... something to keep in mind/fix as a separate CL. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1983: bool ShellUtil::RemoveShortcut(ShellUtil::ShortcutLocation location, Consider renaming to RemoveShortcutsAtLocation now that this no longer removes a single shortcut. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1992: void ShellUtil::RemoveTaskbarShortcuts(const string16& target_exe) { This call and RemoveStartScreenShortcuts() below do not have to be special cased imo, they can easily be folded in the generic call above, no? The early return by OS version will be handled by the path not being found on older OS versions anyways. (same comment for Migrate*) https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:2013: if (!BatchShortcutAction(base::Bind(&ShortcutOpNone), This will still unnecessarily iterate over all shortcuts. A Callback doesn't have to be bound, in which case it is "null" (see [1] for example; I think here you could use FileOperationCallback() directly). I think null callbacks are preferable to empty operations here, you can then check that the callback isn't null before starting to iterate. [1] https://code.google.com/p/chromium/codesearch#chromium/src/base/callback_unit... This is also true in the generic call if |delete_folder| is true since you delete all the shortcuts one by one to later unconditionally delete the folder recursively (making the individual shortcut operation superfluous). https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:2026: const base::FilePath& new_target_path) { A per above comment, change _path to _exe when referring to an executable rather than a directory, easier to read when variable naming conventions are uniform. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.h:514: static bool RemoveShortcutWithName(ShellUtil::ShortcutLocation location, I think that (once a comment above is adressed), there will be no remaning users of this call and we can thus remove it and unconditionally use *.lnk :). https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.h:524: const base::FilePath& target_path); Please keep this named |target_exe|, variables ending in _path often refer to directories rather than files from experience.
Note that in BatchShortcutAction(), the |name_filter| parameter is now always kAllShortcuts or L"". But since we're now using "!shortcut_operation.is_null()", the L"" parameter need not exist. Can we get rid of |name_filter|, and just default to kAllShortcuts always? No one is using it any more (killed the last indirect caller from uninstall.cc, and removed the unit test). PTAL. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/s... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/s... chrome/installer/setup/uninstall.cc:350: if (!ShellUtil::RemoveShortcutWithName( Removed, noting that RemoveShortcut() continues to process even on error. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... File chrome/installer/util/shell_util.cc (left): https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1303: system_shortcut_path.empty()) { This IS the original code; my CL already removes these checks. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1164: base::FilePath GetAppShortcutsFolder(BrowserDistribution* dist, On 2013/04/29 19:06:36, gab wrote: > I feel it would be better to have GetAppShortcutsFolder() use the same style as > GetShortcutPath() (i.e. return bool and return the path as out-param). > This would avoid returning empty paths here on failure and would integrate more > cleanly in GetShortcutPath(). Done. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1184: const wchar_t kAllFiles[] = L"*"; On 2013/04/29 19:06:36, gab wrote: > Rename kAllShortcuts and add the extension (if you can help it via a constant I > think it's better if the shortcut code doesn't have to fix this by adding the > extension). Done. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1235: // |name_filter|, and have |target_exe| as the target. Rephrased. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1237: // |opFolder| on |shortcut_folder|. On 2013/04/29 19:06:36, gab wrote: > What about: > > // Applies |shortcut_operation| on each matching shortcuts and then applies > |folder_operation| on |shortcut_folder|. > > ? Done. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1241: bool BatchShortcutAction(const FileOperationCallback& opShortcut, Done (language mix-up :). https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1251: LOG(ERROR) << "Cannot find path at location " << location; On 2013/04/29 19:06:36, gab wrote: > This is more an WARNING, then an ERROR imo. Done. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1261: if (!EndsWith(pattern, installer::kLnkExt, false)) Ah, good catch! Done. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1264: file_util::FileEnumerator::FILES, pattern); On 2013/04/29 19:06:36, gab wrote: > either indent parameters at the paranthesis or 4 spaces in, not both. Done. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1271: if (!opShortcut.Run(shortcut_path)) { On 2013/04/29 19:06:36, gab wrote: > Merge this if with the previous one via "&&" (since the previous if itself > doesn't do anything else but nest this one). Done. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1374: case SHORTCUT_LOCATION_TASKBAR_PINS: You mean, fail if (base::win::GetVersion() < base::win::VERSION_WIN7)? Regarding PathService::Get(), I'll make a separate CL to fix this -- but not in this CL, which is blocking another task. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1983: bool ShellUtil::RemoveShortcut(ShellUtil::ShortcutLocation location, Moot; removed routine. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:1992: void ShellUtil::RemoveTaskbarShortcuts(const string16& target_exe) { Changed routine and the caller. To maintain behaviour, just making this case unpin (and the caller in uninstlal.cc will need to know that this is for user-level only). https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:2013: if (!BatchShortcutAction(base::Bind(&ShortcutOpNone), This won't do useless cycles because |name_filter| was set to L"". Defined kNullFileOperation and updated BatchShortcutAction() to skip if |*_operation|.is_null(). https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.cc:2026: const base::FilePath& new_target_path) { Done. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.h:514: static bool RemoveShortcutWithName(ShellUtil::ShortcutLocation location, On 2013/04/29 19:06:36, gab wrote: > I think that (once a comment above is adressed), there will be no remaning users > of this call and we can thus remove it and unconditionally use *.lnk :). Done. https://chromiumcodereview.appspot.com/14287008/diff/15001/chrome/installer/u... chrome/installer/util/shell_util.h:524: const base::FilePath& target_path); I wanted to distinguish string16 from FilePath (and called directories *_folder). Changed. Since gloves are coming off w.r.t. callers, fixed callers to pass base::FilePath instead, for uniformity.
https://codereview.chromium.org/14287008/diff/23002/chrome/installer/setup/un... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/14287008/diff/23002/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:367: if (base::win::GetVersion() >= base::win::VERSION_WIN7) LOG_IF(WARNING, base::win::GetVersion() >= base::win::VERSION_WIN7) << "Failed to unpin taskbar shortcuts at user-level."; https://codereview.chromium.org/14287008/diff/23002/chrome/installer/util/she... File chrome/installer/util/shell_util.cc (left): https://codereview.chromium.org/14287008/diff/23002/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1303: system_shortcut_path.empty()) { why remove this? it seems like a reasonable safeguard. https://codereview.chromium.org/14287008/diff/23002/chrome/installer/util/she... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/14287008/diff/23002/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1166: bool is_user_level, indentation https://codereview.chromium.org/14287008/diff/23002/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1166: bool is_user_level, bool -> ShellChange for consistency https://codereview.chromium.org/14287008/diff/23002/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1192: FileOperationCallback kNullFileOperation; remove this (globals are banned). https://codereview.chromium.org/14287008/diff/23002/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1241: const FileOperationCallback& folder_operation, folder_operation seems overly abstract. it's only either null or delete, so how about a delete_folder boolean? since it does a recursive delete, is there any point in operating on any shortcuts in the folder since they will just be deleted anyway? https://codereview.chromium.org/14287008/diff/23002/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1246: const string16& name_filter) { simplify by removing name_filter and kAllShortcuts and always use * + kLnkExt as the enumerator pattern. https://codereview.chromium.org/14287008/diff/23002/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1260: if (!EndsWith (pattern, installer::kLnkExt, false)) remove space before open paren https://codereview.chromium.org/14287008/diff/23002/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1271: LOG(ERROR) << "Failed shortcut operation on " suggestion: move the logging into the operations so that it can be more meaningful. https://codereview.chromium.org/14287008/diff/23002/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1283: if (!folder_operation.is_null() && !folder_operation.Run(shortcut_folder)) { document that folder_operation (or delete_folder boolean if you follow my advice above) takes place regardless of whether shortcut_operation succeeds or fails. (and be sure that this is the right thing to do!) https://codereview.chromium.org/14287008/diff/23002/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1987: ShortcutLocation location = SHORTCUT_LOCATION_APP_SHORTCUTS; nit: remove this local var and put the constant inline in the function call below. https://codereview.chromium.org/14287008/diff/23002/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1994: location, dist, level, base::FilePath(target_exe), base::FilePath(target_exe) -> target_exe https://codereview.chromium.org/14287008/diff/23002/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1995: L"")) { L"" -> string16() https://codereview.chromium.org/14287008/diff/23002/chrome/installer/util/she... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/14287008/diff/23002/chrome/installer/util/she... chrome/installer/util/shell_util.h:503: // |level|: CURRENT_USER to remove the per-user shortcut and SYSTEM_LEVEL to and -> or https://codereview.chromium.org/14287008/diff/23002/chrome/installer/util/she... chrome/installer/util/shell_util.h:512: static bool RemoveShortcut(ShellUtil::ShortcutLocation location, RemoveShortcut -> RemoveShortcuts https://codereview.chromium.org/14287008/diff/23002/chrome/installer/util/she... chrome/installer/util/shell_util.h:525: static bool MigrateShortcut(ShellUtil::ShortcutLocation location, "Migrate" is very generic. Since this updates the target, I suggest UpdateTargetOfShortcuts or something like that. https://codereview.chromium.org/14287008/diff/23002/chrome/installer/util/she... chrome/installer/util/shell_util.h:531: // Similar to RemoveStartScreenShortcuts(), but performs migration. Same comment about "migrate" here in comment and method name below. https://codereview.chromium.org/14287008/diff/23002/chrome/installer/util/she... chrome/installer/util/shell_util.h:532: static void MigrateStartScreenShortcuts(BrowserDistribution* dist, this doesn't appear to ever be called. why is it included in this CL? if it's for future use, is it possible to at least include a test for it?
Also updated main description. PTAL. https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/s... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/s... chrome/installer/setup/uninstall.cc:367: if (base::win::GetVersion() >= base::win::VERSION_WIN7) On 2013/04/30 14:38:07, grt wrote: > LOG_IF(WARNING, base::win::GetVersion() >= base::win::VERSION_WIN7) > << "Failed to unpin taskbar shortcuts at user-level."; Done. https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... File chrome/installer/util/shell_util.cc (left): https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... chrome/installer/util/shell_util.cc:1303: system_shortcut_path.empty()) { GetShortcutPath() already checks for this. Removed instances in BatchShortcutAction() as well -- but will put it back if you insist! https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... chrome/installer/util/shell_util.cc:1166: bool is_user_level, On 2013/04/30 14:38:07, grt wrote: > indentation Done. https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... chrome/installer/util/shell_util.cc:1166: bool is_user_level, On 2013/04/30 14:38:07, grt wrote: > bool -> ShellChange for consistency Done. https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... chrome/installer/util/shell_util.cc:1192: FileOperationCallback kNullFileOperation; Done. Replacing uses with FileOperationCallback(). https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... chrome/installer/util/shell_util.cc:1241: const FileOperationCallback& folder_operation, Decided to extract RemoveShortcutFolder(). Before it was munged here to reuse shortcut_folder. But the GetShortcutPath() cleanup makes it less expensive to duplicate code to get |shortcut_folder|. https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... chrome/installer/util/shell_util.cc:1246: const string16& name_filter) { On 2013/04/30 14:38:07, grt wrote: > simplify by removing name_filter and kAllShortcuts and always use * + kLnkExt as > the enumerator pattern. Done. https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... chrome/installer/util/shell_util.cc:1260: if (!EndsWith (pattern, installer::kLnkExt, false)) Moot; deleted code. https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... chrome/installer/util/shell_util.cc:1271: LOG(ERROR) << "Failed shortcut operation on " Done. https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... chrome/installer/util/shell_util.cc:1283: if (!folder_operation.is_null() && !folder_operation.Run(shortcut_folder)) { Moved to RemoveShortcutFolder(). Right thing to do or not, it was the previous thing -- and I'm just refactoring! Added comment on BatchShortcutAction() to say that operations are performed even if failures occur. https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... chrome/installer/util/shell_util.cc:1987: ShortcutLocation location = SHORTCUT_LOCATION_APP_SHORTCUTS; Done. https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... chrome/installer/util/shell_util.cc:1994: location, dist, level, base::FilePath(target_exe), Ah, right. But moot: Removed code. https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... chrome/installer/util/shell_util.cc:1995: L"")) { Moot; removed code. https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... chrome/installer/util/shell_util.h:503: // |level|: CURRENT_USER to remove the per-user shortcut and SYSTEM_LEVEL to On 2013/04/30 14:38:07, grt wrote: > and -> or Done. https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... chrome/installer/util/shell_util.h:512: static bool RemoveShortcut(ShellUtil::ShortcutLocation location, On 2013/04/30 14:38:07, grt wrote: > RemoveShortcut -> RemoveShortcuts Done. https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... chrome/installer/util/shell_util.h:525: static bool MigrateShortcut(ShellUtil::ShortcutLocation location, Using "Retarget". :) https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... chrome/installer/util/shell_util.h:531: // Similar to RemoveStartScreenShortcuts(), but performs migration. On 2013/04/30 14:38:07, grt wrote: > Same comment about "migrate" here in comment and method name below. Done. https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... chrome/installer/util/shell_util.h:532: static void MigrateStartScreenShortcuts(BrowserDistribution* dist, This CL is intended for immediate use by: https://codereview.chromium.org/14031025 In the interest of time, I need to get this CL out of the way ASAP. Potential issues of Retarget*Shortcuts() can be ironed out in 14031025 when there's a clear use case.
This is looking great :)! The new tests duplicate a lot of code though :(, can you find a way to restructure them to avoid that? Cheers! Gab https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... chrome/installer/util/shell_util.cc:1249: !file_util::PathExists(shortcut_folder)) { Why did you remove this PathExists() check? This is why I had suggested making it a WARNING (i.e. the folder may simply not exist). GetShortcutPath() itself should never fail. The Create methods you might have taken this from are different in that they do not need to verify that the folder exists because they will create it if it doesn't (or silently fail in the case of an update). In your case you are reading/deleting something and should make sure it exists first imo (unless you can rely on FileEnumerator to successfully generate an empty list if |shortcut_folder| doesn't exist?). (Either way GetShortcutPath() failing by itself is a NOTREACHED() case). https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/s... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/s... chrome/installer/setup/uninstall.cc:342: VLOG(1) << "Deleting Desktop shortcut."; nit: s/shortcut/shortcuts https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/s... chrome/installer/setup/uninstall.cc:348: VLOG(1) << "Deleting Quick Launch shortcut."; nit: s/shortcut/shortcuts https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/s... chrome/installer/setup/uninstall.cc:355: if (!ShellUtil::RemoveShortcuts(ShellUtil::SHORTCUT_LOCATION_START_MENU, dist, Note to self (and huangs/calamity): this could easily be tweaked now to fix app shortcut issue crbug.com/171645; the issue is that shortcuts to chrome.exe in the Start Menu itself are not deleted because SHORTCUT_LOCATION_START_MENU is currently the Google Chrome folder in the Start Menu (and thus doesn't look for shortcuts in the Start Menu itself...). This migration logic will probably need to include the Start Menu shortcuts so it will be a good time to also fix their deletion :). https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/s... chrome/installer/setup/uninstall.cc:366: dist, ShellUtil::CURRENT_USER, target_exe)) { nit: indent https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1163: // (Windows 8+) Finds the folder for app shortcuts, store it in *|path|. s/store/stores https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1229: // In |shortcut_folder|, applies |shortcut_operation| to each shortcut that I find this wording weird, I'd prefer: Applies |shortcut_operation| to each shortcut in |shortcut_folder| that points to |target_exe| . https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1230: // point to |target_exe|, dot, not comma, at end of sentence. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1249: kAllShortcuts); I think it's fine to hardcode ("*" + installer::kLnkExt) here instead of using the global constant. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1273: LOG(WARNING) << "Cannot find path at location " << location; See other other comment on "why did you remove the check for PathExists()?" And same comment here, I guess it's OK if you can rely on file_util::Delete to succeed when the path doesn't exist. Either way GetShortcutPath() failing by itself is a NOTREACHED() case. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1367: if (base::win::GetVersion() < base::win::VERSION_WIN7) I don't believe this should take care of this logic, base_paths_win.cc should do that for base::DIR_TASKBAR_PINS imo. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1968: base::Bind(&ShortcutOpUnpin) : base::Bind(&ShortcutOpUnpinAndDelete); indent this line 4 more spaces in. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1972: if (location == SHORTCUT_LOCATION_START_MENU && Put this first and only do the BatchShortcutAction if not doing the RemoveShortcutFolder() -- it's useless to delete shortcuts one-by-one only to delete the entire folder latter (given it's an unconditional recursive folder deletion...). If your main concern is the fact that this wouldn't call unpin for the shortcuts in that folder, you are right, but I don't think this matters anymore now that we do a cleanup pass for the pinned shortcuts, I'd suggest removing the ShortcutOpUnpinAndDelete operation altogether, using ShortcutOpDelete instead and relying on the BatchShortcutAction on SHORTCUT_LOCATION_TASKBAR_PINS to take care of all the pins (this didn't use to exist hence the need to unpin individual shortcuts, it was added to cleanup non-installer-pinned-shortcuts and now that it cleans all pinned shortcuts, it's unnecessary for individual shortcuts to unpin themselves imo!). https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1980: void ShellUtil::RemoveStartScreenShortcuts(BrowserDistribution* dist, Once again, I think this can be merged in RemoveShortcuts, the logic for |level| can be left to the caller as it already is for other locations and the RemoveShortcutFolder call is then the same logic as the Start Menu shortcuts. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:2004: void ShellUtil::RetargetStartScreenShortcuts( This can also easily be folded in RetargetShortcuts imo. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.h:524: // to point to points to |new_target_exe|. "to point to points to"?! :)
https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/14287008/diff/23002/chrome/installer/u... chrome/installer/util/shell_util.h:532: static void MigrateStartScreenShortcuts(BrowserDistribution* dist, On 2013/04/30 15:58:38, huangs1 wrote: > This CL is intended for immediate use by: > https://codereview.chromium.org/14031025 I don't see MigrateStartScreenShortcuts being used in that CL. Why is it present here? https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/s... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/s... chrome/installer/setup/uninstall.cc:344: install_level, target_exe)) { fix indentation for all of these https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1249: kAllShortcuts); On 2013/04/30 19:20:33, gab wrote: > I think it's fine to hardcode ("*" + installer::kLnkExt) here instead of using > the global constant. +1 https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1253: base::FilePath target_path; nit: move this out of the loop so it isn't created/destroyed on each iteration https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1273: LOG(WARNING) << "Cannot find path at location " << location; On 2013/04/30 19:20:33, gab wrote: > See other other comment on "why did you remove the check for PathExists()?" > > And same comment here, I guess it's OK if you can rely on file_util::Delete to > succeed when the path doesn't exist. > > Either way GetShortcutPath() failing by itself is a NOTREACHED() case. Looks like this isn't always the case -- on Win8 false may legitimately be returned for SHORTCUT_LOCATION_APP_SHORTCUTS if the dir isn't there. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1969: bool success = true; bool success = BatchShortcutAction(...);
Also added base/base_paths_win.cc (which I think should be a separate cleanup CL; oh well). PTAL. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/s... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/s... chrome/installer/setup/uninstall.cc:342: VLOG(1) << "Deleting Desktop shortcut."; On 2013/04/30 19:20:33, gab wrote: > nit: s/shortcut/shortcuts Done. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/s... chrome/installer/setup/uninstall.cc:344: install_level, target_exe)) { On 2013/04/30 19:59:53, grt wrote: > fix indentation for all of these Done. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/s... chrome/installer/setup/uninstall.cc:348: VLOG(1) << "Deleting Quick Launch shortcut."; On 2013/04/30 19:20:33, gab wrote: > nit: s/shortcut/shortcuts Done. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/s... chrome/installer/setup/uninstall.cc:355: if (!ShellUtil::RemoveShortcuts(ShellUtil::SHORTCUT_LOCATION_START_MENU, dist, Let's worry about this after my App Launcher thing is done. :) https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/s... chrome/installer/setup/uninstall.cc:366: dist, ShellUtil::CURRENT_USER, target_exe)) { On 2013/04/30 19:20:33, gab wrote: > nit: indent Done. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1163: // (Windows 8+) Finds the folder for app shortcuts, store it in *|path|. Rephrased. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1229: // In |shortcut_folder|, applies |shortcut_operation| to each shortcut that Much better. Done. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1230: // point to |target_exe|, On 2013/04/30 19:20:33, gab wrote: > dot, not comma, at end of sentence. Done. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1249: kAllShortcuts); On 2013/04/30 19:20:33, gab wrote: > I think it's fine to hardcode ("*" + installer::kLnkExt) here instead of using > the global constant. Done. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1249: kAllShortcuts); On 2013/04/30 19:59:53, grt wrote: > On 2013/04/30 19:20:33, gab wrote: > > I think it's fine to hardcode ("*" + installer::kLnkExt) here instead of using > > the global constant. > > +1 Done. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1253: base::FilePath target_path; On 2013/04/30 19:59:53, grt wrote: > nit: move this out of the loop so it isn't created/destroyed on each iteration Done. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1273: LOG(WARNING) << "Cannot find path at location " << location; The NOTREACHED() case is only for cases where folders are expected to exist; this is not invoked for SHORTCUT_LOCATION_TASKBAR_PINS or SHORTCUT_LOCATION_APP_SHORTCUTS. Awaiting decisions on whether or not to put extra check. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1273: LOG(WARNING) << "Cannot find path at location " << location; Acknowledged. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1367: if (base::win::GetVersion() < base::win::VERSION_WIN7) Modified in base_paths_win.cc. There are many unneeded "base::", but this is not the time to change it. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1968: base::Bind(&ShortcutOpUnpin) : base::Bind(&ShortcutOpUnpinAndDelete); On 2013/04/30 19:20:33, gab wrote: > indent this line 4 more spaces in. Done. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1969: bool success = true; Now returning directly, so got rid of variable |success|, https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1972: if (location == SHORTCUT_LOCATION_START_MENU && Done; doing 3 distinct things for 3 separate cases. Also fixing caller comments in uninstall.cc. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1980: void ShellUtil::RemoveStartScreenShortcuts(BrowserDistribution* dist, Done. Key Changes: - Updated the single caller from uninstall.cc. - Instead of grabbing level from taget_exe, it is assigned by the caller to be |install_level|. - The verbose message is gone (can put it in the caller if needed). https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:2004: void ShellUtil::RetargetStartScreenShortcuts( On 2013/04/30 19:20:33, gab wrote: > This can also easily be folded in RetargetShortcuts imo. Done. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.h:524: // to point to points to |new_target_exe|. "We must go deeper". Fixed. :)
Very very nice! Do we need all of these LOGs in release builds (they will bloat chrome.dll since installer_util is linked in it) or would DLOGs suffice? Cheers! Gab https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1273: LOG(WARNING) << "Cannot find path at location " << location; On 2013/04/30 20:52:10, huangs1 wrote: > The NOTREACHED() case is only for cases where folders are expected to exist; > this is not invoked for SHORTCUT_LOCATION_TASKBAR_PINS or > SHORTCUT_LOCATION_APP_SHORTCUTS. > > Awaiting decisions on whether or not to put extra check. Ok so lets keep LOG(WARNING), but also check !PathExists()? https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1367: if (base::win::GetVersion() < base::win::VERSION_WIN7) On 2013/04/30 20:52:10, huangs1 wrote: > Modified in base_paths_win.cc. > > There are many unneeded "base::", but this is not the time to change it. I didn't meant to change it in this CL, I think it's fine to fix this in a closely following CL. https://chromiumcodereview.appspot.com/14287008/diff/44001/chrome/installer/s... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/44001/chrome/installer/s... chrome/installer/setup/uninstall.cc:362: // retargeted at some point). "Unpin all pinned-to-taskbar shortcuts that point to |chrome_exe|." I don't think this comment needs to be more specific than that, it implies all pins (i.e. user-pinned or installer-pinned), plus any future reader of this code will not know this wasn't how it used to work :). https://chromiumcodereview.appspot.com/14287008/diff/44001/chrome/installer/s... chrome/installer/setup/uninstall.cc:370: // start screen for |dist|. This comment is wrong, the code actually deletes the entire folder unconditionally. https://chromiumcodereview.appspot.com/14287008/diff/44001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/44001/chrome/installer/u... chrome/installer/util/shell_util.cc:1223: // performed even if failures occur. s/performed/attempted ? (as there is no guarantee that they will actually be performed/succeed). https://chromiumcodereview.appspot.com/14287008/diff/44001/chrome/installer/u... chrome/installer/util/shell_util.cc:1240: string16(L"*") + installer::kLnkExt); Is string16() even required here? https://chromiumcodereview.appspot.com/14287008/diff/44001/chrome/installer/u... chrome/installer/util/shell_util.cc:1959: return RemoveShortcutFolder(location, dist, level); nit: wrap in {} since condition spans more than one line. https://chromiumcodereview.appspot.com/14287008/diff/44001/chrome/installer/u... chrome/installer/util/shell_util.cc:1961: if (location == SHORTCUT_LOCATION_TASKBAR_PINS) { Sweet, much cleaner :). Use a switch(location) {} block instead of ifs though. https://chromiumcodereview.appspot.com/14287008/diff/44001/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/14287008/diff/44001/chrome/installer/u... chrome/installer/util/shell_util.h:504: // remove the all-users shortcut. s/shortcut/shortcuts
PTAL. https://chromiumcodereview.appspot.com/14287008/diff/44001/chrome/installer/s... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/44001/chrome/installer/s... chrome/installer/setup/uninstall.cc:362: // retargeted at some point). On 2013/04/30 22:30:22, gab wrote: > "Unpin all pinned-to-taskbar shortcuts that point to |chrome_exe|." > > I don't think this comment needs to be more specific than that, it implies all > pins (i.e. user-pinned or installer-pinned), plus any future reader of this code > will not know this wasn't how it used to work :). Done. https://chromiumcodereview.appspot.com/14287008/diff/44001/chrome/installer/s... chrome/installer/setup/uninstall.cc:370: // start screen for |dist|. Just copying the old comment for RemoveStartScreenShortcuts(). Should have been more careful. Rephrased, PTAL. https://chromiumcodereview.appspot.com/14287008/diff/44001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/44001/chrome/installer/u... chrome/installer/util/shell_util.cc:1223: // performed even if failures occur. Ah, of course. Done. https://chromiumcodereview.appspot.com/14287008/diff/44001/chrome/installer/u... chrome/installer/util/shell_util.cc:1240: string16(L"*") + installer::kLnkExt); Yes, because we cannot add two wchar_t*'s. https://chromiumcodereview.appspot.com/14287008/diff/44001/chrome/installer/u... chrome/installer/util/shell_util.cc:1959: return RemoveShortcutFolder(location, dist, level); Moot; using "switch" now. https://chromiumcodereview.appspot.com/14287008/diff/44001/chrome/installer/u... chrome/installer/util/shell_util.cc:1961: if (location == SHORTCUT_LOCATION_TASKBAR_PINS) { On 2013/04/30 22:30:22, gab wrote: > Sweet, much cleaner :). > > Use a switch(location) {} block instead of ifs though. Done. https://chromiumcodereview.appspot.com/14287008/diff/44001/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/14287008/diff/44001/chrome/installer/u... chrome/installer/util/shell_util.h:504: // remove the all-users shortcut. Done, with more comment fixes (also for RetargetShortcuts()).
looking good. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1367: if (base::win::GetVersion() < base::win::VERSION_WIN7) On 2013/04/30 20:52:10, huangs1 wrote: > There are many unneeded "base::" Could you explain? https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/s... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/s... chrome/installer/setup/uninstall.cc:363: LOG_IF(WARNING, base::win::GetVersion() >= base::win::VERSION_WIN7) on second thought, it's a bit clunky that the caller here needs to know that taskbar pins and app shortcuts only apply to specific OS versions just for the sake of logging. is the logging here really necessary? if so, could RemoveShortcuts be changed so it returns true when not applicable (noop) rather than false? the same applies to RetargetShortcuts, i think. https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/u... chrome/installer/util/shell_util.cc:1169: if (base::win::GetVersion() < base::win::VERSION_WIN8) this check is performed in base_paths_win, so it isn't needed here. you could make it a DCHECK if you really wanted to, or make the LOG on 1174 at level DFATAL rather than ERROR. https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/u... chrome/installer/util/shell_util.cc:1173: if (!PathService::Get(base::DIR_APP_SHORTCUTS, &folder) || folder.empty()) { now that i've looked, i see that PathService will never return true and an empty path. please fix here and elsewhere. https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/u... chrome/installer/util/shell_util.cc:1232: LOG(WARNING) << "Cannot find path at location " << location; isn't this going to log warnings on Vista/Win7 and below for locations that aren't supported on those OSes? also for Win8 if there are no app shortcuts, no? https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/u... chrome/installer/util/shell_util.cc:1264: LOG(WARNING) << "Cannot find path at location " << location; same comment: isn't this going to log warnings on Vista/Win7 and below for locations that aren't supported on those OSes? also for Win8 if there are no app shortcuts, no? https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/u... chrome/installer/util/shell_util.cc:1358: if (base::win::GetVersion() < base::win::VERSION_WIN7) isn't this unnecessary with your change to base_paths_win.cc? https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/u... chrome/installer/util/shell_util.cc:1383: ShellUtil::ShortcutLocation location, if i'm reading the code correctly, this function will NOTREACHED and return false if |location| is SHORTCUT_LOCATION_APP_SHORTCUTS and the user had never created an app shortcut. please at least make note of this in the doc comment. it's probably worth removing the NOTREACHED's as well since a crash in this case isn't desirable.
PTAL. https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/32001/chrome/installer/u... chrome/installer/util/shell_util.cc:1367: if (base::win::GetVersion() < base::win::VERSION_WIN7) I was commenting on https://chromiumcodereview.appspot.com/14287008/diff/56001/base/base_paths_wi... Even though it's in namespace base, "base::" is still used everywhere. All their base is not belong to there. :) https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/s... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/s... chrome/installer/setup/uninstall.cc:363: LOG_IF(WARNING, base::win::GetVersion() >= base::win::VERSION_WIN7) Adding ShellUtil::ShortcutLocationIsExpected() to do the check. Placing check sin RemoveShortcuts() & RetargetShortcuts() rather than BatchShortcutAction() & RemoveShortcutFolder(). Replaced LOG_IF() with LOG(). https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/u... chrome/installer/util/shell_util.cc:1169: if (base::win::GetVersion() < base::win::VERSION_WIN8) With ShortcutLocationIsExpected(), code will not flow here for non-Windows 8. Adding DCHECK(), since this is a specialized routine. https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/u... chrome/installer/util/shell_util.cc:1173: if (!PathService::Get(base::DIR_APP_SHORTCUTS, &folder) || folder.empty()) { On 2013/05/01 13:35:58, grt wrote: > now that i've looked, i see that PathService will never return true and an empty > path. please fix here and elsewhere. Done. https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/u... chrome/installer/util/shell_util.cc:1232: LOG(WARNING) << "Cannot find path at location " << location; This CL is really a means to an end for a high priority CL (I need RetargetShortcuts()). To reduce risk, I want to preserve as much old behaviour, but looks like we diverged from that... Meanwhile, the warning should not appear, now that ShortcutLocationIsExpected() is added. But it's good to log it in case of a goof. Also, I don't a DCHECK should be used (this is a common routine). https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/u... chrome/installer/util/shell_util.cc:1264: LOG(WARNING) << "Cannot find path at location " << location; Same response. :) Do you mean "for non-Win8"? Otherwise I made a mistake. https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/u... chrome/installer/util/shell_util.cc:1358: if (base::win::GetVersion() < base::win::VERSION_WIN7) On 2013/05/01 13:35:58, grt wrote: > isn't this unnecessary with your change to base_paths_win.cc? Done (removed). https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/u... chrome/installer/util/shell_util.cc:1383: ShellUtil::ShortcutLocation location, I'm tempted to add if (!ShortcutLocationIsExpected(location)) return false; on the top, but decided not risk shortcuts not being created. Removing this particular NOTREACHED().
https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/u... chrome/installer/util/shell_util.cc:1264: LOG(WARNING) << "Cannot find path at location " << location; On 2013/05/01 15:56:29, huangs1 wrote: > Same response. :) > > Do you mean "for non-Win8"? No. TASKBAR_PINS does not apply to pre-Win7, APP_SHORTCUTS does not apply to pre-Win8. https://chromiumcodereview.appspot.com/14287008/diff/56001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/56001/chrome/installer/u... chrome/installer/util/shell_util.cc:1169: DCHECK(base::win::GetVersion() >= base::win::VERSION_WIN8); DCHECK_GE https://chromiumcodereview.appspot.com/14287008/diff/56001/chrome/installer/u... chrome/installer/util/shell_util.cc:1258: bool RemoveShortcutFolder(ShellUtil::ShortcutLocation location, please explicitly whitelist the |location| values for which this function is valid. as it is, an accidental call to this with something like SHORTCUT_LOCATION_DESKTOP would be very bad. https://chromiumcodereview.appspot.com/14287008/diff/56001/chrome/installer/u... chrome/installer/util/shell_util.cc:1335: bool ShellUtil::ShortcutLocationIsExpected( Expected -> Supported https://chromiumcodereview.appspot.com/14287008/diff/56001/chrome/installer/u... chrome/installer/util/shell_util.cc:1343: return base::win::GetVersion() < base::win::VERSION_WIN8; why? https://chromiumcodereview.appspot.com/14287008/diff/56001/chrome/installer/u... chrome/installer/util/shell_util.cc:1398: ShellUtil::ShortcutLocation location, what does it mean to call this function with location == SHORTCUT_LOCATION_TASKBAR_PINS? should that not be prohibited?
PTAL https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/u... chrome/installer/util/shell_util.cc:1264: LOG(WARNING) << "Cannot find path at location " << location; Right. I was thinking of the phrase "isn't this going to log warnings for non-Win8 if there are no app shortcuts, no?" The ShortcutLocationIsSupported() check from RemoveShortcuts() will limit the case of SHORTCUT_LOCATION_APP_SHORTCUTS to Windows 8+. https://chromiumcodereview.appspot.com/14287008/diff/56001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/56001/chrome/installer/u... chrome/installer/util/shell_util.cc:1169: DCHECK(base::win::GetVersion() >= base::win::VERSION_WIN8); On 2013/05/01 16:33:43, grt wrote: > DCHECK_GE Done. https://chromiumcodereview.appspot.com/14287008/diff/56001/chrome/installer/u... chrome/installer/util/shell_util.cc:1258: bool RemoveShortcutFolder(ShellUtil::ShortcutLocation location, On 2013/05/01 16:33:43, grt wrote: > please explicitly whitelist the |location| values for which this function is > valid. as it is, an accidental call to this with something like > SHORTCUT_LOCATION_DESKTOP would be very bad. Done. https://chromiumcodereview.appspot.com/14287008/diff/56001/chrome/installer/u... chrome/installer/util/shell_util.cc:1335: bool ShellUtil::ShortcutLocationIsExpected( On 2013/05/01 16:33:43, grt wrote: > Expected -> Supported Done. https://chromiumcodereview.appspot.com/14287008/diff/56001/chrome/installer/u... chrome/installer/util/shell_util.cc:1343: return base::win::GetVersion() < base::win::VERSION_WIN8; I thought Windows 8 does not have a Start Menu? Unless it's used for something else. Since I'm not an expert, removing. https://chromiumcodereview.appspot.com/14287008/diff/56001/chrome/installer/u... chrome/installer/util/shell_util.cc:1398: ShellUtil::ShortcutLocation location, Oops, I was going to abandon the call to ShortcutLocationIsSupported(). Meanwhile, adding explicit whitelist again, to preserve old behaviour (since I added SHORTCUT_LOCATION_TASKBAR_PINS and SHORTCUT_LOCATION_APP_SHORTCUTS).
OOO but quick comment! Cheers, Gab https://chromiumcodereview.appspot.com/14287008/diff/56001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/56001/chrome/installer/u... chrome/installer/util/shell_util.cc:1343: return base::win::GetVersion() < base::win::VERSION_WIN8; On 2013/05/01 17:42:09, huangs1 wrote: > I thought Windows 8 does not have a Start Menu? Unless it's used for something > else. Since I'm not an expert, removing. The Windows 8 start screen replaces the start menu, but uses the same shortcut folder and we treat it the same in the installer.
https://chromiumcodereview.appspot.com/14287008/diff/56001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/56001/chrome/installer/u... chrome/installer/util/shell_util.cc:1343: return base::win::GetVersion() < base::win::VERSION_WIN8; Ah, good to know. Fortunately I removed, so no extra code change necessary. :)
lgtm. i think this change merits extensive testing. i doubt we have automated tests that cover this adequately. gab: do you have a test plan for the previous shortcut code that QA can execute? https://chromiumcodereview.appspot.com/14287008/diff/60001/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/14287008/diff/60001/chrome/installer/u... chrome/installer/util/shell_util.h:315: // |dist| gives the type of browser distribution currently in use. please add: // |location| may be one of SHORTCUT_LOCATION_DESKTOP, SHORTCUT_LOCATION_QUICK_LAUNCH, or SHORTCUT_LOCATION_START_MENU.
OWNER review to darin@ for: base/base_paths_win.cc: PathProviderWin(): adding Windows 7+ check for DIR_TASKBAR_PINS.
Uploaded. Also verified proper shortcut deletion in Windows 7 & Windows 8 for basic install / uninstall flows. In Windows 8, the Application Shortcuts in C:\Users\Samuel\AppData\Local\Microsoft\Windows\Application Shortcuts does not get created (also see this with Dev channel Chrome on Windows 8), so its attempted removal created logging message. I think this is expected?
On 2013/05/01 20:19:28, huangs1 wrote: > Uploaded. > > Also verified proper shortcut deletion in Windows 7 & Windows 8 for basic > install / uninstall flows. > > In Windows 8, the Application Shortcuts in > > C:\Users\Samuel\AppData\Local\Microsoft\Windows\Application Shortcuts > > does not get created (also see this with Dev channel Chrome on Windows 8), so > its attempted removal created logging message. I think this is expected? Expected based on the code, but undesirable. In my comment here: https://chromiumcodereview.appspot.com/14287008/diff/51001/chrome/installer/u..., I was trying to point out that we shouldn't log a WARNING for entirely acceptable (and expected) scenarios.
gab@: I changed "retarget" to "update". Questions: - Would weird side effect arise if we're now allowed to batch update all properties of shortcut? - Is it okay that I pass base::win::ShortcutProperties instead of ShellUtil::ShortcutOperation? PTAL.
Did another full-pass, this CL is awesome and makes the code much better (on top of adding new functionality)! Thanks! Gab https://codereview.chromium.org/14287008/diff/78001/chrome/installer/util/she... File chrome/installer/util/shell_util.cc (left): https://codereview.chromium.org/14287008/diff/78001/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1304: NOTREACHED(); I think this NOTREACHED() should stay as this should really never happen for the whitelisted locations. Technically a NOTREACHED() in GetShortcutPath() will be hit before, but a NOTREACHED() here helps the reader see that this is not a regular check, but rather something that should never happen. https://codereview.chromium.org/14287008/diff/78001/chrome/installer/util/she... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/14287008/diff/78001/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1384: return GetAppShortcutsFolder(dist, level, path); As part of your follow-up CL for base_paths_win, I think it would be nice to add an entry for this folder instead of having a custom method here that is different from the way all other paths are obtained (not a blocker for this CL though). https://codereview.chromium.org/14287008/diff/78001/chrome/installer/util/she... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/14287008/diff/78001/chrome/installer/util/she... chrome/installer/util/shell_util.h:522: // Also attempts to unpin the removed shortcut(s) from the taskbar. This part is no longer true. https://codereview.chromium.org/14287008/diff/78001/chrome/installer/util/she... chrome/installer/util/shell_util.h:524: // vacuous case no shortcuts are found. // Returns true if all shortcuts pointing to |target_exe| are successfully deleted, including the case where no such shortcuts are found. https://codereview.chromium.org/14287008/diff/78001/chrome/installer/util/she... chrome/installer/util/shell_util.h:533: // vacuous case no shortcuts are found. I'd suggest: // Returns true if all shortcuts pointing to |target_exe| are successfully updated, including the case where no such shortcuts are found. https://codereview.chromium.org/14287008/diff/78001/chrome/installer/util/she... chrome/installer/util/shell_util.h:539: const base::win::ShortcutProperties& shortcut_properties); Use a ShellUtil::ShortcutProperties and use TranslateShortcutProperties() internally to map it to a base::win::ShortcutProperties. This avoids letting the caller take care of details like the working_directory.
Also, ping on an earlier comment, can you avoid duplicating code in tests? Cheers, Gab https://codereview.chromium.org/14287008/diff/78001/chrome/installer/util/she... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/14287008/diff/78001/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1384: return GetAppShortcutsFolder(dist, level, path); On 2013/05/02 19:20:50, gab wrote: > As part of your follow-up CL for base_paths_win, I think it would be nice to add > an entry for this folder instead of having a custom method here that is > different from the way all other paths are obtained (not a blocker for this CL > though). Maybe add a TODO for this?
PTAL. https://chromiumcodereview.appspot.com/14287008/diff/78001/chrome/installer/u... File chrome/installer/util/shell_util.cc (left): https://chromiumcodereview.appspot.com/14287008/diff/78001/chrome/installer/u... chrome/installer/util/shell_util.cc:1304: NOTREACHED(); Okay, added back (but without checking for system_shortcut_path.empty()). https://chromiumcodereview.appspot.com/14287008/diff/78001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/78001/chrome/installer/u... chrome/installer/util/shell_util.cc:1384: return GetAppShortcutsFolder(dist, level, path); Agreed. https://chromiumcodereview.appspot.com/14287008/diff/78001/chrome/installer/u... chrome/installer/util/shell_util.cc:1384: return GetAppShortcutsFolder(dist, level, path); Added TODO. https://chromiumcodereview.appspot.com/14287008/diff/78001/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/14287008/diff/78001/chrome/installer/u... chrome/installer/util/shell_util.h:522: // Also attempts to unpin the removed shortcut(s) from the taskbar. Deleted. https://chromiumcodereview.appspot.com/14287008/diff/78001/chrome/installer/u... chrome/installer/util/shell_util.h:524: // vacuous case no shortcuts are found. On 2013/05/02 19:20:50, gab wrote: > // Returns true if all shortcuts pointing to |target_exe| are successfully > deleted, including the case where no such shortcuts are found. Done. https://chromiumcodereview.appspot.com/14287008/diff/78001/chrome/installer/u... chrome/installer/util/shell_util.h:533: // vacuous case no shortcuts are found. On 2013/05/02 19:20:50, gab wrote: > I'd suggest: > // Returns true if all shortcuts pointing to |target_exe| are successfully > updated, including the case where no such shortcuts are found. Done. https://chromiumcodereview.appspot.com/14287008/diff/78001/chrome/installer/u... chrome/installer/util/shell_util.h:539: const base::win::ShortcutProperties& shortcut_properties); Done. Doing translation only once in UpdateShortcuts(), since it would be silly for ShortcutOpUpdate() to do it repeatedly.
Almost there! After looking at it again, I'm fine with keeping the code duplication from the delete tests to the migrate tests for now, see comments below. Cheers, Gab https://codereview.chromium.org/14287008/diff/78001/chrome/installer/util/she... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/14287008/diff/78001/chrome/installer/util/she... chrome/installer/util/shell_util.h:539: const base::win::ShortcutProperties& shortcut_properties); On 2013/05/02 20:48:02, huangs1 wrote: > Done. Doing translation only once in UpdateShortcuts(), since it would be silly > for ShortcutOpUpdate() to do it repeatedly. Indeed :)! https://codereview.chromium.org/14287008/diff/85001/chrome/installer/util/she... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/14287008/diff/85001/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1384: // TODO(huangs): Refactor away GetAppShortcutsFolder(). // TODO(huangs): Move GetAppShortcutsFolder() logic into base_paths_win. https://codereview.chromium.org/14287008/diff/85001/chrome/installer/util/she... File chrome/installer/util/shell_util_unittest.cc (right): https://codereview.chromium.org/14287008/diff/85001/chrome/installer/util/she... chrome/installer/util/shell_util_unittest.cc:80: // Creates a shortcut in |temp_dir| using |test_properties_|. The shortcut This is not actually true, your method always creates it on the desktop, temp_dir_ is only used for the verification. https://codereview.chromium.org/14287008/diff/85001/chrome/installer/util/she... chrome/installer/util/shell_util_unittest.cc:82: base::FilePath CreateTestShortcutHelper( I feel this method makes the tests slightly less readable and doesn't really save that much code duplication, please remove it. I realize that doing more than this would be more involved and I'm ok with keeping the task of deduping some of this test code for another day. https://codereview.chromium.org/14287008/diff/85001/chrome/installer/util/she... chrome/installer/util/shell_util_unittest.cc:85: bool res = ShellUtil::CreateOrUpdateShortcut( s/res/result https://codereview.chromium.org/14287008/diff/85001/chrome/installer/util/she... chrome/installer/util/shell_util_unittest.cc:424: ASSERT_TRUE(base::win::ResolveShortcut(shortcut_path, &target_path, NULL)); Please use ValidateChromeShortcut to validate all of this shortcut's properties (and in other tests below -- to make sure no other properties were affected instead of only verifying the target -- see other tests for Create/Replace/Update that do this already in this file).
PTAL. https://chromiumcodereview.appspot.com/14287008/diff/60001/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/14287008/diff/60001/chrome/installer/u... chrome/installer/util/shell_util.h:315: // |dist| gives the type of browser distribution currently in use. On 2013/05/01 19:53:49, grt wrote: > please add: > // |location| may be one of SHORTCUT_LOCATION_DESKTOP, > SHORTCUT_LOCATION_QUICK_LAUNCH, or SHORTCUT_LOCATION_START_MENU. Done. https://chromiumcodereview.appspot.com/14287008/diff/85001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/85001/chrome/installer/u... chrome/installer/util/shell_util.cc:1384: // TODO(huangs): Refactor away GetAppShortcutsFolder(). On 2013/05/02 21:23:59, gab wrote: > // TODO(huangs): Move GetAppShortcutsFolder() logic into base_paths_win. Done. https://chromiumcodereview.appspot.com/14287008/diff/85001/chrome/installer/u... File chrome/installer/util/shell_util_unittest.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/85001/chrome/installer/u... chrome/installer/util/shell_util_unittest.cc:80: // Creates a shortcut in |temp_dir| using |test_properties_|. The shortcut Moot; undoing change. https://chromiumcodereview.appspot.com/14287008/diff/85001/chrome/installer/u... chrome/installer/util/shell_util_unittest.cc:82: base::FilePath CreateTestShortcutHelper( On 2013/05/02 21:23:59, gab wrote: > I feel this method makes the tests slightly less readable and doesn't really > save that much code duplication, please remove it. > > I realize that doing more than this would be more involved and I'm ok with > keeping the task of deduping some of this test code for another day. Done. https://chromiumcodereview.appspot.com/14287008/diff/85001/chrome/installer/u... chrome/installer/util/shell_util_unittest.cc:85: bool res = ShellUtil::CreateOrUpdateShortcut( Okay, do so in the future. https://chromiumcodereview.appspot.com/14287008/diff/85001/chrome/installer/u... chrome/installer/util/shell_util_unittest.cc:424: ASSERT_TRUE(base::win::ResolveShortcut(shortcut_path, &target_path, NULL)); Not using ValidateChromeShortcut for now, per discussion. But added TODO's.
Awesome, thanks a lot! LGTM w/ slight nit below Cheers! Gab https://codereview.chromium.org/14287008/diff/92001/chrome/installer/util/she... File chrome/installer/util/shell_util_unittest.cc (right): https://codereview.chromium.org/14287008/diff/92001/chrome/installer/util/she... chrome/installer/util/shell_util_unittest.cc:430: ShellUtil::ShortcutProperties properties(ShellUtil::CURRENT_USER); nit: s/properties/updated_properties (here and in tests below). This is the naming scheme used in this file for such update properties, makes it clearer at the callsite below imo).
I agree with Greg that this will need massive testing. There is a detailed plan of which shortcuts are installed when at http://www.chromium.org/developers/installer#shortcuts, this CL also affects shortcut deletion a lot and we should make sure it deletes all shortcuts it needs to delete, but no more than that (i.e. if there are multiple shortcuts to two separate chrome.exe, uninstalling one should delete all of its shortcuts ({chrome, profile, app} shortcuts), but not the other chrome.exe's shortcuts... and that for all ShortcutLocations). Please coordinate with QA to make sure they test this in the next official release. Thanks! Gab
https://chromiumcodereview.appspot.com/14287008/diff/92001/chrome/installer/u... File chrome/installer/util/shell_util_unittest.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/92001/chrome/installer/u... chrome/installer/util/shell_util_unittest.cc:430: ShellUtil::ShortcutProperties properties(ShellUtil::CURRENT_USER); On 2013/05/02 22:05:40, gab wrote: > nit: s/properties/updated_properties (here and in tests below). This is the > naming scheme used in this file for such update properties, makes it clearer at > the callsite below imo). Done (for all 3 tests, of course).
OWNER review for sky@ for chrome/browser/chrome_browser_main_win.cc: Fixing caller due to changed interface (and rename) of ShellUtil::RemoveShortcuts().
See below, LGTM++! Thanks! Gab https://chromiumcodereview.appspot.com/14287008/diff/97002/chrome/browser/chr... File chrome/browser/chrome_browser_main_win.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/97002/chrome/browser/chr... chrome/browser/chrome_browser_main_win.cc:148: ShellUtil::CURRENT_USER, chrome_exe)) { You can TBR sky for this change, it's only adapting to an interface change which has already been reviewed by OWNERs of the said interface. https://chromiumcodereview.appspot.com/14287008/diff/97002/chrome/browser/chr... chrome/browser/chrome_browser_main_win.cc:148: ShellUtil::CURRENT_USER, chrome_exe)) { FWIW, we will now be able to remove this legacy code :) (a follow-up CL is fine), it was here to ensure user-level shortcuts get removed, but now that all shortcuts are removed this shouldn't be necessary anymore :) -- we only have to make sure we also remove per-user shortcuts in uninstall.cc for the user running the uninstall (e.g., CURRENT_USER should work) on system-level installs!
https://chromiumcodereview.appspot.com/14287008/diff/97002/chrome/browser/chr... File chrome/browser/chrome_browser_main_win.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/97002/chrome/browser/chr... chrome/browser/chrome_browser_main_win.cc:148: ShellUtil::CURRENT_USER, chrome_exe)) { SGTM. :)
still lgtm from me. two nits below. https://chromiumcodereview.appspot.com/14287008/diff/24008/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/14287008/diff/24008/chrome/installer/u... chrome/installer/util/shell_util.h:49: SHORTCUT_LOCATION_APP_SHORTCUTS, // base::win::VERSION_WIN8 + nit: two spaces before comment https://chromiumcodereview.appspot.com/14287008/diff/24008/chrome/installer/u... chrome/installer/util/shell_util.h:528: // Iterates over all shortcuts at |location| that targets |target_exe|, this comment has some grammar issues. suggested text: "Applies the updates in |shortcut_properties| to all shortcuts in |location| that target |target_exe|."
Done. For some reason commit queue is not working for me now. https://chromiumcodereview.appspot.com/14287008/diff/24008/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/14287008/diff/24008/chrome/installer/u... chrome/installer/util/shell_util.h:49: SHORTCUT_LOCATION_APP_SHORTCUTS, // base::win::VERSION_WIN8 + On 2013/05/03 15:28:32, grt wrote: > nit: two spaces before comment Done. https://chromiumcodereview.appspot.com/14287008/diff/24008/chrome/installer/u... chrome/installer/util/shell_util.h:528: // Iterates over all shortcuts at |location| that targets |target_exe|, On 2013/05/03 15:28:32, grt wrote: > this comment has some grammar issues. suggested text: > "Applies the updates in |shortcut_properties| to all shortcuts in |location| > that target |target_exe|." Done.
chrome_browser_main_win.cc LGTM
Message was sent while issue was closed.
Committed patchset #19 manually as r198514 (presubmit successful).
Message was sent while issue was closed.
Asked gab@ to dcommit for me, as this is blocking time-critical task -- can sort out my dcommit woes later (cmp@ is helping me with this).
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/14287008/diff/22002/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/22002/chrome/installer/u... chrome/installer/util/shell_util.cc:1210: shortcut_path, shortcut_properties, base::win::SHORTCUT_REPLACE_EXISTING); Arg... my bad, this should be using SHORTCUT_UPDATE_EXISTING.
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/14287008/diff/22002/chrome/installer/u... File chrome/installer/util/shell_util_unittest.cc (right): https://chromiumcodereview.appspot.com/14287008/diff/22002/chrome/installer/u... chrome/installer/util/shell_util_unittest.cc:463: // TODO(huangs): Fix ValidateChromeShortcut() and use it. And the fact that we checked it in with SHORTCUT_REPLACE_EXISTING instead of SHORTCUT_UPDATE_EXISTING just proves my point here that this test should really test that the update only affected the desired properties and that all other properties remained the same (as all other tests in this file do) -- I was originally satisfied with this for now because it at least tested one unmodified property (arguments), but I forgot arguments are special cased and ported over when overwriting shortcuts to avoid overwriting custom arguments some people add on their shortcuts.... Please fix this as well, it would have catched this issue prior to checking it in :(. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
