Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
108193019:
Installer: adding ResolveShortcutProperties(); updating shortcut icons during shortcut migration. (Closed)
|
Created:
7 years ago by huangs Modified:
6 years, 2 months ago CC:
chromium-reviews, grt+watch_chromium.org, erikwright+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDuring the self-destruct flow (user-level Chrome being replaced by system-level Chrome), existing code to retarget shortcuts failed to update icon executables. This CL fixes this. The heuristics are summarized below:
1. Only considering shortcuts with parameters .
2. Shortcuts whose icon file == old target (user-level chrome.exe) gets updated to new target (system-level chrome.exe).
3. Shortcut index is kept same as before.
(1) lets us delete user-level (pure) Chrome shortcut since system-level Chrome shortcut is already present. (2) lets us select migrate App Launcher shortcut (points to chrome.exe), without changing app shortcuts (points to fixed .ico files). (3) prevents App Launcher shortcut from mutating into Chrome shortcut (it assumes icon resources enumeration don't change.
To make (2) and (3) possible, we need to read the icon file and icon index. This is done by generalizing ResolveShortcut() to ResolveShortcutProperties().
The new logic requires a new flow in ShellUtils, and no longer uses the UpdateShortcutsWithArgs() flow.
BUG=326562
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243642
Patch Set 1 #Patch Set 2 : Adding ResolveShortcutProperties() to read thumbnails; requiring old target to matchs icon to for i… #
Total comments: 24
Patch Set 3 : Removing 'shortcut update' flow; adding unit tests for shortcut retargeting; refactoring unittests. #
Total comments: 27
Patch Set 4 : Completing ResolveShortcutProperties() and adding tests; more refactoring for ShellUtilShortcutTest. #
Total comments: 23
Patch Set 5 : Cleanup; adding aggregated ShortcutProperties::PROPERTIES_* masks. #Patch Set 6 : Removing helper CreateTestDesktopShortcut() in shell_util_unittest.cc; inlining instead. #
Total comments: 32
Patch Set 7 : Nits; fixing test comments and coverage. #
Total comments: 2
Patch Set 8 : Nits. #Patch Set 9 : Sync. #Patch Set 10 : Fixing path comparison in ShortcutTest. #
Total comments: 4
Patch Set 11 : Cleanups; adding test code to investigate FilePath compare failure in test. #Patch Set 12 : Using ValidatePathsAreEqual() to fix tests; removing test code. #
Messages
Total messages: 33 (0 generated)
The issue is NOT ready for review; the current code tramples over user-created app icons, making them all Chrome! Looks like we will need more specialized code to read the shortcut icons, and act as appropriate. Perhaps it's time to make ResolveShortcut() more sophisticated?
On 2013/12/19 08:24:25, huangs1 wrote: > The issue is NOT ready for review; the current code tramples over user-created > app icons, making them all Chrome! > > Looks like we will need more specialized code to read the shortcut icons, and > act as appropriate. Perhaps it's time to make ResolveShortcut() more > sophisticated? Making ResolveShortcut() optionally return the icon_path sgtm. Take a look at shortcut.cc and shortcut_unittest.cc on ways to achieve that. Cheers! Gab
PTAL.
Looks great, thanks! https://codereview.chromium.org/108193019/diff/20001/base/win/shortcut.cc File base/win/shortcut.cc (right): https://codereview.chromium.org/108193019/diff/20001/base/win/shortcut.cc#new... base/win/shortcut.cc:177: ShortcutProperties* p) { s/p/properties Chromium doesn't encourage abbreviated variable names. https://codereview.chromium.org/108193019/diff/20001/base/win/shortcut.cc#new... base/win/shortcut.cc:178: DCHECK(p != NULL); DCHECK(properties); (i.e. no need for "!= NULL") Also DCHECK(options); otherwise it's kind of pointless to call this (and expensive as it loads the shortcut anyways). https://codereview.chromium.org/108193019/diff/20001/base/win/shortcut.cc#new... base/win/shortcut.cc:217: result = i_shell_link->GetArguments(temp, MAX_PATH); Looks like |result| is a write-only variable here and above. Consider inlining all the calls down into the FAILED() blocks like you did in your new code below, I find this cleaner. https://codereview.chromium.org/108193019/diff/20001/base/win/shortcut.cc#new... base/win/shortcut.cc:242: options |= args ? ShortcutProperties::PROPERTIES_ARGUMENTS : 0; Change these two lines to: if (target_path) options |= ShortcutProperties::PROPERTIES_TARGET; if (args) options |= ShortcutProperties::PROPERTIES_ARGUMENT; Although this results in the same thing I prefer this to OR'ing with 0. https://codereview.chromium.org/108193019/diff/20001/chrome/installer/util/sh... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/108193019/diff/20001/chrome/installer/util/sh... chrome/installer/util/shell_util.cc:1278: if (base::win::ResolveShortcutProperties(shortcut_path, Wrap |shortcut_path| as well. https://codereview.chromium.org/108193019/diff/20001/chrome/installer/util/sh... chrome/installer/util/shell_util.cc:1278: if (base::win::ResolveShortcutProperties(shortcut_path, Unless you strongly oppose, I don't think we should stop retargeting if getting the old icon fails; i.e. retarget the exe just not the icon in that case. https://codereview.chromium.org/108193019/diff/20001/chrome/installer/util/sh... chrome/installer/util/shell_util.cc:1279: base::win::ShortcutProperties::PROPERTIES_ICON, &old_prop)) { Both the ShortcutFilter and this action will end up resolving the same shortcut back to back, one for the args another for the icon :(. Just pointing this out as it's an unfortunate side-effect of the current design, but I don't think there is a clean way to solve this (and it's not worth making a hacky way to solve it unless we could prove that this is indeed way too slow in practice -- which I doubt matters). https://codereview.chromium.org/108193019/diff/20001/chrome/installer/util/sh... chrome/installer/util/shell_util.cc:1282: if (old_prop.icon == old_target_path) tl;dr; use InstallUtil::ProgramCompare here. String comparison of paths isn't reliable (on Windows you at least want to compare FilePaths to deal with case-insensitiveness); in this case however you'll want to use InstallUtil::ProgramCompare which can deal with the strings not matching yet this being the same path (i.e., imagine the IShellLink interface returns an 8.3 path to the shortcut and |new_target_path| is the full path). https://codereview.chromium.org/108193019/diff/20001/chrome/installer/util/sh... chrome/installer/util/shell_util.cc:1289: LOG(ERROR) << "Failed to retarget " << shortcut_path.value(); Rather than putting a generic ERROR at the bottom, prefer the following pattern: if (!first_thing) { LOG(ERROR) << "first thing failed"; return false; } .... if (!second_thing) { LOG(ERROR) << "second thing failed"; return false; } return true; It avoids nesting and provides a clear ERROR case for each failure case. https://codereview.chromium.org/108193019/diff/20001/chrome/installer/util/sh... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/108193019/diff/20001/chrome/installer/util/sh... chrome/installer/util/shell_util.h:533: static bool UpdateShortcutsWithArgs( I don't think this is used anymore, remove it, we can always bring it back later if it becomes useful again. https://codereview.chromium.org/108193019/diff/20001/chrome/installer/util/sh... chrome/installer/util/shell_util.h:540: // Updates the target of all shortcuts in |location| that satisfie the s/satisfie/satisfy https://codereview.chromium.org/108193019/diff/20001/chrome/installer/util/sh... chrome/installer/util/shell_util.h:548: static bool RetargetShortcutsWithArgs( Add tests for this method; you can most likely re-use most of the tests from UpdateShortcutWithArgs.
PTAL. shell_util_unittest.cc was refactored to reduce boilerplate code to create a test shortcut. The number of lines stayed the same, though a new test is added! :) https://codereview.chromium.org/108193019/diff/20001/base/win/shortcut.cc File base/win/shortcut.cc (right): https://codereview.chromium.org/108193019/diff/20001/base/win/shortcut.cc#new... base/win/shortcut.cc:177: ShortcutProperties* p) { On 2013/12/23 14:25:01, gab wrote: > s/p/properties > > Chromium doesn't encourage abbreviated variable names. Done (also .h and comments). https://codereview.chromium.org/108193019/diff/20001/base/win/shortcut.cc#new... base/win/shortcut.cc:178: DCHECK(p != NULL); On 2013/12/23 14:25:01, gab wrote: > DCHECK(properties); > > (i.e. no need for "!= NULL") > > Also DCHECK(options); otherwise it's kind of pointless to call this (and > expensive as it loads the shortcut anyways). Done. https://codereview.chromium.org/108193019/diff/20001/base/win/shortcut.cc#new... base/win/shortcut.cc:217: result = i_shell_link->GetArguments(temp, MAX_PATH); On 2013/12/23 14:25:01, gab wrote: > Looks like |result| is a write-only variable here and above. > > Consider inlining all the calls down into the FAILED() blocks like you did in > your new code below, I find this cleaner. Done. https://codereview.chromium.org/108193019/diff/20001/base/win/shortcut.cc#new... base/win/shortcut.cc:242: options |= args ? ShortcutProperties::PROPERTIES_ARGUMENTS : 0; Done. Also, being defensive: return true if options == 0, since the old behaviour did not forbid target_path == NULL and args == NULL, but we ResolveShortcutProperties() now has DCHECK. https://codereview.chromium.org/108193019/diff/20001/chrome/installer/util/sh... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/108193019/diff/20001/chrome/installer/util/sh... chrome/installer/util/shell_util.cc:1278: if (base::win::ResolveShortcutProperties(shortcut_path, On 2013/12/23 14:25:01, gab wrote: > Wrap |shortcut_path| as well. Done, but |old_prop| now gets wrapped, too, so now 1 param per line. https://codereview.chromium.org/108193019/diff/20001/chrome/installer/util/sh... chrome/installer/util/shell_util.cc:1278: if (base::win::ResolveShortcutProperties(shortcut_path, Makes sense. Restructured code a bit. https://codereview.chromium.org/108193019/diff/20001/chrome/installer/util/sh... chrome/installer/util/shell_util.cc:1279: base::win::ShortcutProperties::PROPERTIES_ICON, &old_prop)) { Yeah I felt a bit dirty about this. :) I think we should restructure BatchShortcutAction() by separating it into 2 parts: (1) Routine to resolve a list of shortcuts with filter, storing it into a vector (with all info). (2) Routine to take vector of shortcut, and apply batch operations. This probably should not be done in this CL. Adding TODO to BatchShortcutAction(). https://codereview.chromium.org/108193019/diff/20001/chrome/installer/util/sh... chrome/installer/util/shell_util.cc:1282: if (old_prop.icon == old_target_path) Ah yes. Done. https://codereview.chromium.org/108193019/diff/20001/chrome/installer/util/sh... chrome/installer/util/shell_util.cc:1289: LOG(ERROR) << "Failed to retarget " << shortcut_path.value(); There is only 1 error now, though I'm adding warning for the case where we fail to resolve the icon. https://codereview.chromium.org/108193019/diff/20001/chrome/installer/util/sh... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/108193019/diff/20001/chrome/installer/util/sh... chrome/installer/util/shell_util.h:533: static bool UpdateShortcutsWithArgs( Removed, along with ShortcutOpUpdate() in the .cc file. https://codereview.chromium.org/108193019/diff/20001/chrome/installer/util/sh... chrome/installer/util/shell_util.h:540: // Updates the target of all shortcuts in |location| that satisfie the On 2013/12/23 14:25:01, gab wrote: > s/satisfie/satisfy Done. https://codereview.chromium.org/108193019/diff/20001/chrome/installer/util/sh... chrome/installer/util/shell_util.h:548: static bool RetargetShortcutsWithArgs( On 2013/12/23 14:25:01, gab wrote: > Add tests for this method; you can most likely re-use most of the tests from > UpdateShortcutWithArgs. Done.
https://codereview.chromium.org/108193019/diff/110001/chrome/installer/util/s... File chrome/installer/util/shell_util_unittest.cc (right): https://codereview.chromium.org/108193019/diff/110001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:92: void CreateTestDesktopShortcut(base::FilePath* shortcut_path_out) { Cannot simply return base::FilePath, because ASSERT_TRUE() triggers the error message error C2664: 'base::FilePath::FilePath(const base::FilePath &)' : cannot convert parameter 1 from 'void' to 'const base::FilePath &' Expressions of type void cannot be converted to other types
lg, mostly comments about test. Cheers! Gab https://codereview.chromium.org/108193019/diff/110001/base/win/shortcut.cc File base/win/shortcut.cc (right): https://codereview.chromium.org/108193019/diff/110001/base/win/shortcut.cc#ne... base/win/shortcut.cc:179: DCHECK(options); Switch order to match argument order (and/or group with &&). https://codereview.chromium.org/108193019/diff/110001/base/win/shortcut.cc#ne... base/win/shortcut.cc:199: properties->options = 0; Empty line below (otherwise it feels like this is only related to the block below). Also add a comment like: // Reset |properties|. above. https://codereview.chromium.org/108193019/diff/110001/base/win/shortcut.cc#ne... base/win/shortcut.cc:227: // Implement other options if the need arise. Add to the comment in the header to mention which options are currently supported. And add: // No unsupported options. DCHECK_EQ(0, options & (~ShortcutProperties::PROPERTIES_TARGET & ~ShortcutProperties::PROPERTIES_ARGUMENTS & ~ShortcutProperties::PROPERTIES_ICON)); at the top of this method. https://codereview.chromium.org/108193019/diff/110001/base/win/shortcut.cc#ne... base/win/shortcut.cc:241: return true; // Vacuous success if we want nothing. I don't think it makes sense to call ResolveShortcut and want nothing. Update the header comment to disallow this and let the DCHECK hit in ResolveShortcutProperties. https://codereview.chromium.org/108193019/diff/110001/base/win/shortcut.cc#ne... base/win/shortcut.cc:243: ShortcutProperties p; s/p/properties https://codereview.chromium.org/108193019/diff/110001/base/win/shortcut.h File base/win/shortcut.h (right): https://codereview.chromium.org/108193019/diff/110001/base/win/shortcut.h#new... base/win/shortcut.h:127: // and intermediate values written to |properties| should be ignored. Remove leading space on this line and ',' on line 126. https://codereview.chromium.org/108193019/diff/110001/base/win/shortcut.h#new... base/win/shortcut.h:132: // Resolve Windows shortcut (.LNK file). s/Resolve/Resolves https://codereview.chromium.org/108193019/diff/110001/base/win/shortcut.h#new... base/win/shortcut.h:134: // cases of resolving target and arguments. |target_path| and |args| are s/commonly used cases/common use case https://codereview.chromium.org/108193019/diff/110001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/108193019/diff/110001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:1282: LOG(ERROR) << "Warning: failed to resolve " << shortcut_path.value(); Is this a warning or an error? The severity is ERROR, but you put "Warning:" in the error string :). I'd say it's an error, so remove "Warning: " from the error string. https://codereview.chromium.org/108193019/diff/110001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:1290: return false; Avoid multiple return statements here by doing: bool result = base::win::CreateOrUpdateShortcutLink( shortcut_path, new_prop, base::win::SHORTCUT_UPDATE_EXISTING); LOG_IF(ERROR, !result) << "Failed to retarget " << shortcut_path.value(); return result; https://codereview.chromium.org/108193019/diff/110001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:1293: // TODO(huangs): Restructure this to separate reading/filtering from operating. Isn't this already what this is doing? This won't help avoiding the resolves needed in both the filtering and operating steps... I don't think a TODO is required here, we should only think about changing this if a real performance impact is demonstrated. https://codereview.chromium.org/108193019/diff/110001/chrome/installer/util/s... File chrome/installer/util/shell_util_unittest.cc (right): https://codereview.chromium.org/108193019/diff/110001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:92: void CreateTestDesktopShortcut(base::FilePath* shortcut_path_out) { On 2013/12/30 20:19:32, huangs1 wrote: > Cannot simply return base::FilePath, because ASSERT_TRUE() triggers the error > message > > error C2664: 'base::FilePath::FilePath(const base::FilePath &)' : cannot convert > parameter 1 from 'void' to 'const base::FilePath &' > > Expressions of type void cannot be converted to other types Right, that's because ASSERTs force a "return;", always use EXPECTs in helper test methods for that reason (ASSERTs in helper methods makes it look like the test will end on failure which is not true... it'll log an error and return from the helper method only, not exit the test). If you want to react differently on the result, then store the result and do EXPECT_TRUE(result); if (!result) return base::FilePath(); Either way... reading this some more I find the previous code easier to read than with this helper method. I find the helper adds complexity as it acts based on test_properties_ (which is a member not a method param so it's not obvious) to support various independent use cases. I understand why you want to do this, but I think the minimal code duplication in each test was better than this, please change it back. Feel free to create a "default_shortcut_name" string variable if you want to turn blocks like string16 shortcut_name( dist_->GetShortcutName(BrowserDistribution::SHORTCUT_CHROME) + installer::kLnkExt); base::FilePath shortcut_path(fake_user_desktop_.path().Append(shortcut_name)); ASSERT_TRUE(base::PathExists(shortcut_path)); into one-liner ASSERT_TRUE(base::PathExists(fake_user_desktop.path().Append(default_shortcut_name_))); Than each test has creation calls + matching verification calls; this doesn't feel like it needs a helper method to me. https://codereview.chromium.org/108193019/diff/110001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:520: // Relies on fact that |test_properties_| has non-empty arguments. To strengthen my point about not needing the helper method; I find this more explicit when the call to create the shortcut via test_properties_ is explicitly in this test.
PTAL. https://codereview.chromium.org/108193019/diff/110001/base/win/shortcut.cc File base/win/shortcut.cc (right): https://codereview.chromium.org/108193019/diff/110001/base/win/shortcut.cc#ne... base/win/shortcut.cc:179: DCHECK(options); Going with && for implicit cast to bool. https://codereview.chromium.org/108193019/diff/110001/base/win/shortcut.cc#ne... base/win/shortcut.cc:199: properties->options = 0; On 2014/01/02 15:51:46, gab wrote: > Empty line below (otherwise it feels like this is only related to the block > below). > > Also add a comment like: > > // Reset |properties|. > > above. Done. https://codereview.chromium.org/108193019/diff/110001/base/win/shortcut.cc#ne... base/win/shortcut.cc:227: // Implement other options if the need arise. I realized that base\test\test_shortcut_win.cc: ValidateShortcut() https://code.google.com/p/chromium/codesearch#chromium/src/base/test/test_sho... reads everything; so may as well do the work, and add sanity check unit test! Since everything is in, maybe I won't have to check for unsupported options? CreateOrUpdateShortcutLink() doesn't check either. https://codereview.chromium.org/108193019/diff/110001/base/win/shortcut.cc#ne... base/win/shortcut.cc:241: return true; // Vacuous success if we want nothing. Done, but adding DCHECK in this routine. https://codereview.chromium.org/108193019/diff/110001/base/win/shortcut.cc#ne... base/win/shortcut.cc:243: ShortcutProperties p; On 2014/01/02 15:51:46, gab wrote: > s/p/properties Done. https://codereview.chromium.org/108193019/diff/110001/base/win/shortcut.h File base/win/shortcut.h (right): https://codereview.chromium.org/108193019/diff/110001/base/win/shortcut.h#new... base/win/shortcut.h:127: // and intermediate values written to |properties| should be ignored. On 2014/01/02 15:51:46, gab wrote: > Remove leading space on this line and ',' on line 126. Done. https://codereview.chromium.org/108193019/diff/110001/base/win/shortcut.h#new... base/win/shortcut.h:132: // Resolve Windows shortcut (.LNK file). On 2014/01/02 15:51:46, gab wrote: > s/Resolve/Resolves Done. Also changed line 120. https://codereview.chromium.org/108193019/diff/110001/base/win/shortcut.h#new... base/win/shortcut.h:134: // cases of resolving target and arguments. |target_path| and |args| are On 2014/01/02 15:51:46, gab wrote: > s/commonly used cases/common use case Done. https://codereview.chromium.org/108193019/diff/110001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/108193019/diff/110001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:1282: LOG(ERROR) << "Warning: failed to resolve " << shortcut_path.value(); Ah, I didn't know LOG(WARNING) exists. Just removed "Warnings: ". https://codereview.chromium.org/108193019/diff/110001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:1290: return false; On 2014/01/02 15:51:46, gab wrote: > Avoid multiple return statements here by doing: > > bool result = base::win::CreateOrUpdateShortcutLink( > shortcut_path, new_prop, base::win::SHORTCUT_UPDATE_EXISTING); > LOG_IF(ERROR, !result) << "Failed to retarget " << shortcut_path.value(); > return result; Done. https://codereview.chromium.org/108193019/diff/110001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:1293: // TODO(huangs): Restructure this to separate reading/filtering from operating. Here {reading/filtering, operating} are integrated. I think it's better to have 2 routines: (1) One to read & filter, save resolved data (& shortcut path) into vector<>. (2) A simple loop over vector<>, apply operation. Anyway, removing TODO. :) https://codereview.chromium.org/108193019/diff/110001/chrome/installer/util/s... File chrome/installer/util/shell_util_unittest.cc (right): https://codereview.chromium.org/108193019/diff/110001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:92: void CreateTestDesktopShortcut(base::FilePath* shortcut_path_out) { Key changes: - Returning base::FilePath now. - Checking for existence outside this routine. - The shortcut path code is extracted from ValidateChromeShortcut() into GetExpectedShortcutPath(), and is called in CreateTestDesktopShortcut(). https://codereview.chromium.org/108193019/diff/110001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:520: // Relies on fact that |test_properties_| has non-empty arguments. Passing test_properties_ as parameter to the routine.
https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut_unitt... File base/win/shortcut_unittest.cc (right): https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:91: TEST_F(ShortcutTest, CreateAndResolveShortcutProperties) { This test takes 160 ms to 170 ms to run.
Some comments below; looks good, I really like fixing ResolveShortcut to handle all options :)! https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut.cc File base/win/shortcut.cc (right): https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut.cc#ne... base/win/shortcut.cc:179: DCHECK(options && properties); I really like supporting all properties here, but I still think we should whitelist supported properties here to force somebody adding a new property to add support here. https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut.cc#ne... base/win/shortcut.cc:248: if (S_OK != property_store->GetValue(PKEY_AppUserModel_ID, Put constant on RHS of equality-check (here and below). https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut.cc#ne... base/win/shortcut.cc:252: properties->set_app_id(pv_app_id.get().vt == VT_LPWSTR ? Use switch statement instead, on VT_EMPTY, set to "", on VT_LPWSTR, set to pwszVal; default: NOTREACHED(). Same with VT_BOOL below. https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut.h File base/win/shortcut.h (right): https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut.h#new... base/win/shortcut.h:135: // optional output variables that are ignored if NULL (but passing NULL to both I'd change the the statement in () to: (but at least one must be non-NULL) https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut_unitt... File base/win/shortcut_unittest.cc (right): https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:25: const uint32 kPropertiesAll = ShortcutProperties::PROPERTIES_TARGET | I would say it's best to put this as the last value of the ShortcutProperties enum itself; e.g. PROPERTIES_ALL = PROPERTIES_TARGET | PROPERTIES_WORKING_DIR | ...; This will make it more obvious to someone that adds a new property that they should add it to PROPERTIES_ALL as well. https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:98: EXPECT_EQ(link_properties_.options, kPropertiesAll); This will fail before Win7. https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:102: EXPECT_EQ(link_properties_.icon, properties_read_1.icon); Also test working_dir and icon_index. Please put the checks in the same order as the properties above for easy visual check that everything is being tested. https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:108: FilePath file_2(temp_dir_.path().Append(L"Link2.lnk")); I don't think testing this twice has any added value. The purpose of the second set of properties is to test update/replace, not double create. https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:124: Please also add a test for resolving a shortcut with no special properties set. https://codereview.chromium.org/108193019/diff/230001/chrome/installer/util/s... File chrome/installer/util/shell_util_unittest.cc (right): https://codereview.chromium.org/108193019/diff/230001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:120: dist_->GetShortcutName(BrowserDistribution::SHORTCUT_CHROME); I typically indent the two items of a ternary operator at the same level (just like with &&/||). i.e., unindent 4 spaces. https://codereview.chromium.org/108193019/diff/230001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:389: base::FilePath shortcut_path = CreateTestDesktopShortcut(test_properties_); So I really like that the refactoring is getting rid of building the shortcut_name everywhere by extracting GetExpectedShortcutPath() out of ValidateChromeShortcut(), but I still don't like the helper method (CreateTestDesktopShortcut()) itself because: 1) It hides the calling convention to ShellUtil::CreateOrUpdateShortcut; I like having it here as it is closely related to, e.g., the call to ShellUtil::RemoveShortcuts below; and it makes the test code easy to read. 2) The helper method really just bundles two independent things: - Creating the shortcut - Getting the expected path I don't see why a common method is required for both. I think we get rid of most of the boiler plate with GetExpectedShortcutPath() and don't think a special helper to create a specific type of shortcut (user-level desktop shortcut) is necessary.
Thanks! PTAL. +grt@: gab@ and I have conflicting views regarding need of helper function CreateTestDesktopShortcut() in shell_util_unittest.cc. We decided to consult you on this. https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut.cc File base/win/shortcut.cc (right): https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut.cc#ne... base/win/shortcut.cc:179: DCHECK(options && properties); Done. https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut.cc#ne... base/win/shortcut.cc:248: if (S_OK != property_store->GetValue(PKEY_AppUserModel_ID, On 2014/01/02 21:01:46, gab wrote: > Put constant on RHS of equality-check (here and below). Done. https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut.cc#ne... base/win/shortcut.cc:252: properties->set_app_id(pv_app_id.get().vt == VT_LPWSTR ? On 2014/01/02 21:01:46, gab wrote: > Use switch statement instead, on VT_EMPTY, set to "", on VT_LPWSTR, set to > pwszVal; default: NOTREACHED(). > > Same with VT_BOOL below. Done. https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut.h File base/win/shortcut.h (right): https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut.h#new... base/win/shortcut.h:135: // optional output variables that are ignored if NULL (but passing NULL to both On 2014/01/02 21:01:46, gab wrote: > I'd change the the statement in () to: > > (but at least one must be non-NULL) Done. https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut_unitt... File base/win/shortcut_unittest.cc (right): https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:25: const uint32 kPropertiesAll = ShortcutProperties::PROPERTIES_TARGET | Going a step further, and defining specialized masks. https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:98: EXPECT_EQ(link_properties_.options, kPropertiesAll); Ah yes. Good catch! https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:102: EXPECT_EQ(link_properties_.icon, properties_read_1.icon); On 2014/01/02 21:01:46, gab wrote: > Also test working_dir and icon_index. > > Please put the checks in the same order as the properties above for easy visual > check that everything is being tested. Done. https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:108: FilePath file_2(temp_dir_.path().Append(L"Link2.lnk")); On 2014/01/02 21:01:46, gab wrote: > I don't think testing this twice has any added value. The purpose of the second > set of properties is to test update/replace, not double create. Done; replacing this with a simpler shortcut. https://codereview.chromium.org/108193019/diff/230001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:124: On 2014/01/02 21:01:46, gab wrote: > Please also add a test for resolving a shortcut with no special properties set. Done. https://codereview.chromium.org/108193019/diff/230001/chrome/installer/util/s... File chrome/installer/util/shell_util_unittest.cc (right): https://codereview.chromium.org/108193019/diff/230001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:120: dist_->GetShortcutName(BrowserDistribution::SHORTCUT_CHROME); On 2014/01/02 21:01:46, gab wrote: > I typically indent the two items of a ternary operator at the same level (just > like with &&/||). > > i.e., unindent 4 spaces. Done. https://codereview.chromium.org/108193019/diff/230001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:389: base::FilePath shortcut_path = CreateTestDesktopShortcut(test_properties_); If we get rid of the routine, we would replace each EXPECT_TRUE(base::PathExists(CreateTestDesktopShortcut(test_properties_))); with (some spacing fix may be needed): EXPECT_TRUE(ShellUtil::CreateOrUpdateShortcut( ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_, ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS)); EXPECT_TRUE(base::PathExists(GetExpectedShortcutPath( ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist_, test_properties_))); The case where we save the path is similar. I like the helper method because: (1) It shortens code and increases uniformity. (2) It removes distraction from the "main point" of tests. Note that the shortcut creation tests were left alone; anyone who wants to understand the nuances of shortcut creation should look at those. (3) It's easier to modify en mass, e.g., if we change param to CreateOrUpdateShortcut(), we have fewer places to change. As we both feel strongly about this, let's defer the decision to grt@. :)
although i dislike repeated code, i think that a test failure will be easier to diagnose if the creation is in-line rather than in the helper function. also, i think that the setup work to create the shortcut(s) to be used in the test should use ASSERT rather than EXPECT since there's no point in running the test if the setup fails.
PTAL.
looks great, last nits I think. Cheers! Gab https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut.cc File base/win/shortcut.cc (right): https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut.cc#ne... base/win/shortcut.cc:184: } No {} https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut.cc#ne... base/win/shortcut.cc:252: != S_OK) { It's more readable to wrap in a way that the != isn't on its own, e.g.: if (property_store->GetValue(PKEY_AppUserModel_ID, pv_app_id.Receive()) != S_OK) { https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut.cc#ne... base/win/shortcut.cc:285: // Implement new properties go here. I don't think this comment is required anymore now that we handle all. It's pretty obvious that if you add a property you must add it somewhere in here and the end makes sense naturally. (imagine if every method had a comment saying "new code goes here" :)!) https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut.h File base/win/shortcut.h (right): https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut.h#new... base/win/shortcut.h:50: Remove extra empty line. https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut_unitt... File base/win/shortcut_unittest.cc (right): https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:45: if (GetVersion() >= VERSION_WIN7) { I think it's good to set these unconditionally (to test that the implementation correctly ignores them -- as opposed to trying to apply them and crash -- on older Win versions). https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:65: if (GetVersion() >= VERSION_WIN7) { Remove condition here too. https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:89: uint32 properties_use = ShortcutProperties::PROPERTIES_BASIC; s/properties_use/valid_properties https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:100: ResolveShortcutProperties(file_1, properties_use, &properties_read_1)); Still resolve on PROPERTIES_ALL and expect the method is able to skip those on its own prior to Win7 https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:122: ResolveShortcutProperties(file_2, properties_use, &properties_read_2)); PROPERTIES_ALL here too https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:126: EXPECT_EQ(L"", properties_read_2.description); Why not test all properties? This will be good in particular to test handling of VT_EMPTY for dual_mode/app_id. https://codereview.chromium.org/108193019/diff/410001/chrome/installer/util/s... File chrome/installer/util/shell_util_unittest.cc (right): https://codereview.chromium.org/108193019/diff/410001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:84: test_properties_.set_icon(icon_path, 0); I'd suggest changing this away from 0; thanks for spotting that icon_index was hardcoded to 0 in the verification and now that you made it work on any index it would be good that the test_properties_ not use 0 for a slightly better coverage IMO. https://codereview.chromium.org/108193019/diff/410001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:438: TEST_F(ShellUtilShortcutTest, RetargetShortcutsWithArgs) { Seems a test is missing to make sure shortcuts that don't point to old_exe aren't retargeted to new_exe; can you add this test as well (either to an existing test or as a new one, as you think is best). https://codereview.chromium.org/108193019/diff/410001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:509: // it has empty target, yet we passed |require_args| = true. |require_args| no longer exists right? please update this comment https://codereview.chromium.org/108193019/diff/410001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:524: test_properties_.set_icon(chrome_exe_, 3); Use test_properties_.target instead of chrome_exe_ to be explicit about the fact that it's important this be the same exe. https://codereview.chromium.org/108193019/diff/410001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:543: // Relies on fact that |test_properties_| has non-empty arguments. s/Relies on fact/Relies on the fact Here and elsewhere in this file https://codereview.chromium.org/108193019/diff/410001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:550: expected_properties1.set_icon(new_exe, 3); Move '3' into a local scope constant to be used here and on line 524. Same for '0' for shortcut 2 (for which I suggest using some index other than '0').
Thanks! PTAL (esp. changes to tests). https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut.cc File base/win/shortcut.cc (right): https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut.cc#ne... base/win/shortcut.cc:184: } On 2014/01/03 18:25:52, gab wrote: > No {} Done. https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut.cc#ne... base/win/shortcut.cc:252: != S_OK) { On 2014/01/03 18:25:52, gab wrote: > It's more readable to wrap in a way that the != isn't on its own, e.g.: > > if (property_store->GetValue(PKEY_AppUserModel_ID, > pv_app_id.Receive()) != S_OK) { Done. https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut.cc#ne... base/win/shortcut.cc:285: // Implement new properties go here. On 2014/01/03 18:25:52, gab wrote: > I don't think this comment is required anymore now that we handle all. > > It's pretty obvious that if you add a property you must add it somewhere in here > and the end makes sense naturally. > > (imagine if every method had a comment saying "new code goes here" :)!) Done; removed. https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut.h File base/win/shortcut.h (right): https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut.h#new... base/win/shortcut.h:50: On 2014/01/03 18:25:52, gab wrote: > Remove extra empty line. Done. https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut_unitt... File base/win/shortcut_unittest.cc (right): https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:45: if (GetVersion() >= VERSION_WIN7) { On 2014/01/03 18:25:52, gab wrote: > I think it's good to set these unconditionally (to test that the implementation > correctly ignores them -- as opposed to trying to apply them and crash -- on > older Win versions). Done. https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:65: if (GetVersion() >= VERSION_WIN7) { On 2014/01/03 18:25:52, gab wrote: > Remove condition here too. Done. https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:89: uint32 properties_use = ShortcutProperties::PROPERTIES_BASIC; On 2014/01/03 18:25:52, gab wrote: > s/properties_use/valid_properties Done. https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:100: ResolveShortcutProperties(file_1, properties_use, &properties_read_1)); On 2014/01/03 18:25:52, gab wrote: > Still resolve on PROPERTIES_ALL and expect the method is able to skip those on > its own prior to Win7 Done. https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:122: ResolveShortcutProperties(file_2, properties_use, &properties_read_2)); On 2014/01/03 18:25:52, gab wrote: > PROPERTIES_ALL here too Done. https://codereview.chromium.org/108193019/diff/410001/base/win/shortcut_unitt... base/win/shortcut_unittest.cc:126: EXPECT_EQ(L"", properties_read_2.description); Test for (empty) defaults. Will need to watch try job carefully, in case different versions of Windows do different things. https://codereview.chromium.org/108193019/diff/410001/chrome/installer/util/s... File chrome/installer/util/shell_util_unittest.cc (right): https://codereview.chromium.org/108193019/diff/410001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:84: test_properties_.set_icon(icon_path, 0); Done. Lucky #7. https://codereview.chromium.org/108193019/diff/410001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:438: TEST_F(ShellUtilShortcutTest, RetargetShortcutsWithArgs) { Added to the multi-case: {targets "iron.exe", icon "chrome.exe"}. Trying to retarget chrome.exe" => "manganese.exe" has no effect. Also added similar test case for shortcut removal (so a dumb algorithm that just deletes everything will fail). https://codereview.chromium.org/108193019/diff/410001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:509: // it has empty target, yet we passed |require_args| = true. On 2014/01/03 18:25:52, gab wrote: > |require_args| no longer exists right? please update this comment Done. https://codereview.chromium.org/108193019/diff/410001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:524: test_properties_.set_icon(chrome_exe_, 3); On 2014/01/03 18:25:52, gab wrote: > Use test_properties_.target instead of chrome_exe_ to be explicit about the fact > that it's important this be the same exe. Done. https://codereview.chromium.org/108193019/diff/410001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:543: // Relies on fact that |test_properties_| has non-empty arguments. On 2014/01/03 18:25:52, gab wrote: > s/Relies on fact/Relies on the fact > > Here and elsewhere in this file Done. https://codereview.chromium.org/108193019/diff/410001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:550: expected_properties1.set_icon(new_exe, 3); Done. The consts for the other 2 cases are only used once, but they deserve const since they are the main thing to test. By contrast, distinct shortcut names "Chrome 1", "Chrome 2", "Iron 3", are just used only once, so no longer stored in variable.
Thanks! PTAL (esp. changes to tests).
Thanks! PTAL (esp. changes to tests).
Awesome, lgtm :)! Did you confirm that this works locally (i.e., doesn't turn taskbar pinned icons white as we'd seen before IIRC?) Cheers! Gab https://codereview.chromium.org/108193019/diff/490001/chrome/installer/util/s... File chrome/installer/util/shell_util_unittest.cc (right): https://codereview.chromium.org/108193019/diff/490001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:535: const int kTest1IconIndex = 3; nit: name these kTestIconIndexX instead of kTestXIconIndex.
Of course; it's been working since Patch Set 1 on VM! Though I now need to run try jobs to ensure the updated unit tests work on older versions of Windows. https://codereview.chromium.org/108193019/diff/490001/chrome/installer/util/s... File chrome/installer/util/shell_util_unittest.cc (right): https://codereview.chromium.org/108193019/diff/490001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:535: const int kTest1IconIndex = 3; On 2014/01/03 21:01:13, gab wrote: > nit: name these kTestIconIndexX instead of kTestXIconIndex. Done.
OWNER review to cpu@ for base/win/shortcut* . Key changes: - Added ResolveShortcutProperties() to read subset of shortcut properties into ShortcutProperties. - ResolveShortcut() is now a wrapper for new routine. Behavior change: triggers DCHECK if both of its out params are NULL -- but no caller does this. - Updated unittest Thanks!
Ping cpu@ for review. Thanks!
lgtm https://codereview.chromium.org/108193019/diff/760001/base/win/shortcut.cc File base/win/shortcut.cc (right): https://codereview.chromium.org/108193019/diff/760001/base/win/shortcut.cc#ne... base/win/shortcut.cc:205: WCHAR temp[MAX_PATH]; use wchar_t like the rest of this file. https://codereview.chromium.org/108193019/diff/760001/base/win/shortcut.cc#ne... base/win/shortcut.cc:262: NOTREACHED() << "Unexpected variant type: " << pv_app_id.get().vt; feels like we should return false for default, since NOTREACHED is not there in release.
Thanks! Still need to investigate failure in FilePath comparison in unit tests (cannot be reproduced locally). https://codereview.chromium.org/108193019/diff/760001/base/win/shortcut.cc File base/win/shortcut.cc (right): https://codereview.chromium.org/108193019/diff/760001/base/win/shortcut.cc#ne... base/win/shortcut.cc:205: WCHAR temp[MAX_PATH]; On 2014/01/08 02:11:59, cpu wrote: > use wchar_t like the rest of this file. Done. https://codereview.chromium.org/108193019/diff/760001/base/win/shortcut.cc#ne... base/win/shortcut.cc:262: NOTREACHED() << "Unexpected variant type: " << pv_app_id.get().vt; Done; also applied below.
The test fails due to difference between: C:\Users\CHROME~2\AppData\Local\Temp\scoped_dir29796_13440\Target 1.txt C:\Users\chrome-bot\AppData\Local\Temp\scoped_dir29796_13440\Target 1.txt Alternatives to address this: 1. Move InstallUtil::ProgramCompare from installer into base, and use it. 2. Extract parts of InstallUtil::ProgramCompare() as helpers of this test. 3. Keep naive comparison, but in the test, somehow expand temp_dir_ and temp_dir_2_ to eliminate shortened "CHROME~2" stuff. I'd prefer 3. Thoughts?
On 2014/01/08 06:14:01, huangs1 wrote: > The test fails due to difference between: > C:\Users\CHROME~2\AppData\Local\Temp\scoped_dir29796_13440\Target 1.txt > C:\Users\chrome-bot\AppData\Local\Temp\scoped_dir29796_13440\Target 1.txt > > Alternatives to address this: > 1. Move InstallUtil::ProgramCompare from installer into base, and use it. > 2. Extract parts of InstallUtil::ProgramCompare() as helpers of this test. > 3. Keep naive comparison, but in the test, somehow expand temp_dir_ and > temp_dir_2_ to eliminate shortened "CHROME~2" stuff. > > I'd prefer 3. Thoughts? Yes, take a look at ValidatePathsAreEqual in test_shortcut_win.cc: https://code.google.com/p/chromium/codesearch#chromium/src/base/test/test_sho... Feel free to make that method part of the public API of test_shortcut_win.h and use it in shell_util_unittests. This method was introduced to solve the exact same problem with base shortcut_unittests, not sure why we haven't hit this before in shell_util_unittests..? Cheers! Gab
gab@, I'm exposing ValidatePathsAreEqual() from test_shortcut_win.cc. PTAL.
lgtm, thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/108193019/960001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
OWNER review for phajdan.jr@: base/test/test_shortcut_win.h base/test/test_shortcut_win.cc I'm exposing ValidatePathsAreEqual(). Thanks!.
base/test LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/108193019/960001
Message was sent while issue was closed.
Change committed as 243642 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
