Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11876027:
ProfileShortcutManagerWin: Don't create user level shortcut when system level one exists. (Closed)
|
Created:
7 years, 11 months ago by Alexei Svitkine (slow) Modified:
7 years, 11 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionMake sure profile shortcut manager does not create a user level shortcut when a system level one exists.
BUG=169495
TEST=New unit tests and manual steps described in the bug.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177256
Patch Set 1 : #
Total comments: 27
Patch Set 2 : #
Total comments: 13
Patch Set 3 : #
Total comments: 12
Patch Set 4 : #Patch Set 5 : #
Messages
Total messages: 26 (0 generated)
Gab/Sailesh, PTAL. The new tests cover all the implementation changes. I've also made the test class recreate the scoped temp dir overrides on every execution, to ensure the different test cases don't interfere with each other (i.e. there are no left over files from the last test case when the next one starts).
https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:55: fake_user_desktop_.reset( Are you moving this here to ensure that it runs once for each test? If so then I don't think that's necessary. The class will be constructed each time the test is run. https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:61: fake_system_desktop_.reset( same here https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:266: FilePath system_shortcuts_directory_; Ideally we shouldn't cache things in member variables. If you're doing this to make the code easier to use then use utility methods instead. I think the following member variables should be removed: distribution_ profile_info_cache_ exe_path_ shortcuts_directory_ system_shortcuts_directory_ This also make it easier to understand how the class works. https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:209: const FilePath possible_system_shortcut = new_system_shortcut_path https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:216: FilePath new_shortcut_path = shortcuts_directory.Append(new_shortcut_file); new_user_shortcut_path
Comments inline, do you guys have a spec of exactly what is meant to happen in all of these scenarios, seems like a detailed doc with exactly when profiles shortcuts are created and brought back down to regular shortcuts (with/without system-level shortcut being present) would be easier to follow. Cheers, Gab https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:684: // Delete the shortcut for the first profile, but keep the one for the 2nd. Why is it necessary to delete shortcut 1 first? Isn't this testing the case where you have two profile shortcuts, delete a profile, and thus the last profile shortcut becomes a regular shortcut, but it should simply get deleted if there is a system-level already present because another regular shortcut is redundant? https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:714: ASSERT_TRUE(file_util::Delete(profile_1_shortcut_path, false)); Same comment about why deleting shortcut 1? It probably makes sense to make a test case that tests specifically for that (i.e., that the desired behavior is still respected if shortcut 1 has already been deleted), but I don't think this is the core of this specific test case. https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:189: const string16& new_shortcut_file) { optional nit: Rename these parameters to old_shortcut_filename and new_shortcut_filename... from the code they look like filenames, not the file itself. https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:205: DCHECK(ShellUtil::GetShortcutPath(ShellUtil::SHORTCUT_LOCATION_DESKTOP, Take the call out of the DCHECK, store the result and DCHECK on that. Or instead do: if (!ShellUtil::GetShortcutPath...) NOTREACHED(); Otherwise this code will not run in Release builds.. Also if this fails you probably don't want to keep going with the verification. https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:279: operation = ShellUtil::SHELL_SHORTCUT_CREATE_IF_NO_SYSTEM_LEVEL; This will prevent creation of all profile shortcuts when a system-level shortcut is present, I think you want to only prevent creation of the shortcut when it is unbadged (i.e. only 1 profile). It is unfortunate that the global shortcut is still visible when you have 2 profiles, but I think you still want to show the both profile's badged/named shortcut in that case as it feels otherwise confusing... I guess the global shortcut will keep the current behavior of opening the last profile.
https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:55: fake_user_desktop_.reset( On 2013/01/14 21:45:19, sail wrote: > Are you moving this here to ensure that it runs once for each test? If so then I > don't think that's necessary. The class will be constructed each time the test > is run. Ah, that makes sense. Thanks for letting me know, I'll change it back. https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:266: FilePath system_shortcuts_directory_; On 2013/01/14 21:45:19, sail wrote: > Ideally we shouldn't cache things in member variables. If you're doing this to > make the code easier to use then use utility methods instead. > > I think the following member variables should be removed: > distribution_ > profile_info_cache_ > exe_path_ > shortcuts_directory_ > system_shortcuts_directory_ > > This also make it easier to understand how the class works. Things are cached in member variables to make the test cases' code more brief and easier to read. Utility methods will not result in code that is as short. If you still prefer I change it, let me do it in a separate CL, so we don't introduce noise too this one, OK? https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:684: // Delete the shortcut for the first profile, but keep the one for the 2nd. On 2013/01/14 21:49:30, gab wrote: > Why is it necessary to delete shortcut 1 first? > > Isn't this testing the case where you have two profile shortcuts, delete a > profile, and thus the last profile shortcut becomes a regular shortcut, but it > should simply get deleted if there is a system-level already present because > another regular shortcut is redundant? These two tests are based on the existing tests DeleteSecondToLastProfileWithoutShortcut and DeleteSecondToLastProfileWithShortcut. In the new tests I'm adding, the profile deletion happens in the presence of a system level shortcut, so the expected result is different. If there is no system level shortcut, then a user-level one should be created. If there is, it shouldn't be (since otherwise you'd be getting two non-profile shortcuts). Why both cases? It's necessary to exercise two separate codepaths in the implementation. In the case when you're deleting the 2nd-to-last profile without a shortcut, then the creation of the non-profile shortcut happens as a result of RenameChromeDesktopShortcutForProfile() for the other profile. In the case when you're deleting the 2nd-to-last profile that does have a shortcut, the creation of the non-profile shortcut happens via DeleteDesktopShortcutsAndIconFile(). The two test cases are necessary to ensure both code paths are covered. Now, strictly speaking, the DeleteSecondToLastProfileWithoutShortcutWhenSystemLevelShortcutExists test does not need to delete the shortcut itself to achieve coverage for the code path its testing, but I kept the new tests similar to the ones they're based on. I can simplify if you prefer. If you wish, I can simplify them as much as possible, by basically removing all the ASSERTs/EXPECTs for the other conditions (e.g. profile shortcuts existing or not existing), and just leave the check for the user level regular shortcut not being created.
https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:266: FilePath system_shortcuts_directory_; On 2013/01/14 22:40:24, Alexei Svitkine wrote: > On 2013/01/14 21:45:19, sail wrote: > > Ideally we shouldn't cache things in member variables. If you're doing this to > > make the code easier to use then use utility methods instead. > > > > I think the following member variables should be removed: > > distribution_ > > profile_info_cache_ > > exe_path_ > > shortcuts_directory_ > > system_shortcuts_directory_ > > > > This also make it easier to understand how the class works. > > Things are cached in member variables to make the test cases' code more brief > and easier to read. Utility methods will not result in code that is as short. It's not too bad, GetFakeUserDesktop(). That's only 2 more letters than the old one. > If you still prefer I change it, let me do it in a separate CL, so we don't > introduce noise too this one, OK? Sounds good.
+other comments pending on profile_shortcut_manager.cc https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:684: // Delete the shortcut for the first profile, but keep the one for the 2nd. On 2013/01/14 22:40:24, Alexei Svitkine wrote: > On 2013/01/14 21:49:30, gab wrote: > > Why is it necessary to delete shortcut 1 first? > > > > Isn't this testing the case where you have two profile shortcuts, delete a > > profile, and thus the last profile shortcut becomes a regular shortcut, but it > > should simply get deleted if there is a system-level already present because > > another regular shortcut is redundant? > > These two tests are based on the existing tests > DeleteSecondToLastProfileWithoutShortcut and > DeleteSecondToLastProfileWithShortcut. > > In the new tests I'm adding, the profile deletion happens in the presence of a > system level shortcut, so the expected result is different. If there is no > system level shortcut, then a user-level one should be created. If there is, it > shouldn't be (since otherwise you'd be getting two non-profile shortcuts). > > Why both cases? It's necessary to exercise two separate codepaths in the > implementation. > > In the case when you're deleting the 2nd-to-last profile without a shortcut, > then the creation of the non-profile shortcut happens as a result of > RenameChromeDesktopShortcutForProfile() for the other profile. > > In the case when you're deleting the 2nd-to-last profile that does have a > shortcut, the creation of the non-profile shortcut happens via > DeleteDesktopShortcutsAndIconFile(). > > The two test cases are necessary to ensure both code paths are covered. Right that sgtm. > > Now, strictly speaking, the > DeleteSecondToLastProfileWithoutShortcutWhenSystemLevelShortcutExists test does > not need to delete the shortcut itself to achieve coverage for the code path its > testing, but I kept the new tests similar to the ones they're based on. I can > simplify if you prefer. Right, I would prefer not manually deleting the shortcut, the deletion of the profile should do that anyways; perhaps move file_util::PathExists(profile_1_shortcut_path)); up right after RunPendingTasks(); to make it clear that this one goes away at that point. Deleting profile 1 when its shortcut was manually deleted and expecting the same result is also an interesting test case from a black box p.o.v. (i.e., perhaps its the same codepath for now, but its definitely not the same scenario). > > If you wish, I can simplify them as much as possible, by basically removing all > the ASSERTs/EXPECTs for the other conditions (e.g. profile shortcuts existing or > not existing), and just leave the check for the user level regular shortcut not > being created. I like that there be EXPECTs around as many methods as possible, I'm just not a fan of doing the extra unnecessary deletion when this is not the core of this test case.
Should also add a test that you are able to create profile shortcuts when a system-level shortcut is already present (as per my comment in profile_shortcut_manager.cc I think this is currently broken with this CL).
(I'll update the actual CL tomorrow.) https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:279: operation = ShellUtil::SHELL_SHORTCUT_CREATE_IF_NO_SYSTEM_LEVEL; On 2013/01/14 21:49:30, gab wrote: > This will prevent creation of all profile shortcuts when a system-level shortcut > is present, I think you want to only prevent creation of the shortcut when it is > unbadged (i.e. only 1 profile). > > It is unfortunate that the global shortcut is still visible when you have 2 > profiles, but I think you still want to show the both profile's badged/named > shortcut in that case as it feels otherwise confusing... I guess the global > shortcut will keep the current behavior of opening the last profile. Makes sense. Just to clarify, SHELL_SHORTCUT_CREATE_IF_NO_SYSTEM_LEVEL will prevent the creation of a user-level shortcut "Foo.lnk" when there's a system level shortcut "Google Chrome.lnk"? When I wrote this, I did it under the impression that it would check for a system-level shortcut with the same name, in which case this code should be correct, I think. Please clarify.
https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:279: operation = ShellUtil::SHELL_SHORTCUT_CREATE_IF_NO_SYSTEM_LEVEL; On 2013/01/14 22:57:14, Alexei Svitkine wrote: > On 2013/01/14 21:49:30, gab wrote: > > This will prevent creation of all profile shortcuts when a system-level > shortcut > > is present, I think you want to only prevent creation of the shortcut when it > is > > unbadged (i.e. only 1 profile). > > > > It is unfortunate that the global shortcut is still visible when you have 2 > > profiles, but I think you still want to show the both profile's badged/named > > shortcut in that case as it feels otherwise confusing... I guess the global > > shortcut will keep the current behavior of opening the last profile. > > Makes sense. Just to clarify, SHELL_SHORTCUT_CREATE_IF_NO_SYSTEM_LEVEL will > prevent the creation of a user-level shortcut "Foo.lnk" when there's a system > level shortcut "Google Chrome.lnk"? When I wrote this, I did it under the > impression that it would check for a system-level shortcut with the same name, > in which case this code should be correct, I think. Please clarify. Ah, you're right, I wrongly assumed it would prevent all creations of user-level, but just looked at the details of the implementation and you're right; this was not really intended when I wrote it, but I guess it has clever/useful side-effects now :). Could you add to the documentation for SHELL_SHORTCUT_CREATE_IF_NO_SYSTEM_LEVEL to be clear that this is the intended behavior for it to stick? Cheers! Gab
Please have another look. In this updated CL, I've added test coverage for actually creating profile shortcuts (not just regular ones) in the presence of a system level shortcut. This actually found an issue where the name change operation didn't do the right thing when it was trying to change a nonprofile shortcut to a profile shortcut when the nonprofile shortcut was systemlevel, which I've now fixed. I've also fixed a TODO of mine to fold the RenameChromeDesktopShortcutForProfile() call into CreateOrUpdateDesktopShortcutsForProfile(), removing the need for StartProfileShortcutNameChange() entirely. I've simplified the DeleteSecondToLastProfileWithSystemLevelShortcut test to not delete any shortcuts, which made it much cleaner. However, I kept DeleteSecondToLastProfileWithShortcutWhenSystemLevelShortcutExists() the same and added a comment explaining why it needs to delete a shortcut to achieve test coverage for the code it's testing. https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:55: fake_user_desktop_.reset( On 2013/01/14 21:45:19, sail wrote: > Are you moving this here to ensure that it runs once for each test? If so then I > don't think that's necessary. The class will be constructed each time the test > is run. Done. https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:61: fake_system_desktop_.reset( On 2013/01/14 21:45:19, sail wrote: > same here Done. https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:189: const string16& new_shortcut_file) { On 2013/01/14 21:49:30, gab wrote: > optional nit: Rename these parameters to old_shortcut_filename and > new_shortcut_filename... from the code they look like filenames, not the file > itself. Done. https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:205: DCHECK(ShellUtil::GetShortcutPath(ShellUtil::SHORTCUT_LOCATION_DESKTOP, On 2013/01/14 21:49:30, gab wrote: > Take the call out of the DCHECK, store the result and DCHECK on that. > > Or instead do: > if (!ShellUtil::GetShortcutPath...) > NOTREACHED(); > > Otherwise this code will not run in Release builds.. > > Also if this fails you probably don't want to keep going with the verification. Doh, thanks for catching this. Fixed. https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:209: const FilePath possible_system_shortcut = On 2013/01/14 21:45:19, sail wrote: > new_system_shortcut_path I've changed it to |possible_new_system_shortcut|, to still have the name indicate that it may or may not exist. https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:216: FilePath new_shortcut_path = shortcuts_directory.Append(new_shortcut_file); On 2013/01/14 21:45:19, sail wrote: > new_user_shortcut_path All the paths here are assumed to be user paths except for |possible_new_system_shortcut|, which is named to indicate that. I suppose I could change things to be more explicit (e.g. rename old_shortcut_path to old_user_shortcut_path), but it seems unnecessary to me. WDYT? https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:279: operation = ShellUtil::SHELL_SHORTCUT_CREATE_IF_NO_SYSTEM_LEVEL; On 2013/01/14 23:04:42, gab wrote: > Could you add to the documentation for > SHELL_SHORTCUT_CREATE_IF_NO_SYSTEM_LEVEL to be clear that this is the intended > behavior for it to stick? Done.
LGTM! https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:189: const string16& new_shortcut_file) { On 2013/01/15 22:03:02, Alexei Svitkine wrote: > On 2013/01/14 21:49:30, gab wrote: > > optional nit: Rename these parameters to old_shortcut_filename and > > new_shortcut_filename... from the code they look like filenames, not the file > > itself. > > Done. Didn't see this change. I'm guessing you're doing this in a separate CL.
https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:189: const string16& new_shortcut_file) { On 2013/01/15 22:12:15, sail wrote: > On 2013/01/15 22:03:02, Alexei Svitkine wrote: > > On 2013/01/14 21:49:30, gab wrote: > > > optional nit: Rename these parameters to old_shortcut_filename and > > > new_shortcut_filename... from the code they look like filenames, not the > file > > > itself. > > > > Done. > > Didn't see this change. I'm guessing you're doing this in a separate CL. It should be there, take a closer look at: https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p...
https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11876027/diff/6001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:189: const string16& new_shortcut_file) { On 2013/01/15 22:18:34, Alexei Svitkine wrote: > On 2013/01/15 22:12:15, sail wrote: > > On 2013/01/15 22:03:02, Alexei Svitkine wrote: > > > On 2013/01/14 21:49:30, gab wrote: > > > > optional nit: Rename these parameters to old_shortcut_filename and > > > > new_shortcut_filename... from the code they look like filenames, not the > > file > > > > itself. > > > > > > Done. > > > > Didn't see this change. I'm guessing you're doing this in a separate CL. > > It should be there, take a closer look at: > https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... Oops, I was thinking of the test member variable renaming. LGTM again!
https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:415: TEST_F(ProfileShortcutManagerTest, DeleteOnlyProfileWithShortcuts) { Is this really the desired outcome (i.e. that if the only profile with a shortcut is deleted we end up with no shortcut?) If so then: 1) This is weird, perhaps... 2) The last test tests the same thing, but in a scenario where there is also a system-level shortcut present (but I don't see why that would matter since we never recreate shortcuts apparently...); hence the last test wouldn't add more test coverage. https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:666: // Ensure system level continues to exist and user level was not created. nit: We tend to write "system-level" with a dash around the installer codebase, please keep the same syntax here (and at a few other places in this CL with a quick search). https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:192: FilePath shortcuts_directory; Rename this to user_shortcuts_directory since you now have system_shortcuts_directory. https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:193: if (!GetDesktopShortcutsDirectory(&shortcuts_directory)) I don't really see the value of this method, an if(!..){NOTREACHED;return;} block like below seems just fine to me... Furthermore this call recomputes the BrowserDistribution when it could easily be re-used here. https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:230: return; I'm not a fan of the multiple early returns I feel an if{}else if{}else if{} block would be more readable here (as it is now it feels like continuous code at first glance, but it's really not as any of the conditions met along the way cause an early return). What do you think of replacing 209-234 with something like: if (file_util::PathExists(old_shortcut_path)) { // Rename the old shortcut unless a system level shortcut exists at the // destination, in which case the old shortcut is simply deleted. const FilePath possible_new_system_shortcut = system_shortcuts_directory.Append(new_shortcut_filename); if (file_util::PathExists(possible_new_system_shortcut)) file_util::Delete(old_shortcut_path, false); else if (!file_util::Move(old_shortcut_path, new_shortcut_path)) LOG(ERROR) << "Could not rename Windows profile desktop shortcut."; } else { // If the shortcut does not exist, it may have been renamed by the user. In // that case, its name should not be changed. // It's also possible that a system level shortcut exists instead, for // example the original Chrome shortcut from an installation. If that's the // case, copy that one over - it will get its properties updated by the code // in |CreateOrUpdateDesktopShortcutsForProfile()|. const FilePath possible_old_system_shortcut = system_shortcuts_directory.Append(old_shortcut_filename); if (file_util::PathExists(possible_old_system_shortcut)) file_util::CopyFile(possible_old_system_shortcut, new_shortcut_path); }
https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:415: TEST_F(ProfileShortcutManagerTest, DeleteOnlyProfileWithShortcuts) { On 2013/01/15 23:09:14, gab wrote: > Is this really the desired outcome (i.e. that if the only profile with a > shortcut is deleted we end up with no shortcut?) If so then: If there's *three* (or more) profiles and only one of them has a shortcut and you delete the one that does, its shortcut should be deleted and no other shortcuts should be re-created. This is what the test is testing. This is different when you have *two* profiles to start with, because then you're deleting back to a single-profile mode. This is what other tests are testing. > 2) The last test tests the same thing, No it doesn't, the last test involves there being two shortcuts initially, deleting down to single-profile mode, which should result in a nonprofile shortcut existing if either profile had a shortcut. See DeleteSecondToLastProfileWithoutShortcut and DeleteSecondToLastProfileWithShortcut tests. https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:193: if (!GetDesktopShortcutsDirectory(&shortcuts_directory)) On 2013/01/15 23:09:14, gab wrote: > I don't really see the value of this method, an if(!..){NOTREACHED;return;} > block like below seems just fine to me... Furthermore this call recomputes the > BrowserDistribution when it could easily be re-used here. The method is called from other functions too, to make the code more readable. They would all have to be changed to the more verbose versions. Maybe we can change it to: GetDesktopShortcutsDirectories(FilePath* user_shortcuts_directory, FilePath* system_shortcuts_directory); And have the params be optional (if passed in NULL, then it won't fill them in). This way, it could re-use the distribution for both calls and still be very concise from the call sites. WDYT?
gab: PTAL https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:666: // Ensure system level continues to exist and user level was not created. On 2013/01/15 23:09:14, gab wrote: > nit: We tend to write "system-level" with a dash around the installer codebase, > please keep the same syntax here (and at a few other places in this CL with a > quick search). Done. https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:192: FilePath shortcuts_directory; On 2013/01/15 23:09:14, gab wrote: > Rename this to user_shortcuts_directory since you now have > system_shortcuts_directory. Done. https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:193: if (!GetDesktopShortcutsDirectory(&shortcuts_directory)) On 2013/01/15 23:34:43, Alexei Svitkine wrote: > On 2013/01/15 23:09:14, gab wrote: > > I don't really see the value of this method, an if(!..){NOTREACHED;return;} > > block like below seems just fine to me... Furthermore this call recomputes the > > BrowserDistribution when it could easily be re-used here. > > The method is called from other functions too, to make the code more readable. > They would all have to be changed to the more verbose versions. > > Maybe we can change it to: > > GetDesktopShortcutsDirectories(FilePath* user_shortcuts_directory, FilePath* > system_shortcuts_directory); > > And have the params be optional (if passed in NULL, then it won't fill them in). > This way, it could re-use the distribution for both calls and still be very > concise from the call sites. WDYT? I've now done this in the latest patchset. https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:230: return; On 2013/01/15 23:09:14, gab wrote: > I'm not a fan of the multiple early returns I feel an if{}else if{}else if{} > block would be more readable here (as it is now it feels like continuous code at > first glance, but it's really not as any of the conditions met along the way > cause an early return). > > What do you think of replacing 209-234 with something like: > > if (file_util::PathExists(old_shortcut_path)) { > // Rename the old shortcut unless a system level shortcut exists at the > // destination, in which case the old shortcut is simply deleted. > const FilePath possible_new_system_shortcut = > system_shortcuts_directory.Append(new_shortcut_filename); > if (file_util::PathExists(possible_new_system_shortcut)) > file_util::Delete(old_shortcut_path, false); > else if (!file_util::Move(old_shortcut_path, new_shortcut_path)) > LOG(ERROR) << "Could not rename Windows profile desktop shortcut."; > } else { > // If the shortcut does not exist, it may have been renamed by the user. In > // that case, its name should not be changed. > // It's also possible that a system level shortcut exists instead, for > // example the original Chrome shortcut from an installation. If that's the > // case, copy that one over - it will get its properties updated by the code > // in |CreateOrUpdateDesktopShortcutsForProfile()|. > const FilePath possible_old_system_shortcut = > system_shortcuts_directory.Append(old_shortcut_filename); > if (file_util::PathExists(possible_old_system_shortcut)) > file_util::CopyFile(possible_old_system_shortcut, new_shortcut_path); > } > I like your suggested flow better, done!
Looks great, couple nits and one subtle issue below. Cheers! Gab https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:415: TEST_F(ProfileShortcutManagerTest, DeleteOnlyProfileWithShortcuts) { On 2013/01/15 23:34:43, Alexei Svitkine wrote: > On 2013/01/15 23:09:14, gab wrote: > > Is this really the desired outcome (i.e. that if the only profile with a > > shortcut is deleted we end up with no shortcut?) If so then: > > If there's *three* (or more) profiles and only one of them has a shortcut and > you delete the one that does, its shortcut should be deleted and no other > shortcuts should be re-created. This is what the test is testing. > > This is different when you have *two* profiles to start with, because then > you're deleting back to a single-profile mode. This is what other tests are > testing. > > > 2) The last test tests the same thing, > > No it doesn't, the last test involves there being two shortcuts initially, > deleting down to single-profile mode, which should result in a nonprofile > shortcut existing if either profile had a shortcut. > > See DeleteSecondToLastProfileWithoutShortcut and > DeleteSecondToLastProfileWithShortcut tests. Ah, awesome :)! https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11876027/diff/18001/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:193: if (!GetDesktopShortcutsDirectory(&shortcuts_directory)) On 2013/01/15 23:34:43, Alexei Svitkine wrote: > On 2013/01/15 23:09:14, gab wrote: > > I don't really see the value of this method, an if(!..){NOTREACHED;return;} > > block like below seems just fine to me... Furthermore this call recomputes the > > BrowserDistribution when it could easily be re-used here. > > The method is called from other functions too, to make the code more readable. > They would all have to be changed to the more verbose versions. > > Maybe we can change it to: > > GetDesktopShortcutsDirectories(FilePath* user_shortcuts_directory, FilePath* > system_shortcuts_directory); > > And have the params be optional (if passed in NULL, then it won't fill them in). > This way, it could re-use the distribution for both calls and still be very > concise from the call sites. WDYT? sgtm. https://codereview.chromium.org/11876027/diff/30002/chrome/browser/profiles/p... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11876027/diff/30002/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:116: // Gets the user and system directories for desktop shortcuts. nit: Add comment about return value? https://codereview.chromium.org/11876027/diff/30002/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:175: if (!GetDesktopShortcutsDirectories(&shortcuts_directory, NULL)) nit: perhaps rename all instances of |shortcuts_directory| to |user_shortcuts_directory| even when |system_shortcuts_directory| is not used. I think this makes the callsite more readable (i.e. otherwise the reader can't assume anything and has to go lookup GetDesktopShortcutsDirectories's definition). https://codereview.chromium.org/11876027/diff/30002/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:223: LOG(ERROR) << "Could not rename Windows profile desktop shortcut."; nit: DLOG(ERROR) --> We will never actually get release logs from users and this otherwise only bloats the binary with an unnecessary string imo. https://codereview.chromium.org/11876027/diff/30002/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:234: file_util::CopyFile(possible_old_system_shortcut, new_shortcut_path); Actually I just realized: if the shortcut has been renamed and there is a system-level shortcut this will duplicate this profile's shortcut (i.e. one with the custom name and one with the default name)... Do we have a better way to find manually renamed shortcuts for this profile?
https://codereview.chromium.org/11876027/diff/30002/chrome/browser/profiles/p... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11876027/diff/30002/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:234: file_util::CopyFile(possible_old_system_shortcut, new_shortcut_path); On 2013/01/16 17:44:00, gab wrote: > Actually I just realized: if the shortcut has been renamed and there is a > system-level shortcut this will duplicate this profile's shortcut (i.e. one with > the custom name and one with the default name)... > > Do we have a better way to find manually renamed shortcuts for this profile? Should probably add a test for this too... shortcuts are fun :)!
https://codereview.chromium.org/11876027/diff/30002/chrome/browser/profiles/p... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11876027/diff/30002/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:116: // Gets the user and system directories for desktop shortcuts. On 2013/01/16 17:44:00, gab wrote: > nit: Add comment about return value? Done. https://codereview.chromium.org/11876027/diff/30002/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:175: if (!GetDesktopShortcutsDirectories(&shortcuts_directory, NULL)) On 2013/01/16 17:44:00, gab wrote: > nit: perhaps rename all instances of |shortcuts_directory| to > |user_shortcuts_directory| even when |system_shortcuts_directory| is not used. I > think this makes the callsite more readable (i.e. otherwise the reader can't > assume anything and has to go lookup GetDesktopShortcutsDirectories's > definition). Done. https://codereview.chromium.org/11876027/diff/30002/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:223: LOG(ERROR) << "Could not rename Windows profile desktop shortcut."; On 2013/01/16 17:44:00, gab wrote: > nit: DLOG(ERROR) --> We will never actually get release logs from users and this > otherwise only bloats the binary with an unnecessary string imo. Done. https://codereview.chromium.org/11876027/diff/30002/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:234: file_util::CopyFile(possible_old_system_shortcut, new_shortcut_path); On 2013/01/16 17:44:00, gab wrote: > Actually I just realized: if the shortcut has been renamed and there is a > system-level shortcut this will duplicate this profile's shortcut (i.e. one with > the custom name and one with the default name)... > > Do we have a better way to find manually renamed shortcuts for this profile? There shouldn't ever be a system-level *profile* shortcut (unless a user does something peculiar like manually copying a user level profile shortcut to the system-level desktop directory). So I don't think this is an issue in practice. This code deals primarily with the case where the "old shortcut" is a non-profile shortcut.
LGTM, thanks! https://codereview.chromium.org/11876027/diff/30002/chrome/browser/profiles/p... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11876027/diff/30002/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:234: file_util::CopyFile(possible_old_system_shortcut, new_shortcut_path); On 2013/01/16 17:59:56, Alexei Svitkine wrote: > On 2013/01/16 17:44:00, gab wrote: > > Actually I just realized: if the shortcut has been renamed and there is a > > system-level shortcut this will duplicate this profile's shortcut (i.e. one > with > > the custom name and one with the default name)... > > > > Do we have a better way to find manually renamed shortcuts for this profile? > > There shouldn't ever be a system-level *profile* shortcut (unless a user does > something peculiar like manually copying a user level profile shortcut to the > system-level desktop directory). So I don't think this is an issue in practice. > > This code deals primarily with the case where the "old shortcut" is a > non-profile shortcut. Ah right :) -- perhaps the comment could be clarified to highlight this.
https://codereview.chromium.org/11876027/diff/30002/chrome/browser/profiles/p... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11876027/diff/30002/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:234: file_util::CopyFile(possible_old_system_shortcut, new_shortcut_path); On 2013/01/16 18:17:32, gab wrote: > Ah right :) -- perhaps the comment could be clarified to highlight this. Comment updated.
https://codereview.chromium.org/11876027/diff/30002/chrome/browser/profiles/p... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11876027/diff/30002/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:234: file_util::CopyFile(possible_old_system_shortcut, new_shortcut_path); On 2013/01/16 18:40:42, Alexei Svitkine wrote: > On 2013/01/16 18:17:32, gab wrote: > > Ah right :) -- perhaps the comment could be clarified to highlight this. > > Comment updated. Thanks, still lgtm.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asvitkine@chromium.org/11876027/32002
Retried try job too often on win7_aura for step(s) content_browsertests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asvitkine@chromium.org/11876027/32002
Message was sent while issue was closed.
Change committed as 177256 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
