Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11299160:
Find shortcuts that don't have a profile command line set when creating 2nd profile. (Closed)
|
Created:
8 years, 1 month ago by Alexei Svitkine (slow) Modified:
8 years ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionFind shortcuts that don't have a profile command line set when creating 2nd profile.
This addresses the scenario when a new Chrome install has a default shortcut
with no profile command-line set and the user creates a 2nd profile. With this
change, the existing shortcut will be correctly replaced with the two shortcuts,
one for each profile.
BUG=108203
TEST=New unit tests.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170200
Patch Set 1 : #
Total comments: 16
Patch Set 2 : #
Total comments: 18
Patch Set 3 : #
Total comments: 13
Patch Set 4 : #
Messages
Total messages: 16 (0 generated)
https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:112: profile_shortcut_manager_->CreateProfileShortcut(second_dest_path_); I know this was there previously but is it really necessary? Shouldn't the shortcut manager get a notification that a profile was added and create a shortcut automatically? https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:371: const FilePath default_shortcut_path = naming here is inconsistent. above you refer to the shortcut for the first profile as the default shortcut here you're referring to the shortcut that points to Chrome and not any profile. maybe rename this to regular_shortcut_path (same below) https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:373: EXPECT_TRUE(file_util::PathExists(default_shortcut_path)); move this into CreateRegularShortcutWithName? (same below) https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:380: GetDefaultShortcutPathForProfile(profile_name_); This test is really confusing. Here's what I think you're doing: #1 Call SetupDefaultProfileShortcut which #1a. creates a first profile (named profile_name_ and located at dest_path_) #1b. creates a shortcut that points to the first profile #2. Delete the shortcut that points to the first profile #3. Create a shortcut that just launches Chrome and doesn't point to any profile #4 Call CreateSecondProfileWithShortcut which #4a. creates a second profile (named second_profile_name_ and located at second_dest_path_) #4b. creates a shortcut that points to the second profile #5 Check that the shortcut for the first profile exists (it was previously deleted in step #2) #6 Check that the shortcut for profile 1 has the correct flags I think there are several things you could do to make this easier to follow: - rename variables like dest_path_ and second_dest_path_ to something like profile_1_path_ and profile_2_path_. Same with profile_name_, maybe rename to profile_1_name_. - come up with a better way to distinguish between shortcuts that point to a profile and shortcuts that don't point to a profile. Currently you use "default" and "regular" which is confusing. - Move the shortcut verification into a separate helper function. Maybe VerifyProfile1Shortcut() and VerifyShortcutForProfile2(). This can both check that the shortcut exists and that the command line arguments are correct. With that done you could write this test like this: ASSERT(profile count == 0) profile_1_shortcut_path = CreateProfile1AndShortcut(); // (also verifies the shortcut) Delete(profile_1_shortcut_path) chrome_only_shortcut_path = CreateChromeOnlyShortcut() CreateProfile2AndShortcut(); // (also verifies the shortcut) EXPECT_FALSE(PathExists(chrome_only_shortcut_path)) VerifyProfile2Shortcut() https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:314: UpdateShortcutsForProfileAtPath(profile_path, true, true); these UpdateShortcutsForProfileAtPath() calls are really cryptic. Looking at them, there's a bunch of (true, false), (false, true), etc.. arguments. It's really hard to tell what the calls are doing. Maybe replace with enums for both arguments? https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:333: false); indentation looks wrong here https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:392: bool include_empty_command_lines, this name is too implementation specific. Maybe pick a name that describes what you're trying to do (update default profiles, etc..).
Won't have a chance to address some of the other comments till later, but here's a couple of initial replies. https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:112: profile_shortcut_manager_->CreateProfileShortcut(second_dest_path_); On 2012/11/26 21:27:16, sail wrote: > I know this was there previously but is it really necessary? Shouldn't the > shortcut manager get a notification that a profile was added and create a > shortcut automatically? No. Whether the shortcut is created is based on the value of the checkbox in the UI normally - so the notification doesn't carry enough information on whether the shortcut should be created. It's the web UI handler code that calls into this to create the shortcut after creating the profile. That's equivalent to what's being done here. https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:373: EXPECT_TRUE(file_util::PathExists(default_shortcut_path)); On 2012/11/26 21:27:16, sail wrote: > move this into CreateRegularShortcutWithName? (same below) The reason I didn't is because when an EXPECT fails in a helper function, it doesn't give the stack trace of where it was called. (Thus, if there was a failure, I wouldn't know if it was the first CreateRegularShortcutWithName() or the second one that failed in this test case.) This problem already plagues these tests, since there's a lot of checking done in helper functions. Unless there's a good way to preserve the callsite, I'd rather not tuck things away even more. (If there is a good way to preserve the callsite, then by all means I agree.) I guess one possibility would be to pass __FILE__ and __LINE__ to all the helpers, which would log them on failure. Seems messy, though. What do you think?
https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:373: EXPECT_TRUE(file_util::PathExists(default_shortcut_path)); On 2012/11/27 14:49:26, Alexei Svitkine wrote: > On 2012/11/26 21:27:16, sail wrote: > > move this into CreateRegularShortcutWithName? (same below) > > The reason I didn't is because when an EXPECT fails in a helper function, it > doesn't give the stack trace of where it was called. (Thus, if there was a > failure, I wouldn't know if it was the first CreateRegularShortcutWithName() or > the second one that failed in this test case.) > > This problem already plagues these tests, since there's a lot of checking done > in helper functions. > > Unless there's a good way to preserve the callsite, I'd rather not tuck things > away even more. (If there is a good way to preserve the callsite, then by all > means I agree.) > > I guess one possibility would be to pass __FILE__ and __LINE__ to all the > helpers, which would log them on failure. Seems messy, though. What do you > think? Replying to myself: Looks like we have base/location.h which could work. We can use the FROM_HERE macro from that file to get a tracked_objects::Location object that could be passed to all the helper functions as a parameter. Then, their EXPECT lines could do: "EXPEXT_TRUE(foo) << location.ToString();". It sounds like that may do the trick. WDYT?
I'm not too worried about being able to find the exact problem from test failures. It's much more important to me to have easy to read code. If you can do both then that's even better. On 2012/11/27 14:53:58, Alexei Svitkine wrote: > https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... > File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): > > https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... > chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:373: > EXPECT_TRUE(file_util::PathExists(default_shortcut_path)); > On 2012/11/27 14:49:26, Alexei Svitkine wrote: > > On 2012/11/26 21:27:16, sail wrote: > > > move this into CreateRegularShortcutWithName? (same below) > > > > The reason I didn't is because when an EXPECT fails in a helper function, it > > doesn't give the stack trace of where it was called. (Thus, if there was a > > failure, I wouldn't know if it was the first CreateRegularShortcutWithName() > or > > the second one that failed in this test case.) > > > > This problem already plagues these tests, since there's a lot of checking done > > in helper functions. > > > > Unless there's a good way to preserve the callsite, I'd rather not tuck things > > away even more. (If there is a good way to preserve the callsite, then by all > > means I agree.) > > > > I guess one possibility would be to pass __FILE__ and __LINE__ to all the > > helpers, which would log them on failure. Seems messy, though. What do you > > think? > > Replying to myself: > > Looks like we have base/location.h which could work. We can use the FROM_HERE > macro from that file to get a tracked_objects::Location object that could be > passed to all the helper functions as a parameter. Then, their EXPECT lines > could do: "EXPEXT_TRUE(foo) << location.ToString();". > > It sounds like that may do the trick. WDYT?
Thanks for your comments. I've made most of the changes you suggested, though there's a bit more to do (most of my day was spent in gLearn today and will be again tomorrow). I've also incorporated the base/location stuff so that when things fail in helper functions, the output is actually useful. (Which imho is important since if someone's going to be looking at these tests again, it's most likely because there was a failure.) Even though I haven't addressed all the comments yet, I'd like you to have another look at the changes so far and let me know what you think. Thanks! https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:371: const FilePath default_shortcut_path = On 2012/11/26 21:27:16, sail wrote: > naming here is inconsistent. > above you refer to the shortcut for the first profile as the default shortcut > here you're referring to the shortcut that points to Chrome and not any profile. > > maybe rename this to regular_shortcut_path (same below) Done. https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:373: EXPECT_TRUE(file_util::PathExists(default_shortcut_path)); On 2012/11/26 21:27:16, sail wrote: > move this into CreateRegularShortcutWithName? (same below) Done. https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:380: GetDefaultShortcutPathForProfile(profile_name_); On 2012/11/26 21:27:16, sail wrote: > This test is really confusing. > > Here's what I think you're doing: > #1 Call SetupDefaultProfileShortcut which > #1a. creates a first profile (named profile_name_ and located at > dest_path_) > #1b. creates a shortcut that points to the first profile > #2. Delete the shortcut that points to the first profile > #3. Create a shortcut that just launches Chrome and doesn't point to any > profile > #4 Call CreateSecondProfileWithShortcut which > #4a. creates a second profile (named second_profile_name_ and located at > second_dest_path_) > #4b. creates a shortcut that points to the second profile > #5 Check that the shortcut for the first profile exists (it was previously > deleted in step #2) > #6 Check that the shortcut for profile 1 has the correct flags That's correct. > I think there are several things you could do to make this easier to follow: > - rename variables like dest_path_ and second_dest_path_ to something like > profile_1_path_ and profile_2_path_. Same with profile_name_, maybe rename to > profile_1_name_. Done. This was bothering me too. > - come up with a better way to distinguish between shortcuts that point to a > profile and shortcuts that don't point to a profile. Currently you use "default" > and "regular" which is confusing. I'm changed the test to be consistent with the use of the name "regular", instead of default, which was an overloaded term in the test. > - Move the shortcut verification into a separate helper function. Maybe > VerifyProfile1Shortcut() and VerifyShortcutForProfile2(). This can both check > that the shortcut exists and that the command line arguments are correct. I still need to do this. Right now, we have ValidateProfileShortcut(), which does a bunch of validation already, but I've not yet incorporated the command line checking into it. I'll do this tomorrow. > With that done you could write this test like this: > ASSERT(profile count == 0) > profile_1_shortcut_path = CreateProfile1AndShortcut(); // (also verifies the > shortcut) > Delete(profile_1_shortcut_path) > chrome_only_shortcut_path = CreateChromeOnlyShortcut() > CreateProfile2AndShortcut(); // (also verifies the shortcut) > EXPECT_FALSE(PathExists(chrome_only_shortcut_path)) > VerifyProfile2Shortcut() https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:314: UpdateShortcutsForProfileAtPath(profile_path, true, true); On 2012/11/26 21:27:16, sail wrote: > these UpdateShortcutsForProfileAtPath() calls are really cryptic. Looking at > them, there's a bunch of (true, false), (false, true), etc.. arguments. It's > really hard to tell what the calls are doing. Maybe replace with enums for both > arguments? Done. Let me know if you have better suggestions for the enum names and values. I've also kept the parameters to the anonymous namespace function as-is, since it can't use the private ProfileShortcutManagerWin enums. If you prefer, I can make the enums public and plumb them through to that function. https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:333: false); On 2012/11/26 21:27:16, sail wrote: > indentation looks wrong here Done. https://codereview.chromium.org/11299160/diff/11004/chrome/browser/profiles/p... chrome/browser/profiles/profile_shortcut_manager_win.cc:392: bool include_empty_command_lines, On 2012/11/26 21:27:16, sail wrote: > this name is too implementation specific. Maybe pick a name that describes what > you're trying to do (update default profiles, etc..). Hopefully, this should be clearer with the enum parameters now.
Looks really good! https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:99: ASSERT_FALSE(ProfileShortcutExistsAtDefaultPath(profile_1_name_)); did you want to add a "<< location.ToString()" here? https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:118: CreateProfileWithShortcut(FROM_HERE, profile_2_name_, profile_2_path_); why pass FROM_HERE instead of location? https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:221: FILE_PATH_LITERAL("Google Profile.ico")))); should we just get rid of all these FILE_PATH_LITERALs? https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:372: const FilePath new_shortcut_path = maybe new_profile_1_shortcut_path https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:377: string16 shortcut_args; maybe profile_1_shortcut_args? https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:395: const FilePath second_shortcut_path = I find spelling out second harder to parse. How about regular_shortcut_path_1 and regular_shortcut_path_2 Oh wait. So the first one is a standard chrome shortcut and the second one is a custom made chrome shortcut? If so maybe name it as regular_shortcut_path, customized_regular_shortcut_path. https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:397: EXPECT_TRUE(file_util::PathExists(second_shortcut_path)); don't need this https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:405: const FilePath new_shortcut_path = same as above, maybe "new_profile_1_shortcut_path" https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:131: bool include_empty_command_lines, Yea, I think plumbing the enums here would be great. If that's really awkward then maybe name this to better match the enum? Something like should_update_non_profile_shortcuts
PTAL. I've addressed your comments and made the shortcut validation function check command-line args, which resulted in the new tests being quite a bit shorter. https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:99: ASSERT_FALSE(ProfileShortcutExistsAtDefaultPath(profile_1_name_)); On 2012/11/28 05:12:31, sail wrote: > did you want to add a "<< location.ToString()" here? Done. https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:118: CreateProfileWithShortcut(FROM_HERE, profile_2_name_, profile_2_path_); On 2012/11/28 05:12:31, sail wrote: > why pass FROM_HERE instead of location? Done. https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:221: FILE_PATH_LITERAL("Google Profile.ico")))); On 2012/11/28 05:12:31, sail wrote: > should we just get rid of all these FILE_PATH_LITERALs? Done. I also made all of these use the constant from profile_shortcut_manager_win.cc instead of duplicating it. https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:372: const FilePath new_shortcut_path = On 2012/11/28 05:12:31, sail wrote: > maybe new_profile_1_shortcut_path I've made ValidateProfileShortcut() check the flags and simplified this part of the test to use, instead. https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:377: string16 shortcut_args; On 2012/11/28 05:12:31, sail wrote: > maybe profile_1_shortcut_args? This is now done in ValidateProfileShortcut(). https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:395: const FilePath second_shortcut_path = On 2012/11/28 05:12:31, sail wrote: > I find spelling out second harder to parse. How about regular_shortcut_path_1 > and regular_shortcut_path_2 > > Oh wait. So the first one is a standard chrome shortcut and the second one is a > custom made chrome shortcut? > > If so maybe name it as regular_shortcut_path, customized_regular_shortcut_path. Done. https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:397: EXPECT_TRUE(file_util::PathExists(second_shortcut_path)); On 2012/11/28 05:12:31, sail wrote: > don't need this Done. https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:405: const FilePath new_shortcut_path = On 2012/11/28 05:12:31, sail wrote: > same as above, maybe "new_profile_1_shortcut_path" Done. https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11299160/diff/5003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:131: bool include_empty_command_lines, On 2012/11/28 05:12:31, sail wrote: > Yea, I think plumbing the enums here would be great. > If that's really awkward then maybe name this to better match the enum? > Something like should_update_non_profile_shortcuts I've plumbed them through to CreateOrUpdateDesktopShortcutsForProfile(). I've kept the name of the parameter to ListDesktopShortcutsWithCommandLine(), since in that case, it describes the exact behavior it controls.
Looks good, some comments below on general shortcut things. https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:41: FilePath CreateProfileDirectory(const string16& profile_name) { Put helper methods after overriden methods SetUp() and TearDown(). https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:161: GetDefaultShortcutPathForProfile(profile_name), profile_path); nit: either bring down |location| to this line or indent everything to match the '('. https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:235: ASSERT_TRUE(file_util::PathExists(icon_path)); I don't feel this is sufficient to test the rename mechanism upon adding a 2nd profile, nor do I feel this mechanism should be tested in this first test (the first test should merely test creating a simple profile shortcut imo). Seems there is already a bigger testing the rename mechanism below.... is it at all useful to check the icon path here? What's the value of this test? https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:406: void ProfileShortcutManagerWin::UpdateShortcutsForProfileAtPath( I would rename this method to CreateOrUpdateShortcutsForProfileAtPath() for consistency. https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:423: if (create_mode == UPDATE_EXISTING_ONLY && Even if mode is CREATE_WHEN_NONE_FOUND don't you also want to update the shortcut if it is found? In the other shortcut APIs, UPDATE < REPLACE < CREATE (where '<' means it does strictly less things, i.e. UPDATE == change somethings on existing shortcut, REPLACE == overwrite existing shortcut, CREATE == overwrite existing shortcut, but also create it if it doesn't exist). https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.h (right): https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.h:26: string16 CreateProfileShortcutFlags(const FilePath& profile_path); A test should probably also be added for this newly exposed method?
https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:41: FilePath CreateProfileDirectory(const string16& profile_name) { On 2012/11/28 22:00:58, gab wrote: > Put helper methods after overriden methods SetUp() and TearDown(). Done. https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:161: GetDefaultShortcutPathForProfile(profile_name), profile_path); On 2012/11/28 22:00:58, gab wrote: > nit: either bring down |location| to this line or indent everything to match the > '('. Done. https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:235: ASSERT_TRUE(file_util::PathExists(icon_path)); On 2012/11/28 22:00:58, gab wrote: > I don't feel this is sufficient to test the rename mechanism upon adding a 2nd > profile, nor do I feel this mechanism should be tested in this first test (the > first test should merely test creating a simple profile shortcut imo). > > Seems there is already a bigger testing the rename mechanism below.... is it at > all useful to check the icon path here? What's the value of this test? The other parts of the shortcut get checked by CreateProfileWithShortcut() which calls ValidateProfileShortcut() now, so the remaining part of the test is just checking whether the custom badged icon got created. (It's not a new test, it's just now greatly simplified with the updated helpers.) https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:423: if (create_mode == UPDATE_EXISTING_ONLY && On 2012/11/28 22:00:58, gab wrote: > Even if mode is CREATE_WHEN_NONE_FOUND don't you also want to update the > shortcut if it is found? > > In the other shortcut APIs, UPDATE < REPLACE < CREATE (where '<' means it does > strictly less things, i.e. UPDATE == change somethings on existing shortcut, > REPLACE == overwrite existing shortcut, CREATE == overwrite existing shortcut, > but also create it if it doesn't exist). I noticed this too before, but this CL is not changing the logic here. If we do want to change this logic, we should also have a corresponding test that verifies it. I was planning to take a look at this after, in a separate CL if a change is needed here. (This CL is already getting long in the tooth - smaller changes are generally better.) https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.h (right): https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.h:26: string16 CreateProfileShortcutFlags(const FilePath& profile_path); On 2012/11/28 22:00:58, gab wrote: > A test should probably also be added for this newly exposed method? Let me do it in a separate CL instead of cramming more things into this one. (It's already changing way too much imho for a single CL.)
lgtm for shortcut-login (please let sail lgtm the overall code), better to keep this to not change previous behavior I agree, just noticed while around the lines changed that previous behavior is perhaps not ideal. Thanks, gab https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:423: if (create_mode == UPDATE_EXISTING_ONLY && On 2012/11/28 22:25:55, Alexei Svitkine wrote: > On 2012/11/28 22:00:58, gab wrote: > > Even if mode is CREATE_WHEN_NONE_FOUND don't you also want to update the > > shortcut if it is found? > > > > In the other shortcut APIs, UPDATE < REPLACE < CREATE (where '<' means it does > > strictly less things, i.e. UPDATE == change somethings on existing shortcut, > > REPLACE == overwrite existing shortcut, CREATE == overwrite existing shortcut, > > but also create it if it doesn't exist). > > I noticed this too before, but this CL is not changing the logic here. If we do > want to change this logic, we should also have a corresponding test that > verifies it. > > I was planning to take a look at this after, in a separate CL if a change is > needed here. > > (This CL is already getting long in the tooth - smaller changes are generally > better.) sgtm https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.h (right): https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.h:26: string16 CreateProfileShortcutFlags(const FilePath& profile_path); On 2012/11/28 22:25:55, Alexei Svitkine wrote: > On 2012/11/28 22:00:58, gab wrote: > > A test should probably also be added for this newly exposed method? > > Let me do it in a separate CL instead of cramming more things into this one. > (It's already changing way too much imho for a single CL.) Seems to me the CL exposing the method should also be the one introducing a test for it, but if you say you'll follow up that's fine too I guess.
On 2012/11/28 22:31:37, gab wrote: > lgtm for shortcut-login (please let sail lgtm the overall code), better to keep err... s/login/logic :) > this to not change previous behavior I agree, just noticed while around the > lines changed that previous behavior is perhaps not ideal. > > Thanks, > gab > > https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... > File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): > > https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... > chrome/browser/profiles/profile_shortcut_manager_win.cc:423: if (create_mode == > UPDATE_EXISTING_ONLY && > On 2012/11/28 22:25:55, Alexei Svitkine wrote: > > On 2012/11/28 22:00:58, gab wrote: > > > Even if mode is CREATE_WHEN_NONE_FOUND don't you also want to update the > > > shortcut if it is found? > > > > > > In the other shortcut APIs, UPDATE < REPLACE < CREATE (where '<' means it > does > > > strictly less things, i.e. UPDATE == change somethings on existing shortcut, > > > REPLACE == overwrite existing shortcut, CREATE == overwrite existing > shortcut, > > > but also create it if it doesn't exist). > > > > I noticed this too before, but this CL is not changing the logic here. If we > do > > want to change this logic, we should also have a corresponding test that > > verifies it. > > > > I was planning to take a look at this after, in a separate CL if a change is > > needed here. > > > > (This CL is already getting long in the tooth - smaller changes are generally > > better.) > > sgtm > > https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... > File chrome/browser/profiles/profile_shortcut_manager_win.h (right): > > https://codereview.chromium.org/11299160/diff/4003/chrome/browser/profiles/pr... > chrome/browser/profiles/profile_shortcut_manager_win.h:26: string16 > CreateProfileShortcutFlags(const FilePath& profile_path); > On 2012/11/28 22:25:55, Alexei Svitkine wrote: > > On 2012/11/28 22:00:58, gab wrote: > > > A test should probably also be added for this newly exposed method? > > > > Let me do it in a separate CL instead of cramming more things into this one. > > (It's already changing way too much imho for a single CL.) > > Seems to me the CL exposing the method should also be the one introducing a test > for it, but if you say you'll follow up that's fine too I guess.
LGTM!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asvitkine@chromium.org/11299160/5
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asvitkine@chromium.org/11299160/5
Message was sent while issue was closed.
Change committed as 170200 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
