Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10837352:
Update profile desktop shortcuts (Closed)
|
Created:
8 years, 4 months ago by Halli Modified:
8 years, 4 months ago CC:
chromium-reviews, rginda+watch_chromium.org, SteveT, rpetterson, mgaba Base URL:
https://src.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionUpdate an existing profile desktop shortcut with a new name, avatar, or both.
BUG=144305, 133585
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153333
Patch Set 1 #Patch Set 2 : #
Total comments: 8
Patch Set 3 : Fixed issues and added a unit test for update #
Total comments: 50
Patch Set 4 : #
Total comments: 15
Patch Set 5 : #
Total comments: 5
Patch Set 6 : #Patch Set 7 : #
Total comments: 6
Patch Set 8 : Fixed stub parameters for Profile Shortcut Manager #
Total comments: 12
Patch Set 9 : #Patch Set 10 : #
Total comments: 1
Messages
Total messages: 27 (0 generated)
This CL tightly couples the shortcut code with ProfileManager and the profiles UI. A better approach would be to listen to a notification when the profile settings change. One way to do this would be to have ProfileShortcutManagerWin implement the ProfileInfoCacheObserver interface.
Changed ProfileShortcutManager to implement the ProfileInfoCacheObserver interface (ProfileShortcutManagerWin overrides observer functions).
http://codereview.chromium.org/10837352/diff/7001/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.cc (right): http://codereview.chromium.org/10837352/diff/7001/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.cc:273: GetProfileInfoCache().AddObserver(profile_shortcut_manager_.get()); Another way to do this would be to have ProfileShortcutManagerWin add itself as an observer. One benefit of this is that this way you wouldn't have to change the ProfileShortcutManager interface. http://codereview.chromium.org/10837352/diff/7001/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): http://codereview.chromium.org/10837352/diff/7001/chrome/browser/profiles/pro... chrome/browser/profiles/profile_shortcut_manager_win.cc:139: const gfx::Image* avatar_image) { looks like the avatar_image is leaked here? http://codereview.chromium.org/10837352/diff/7001/chrome/browser/profiles/pro... chrome/browser/profiles/profile_shortcut_manager_win.cc:278: cache.GetIndexOfProfileWithPath(profile_path)); any time you call GetIndexOfProfileWithPath() you should get that the return value is not std::string::npos. http://codereview.chromium.org/10837352/diff/7001/chrome/browser/profiles/pro... chrome/browser/profiles/profile_shortcut_manager_win.cc:317: gfx::Image* avatar_copy = new gfx::Image(avatar_image); I don't think this is thread safe. There was a recent discussion about this on chromium-dev: https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-d... Could you ask oshima about this?
http://codereview.chromium.org/10837352/diff/7001/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.cc (right): http://codereview.chromium.org/10837352/diff/7001/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.cc:273: GetProfileInfoCache().AddObserver(profile_shortcut_manager_.get()); On 2012/08/21 21:59:26, sail wrote: > Another way to do this would be to have ProfileShortcutManagerWin add itself as > an observer. > > One benefit of this is that this way you wouldn't have to change the > ProfileShortcutManager interface. Done. http://codereview.chromium.org/10837352/diff/7001/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): http://codereview.chromium.org/10837352/diff/7001/chrome/browser/profiles/pro... chrome/browser/profiles/profile_shortcut_manager_win.cc:139: const gfx::Image* avatar_image) { On 2012/08/21 21:59:26, sail wrote: > looks like the avatar_image is leaked here? Done. http://codereview.chromium.org/10837352/diff/7001/chrome/browser/profiles/pro... chrome/browser/profiles/profile_shortcut_manager_win.cc:278: cache.GetIndexOfProfileWithPath(profile_path)); On 2012/08/21 21:59:26, sail wrote: > any time you call GetIndexOfProfileWithPath() you should get that the return > value is not std::string::npos. Done. http://codereview.chromium.org/10837352/diff/7001/chrome/browser/profiles/pro... chrome/browser/profiles/profile_shortcut_manager_win.cc:317: gfx::Image* avatar_copy = new gfx::Image(avatar_image); Oshima confirmed that gfx::Image and ToSkBitmap() are only thread safe on the UI thread - will do the refactoring in a separate CL. On 2012/08/21 21:59:26, sail wrote: > I don't think this is thread safe. There was a recent discussion about this on > chromium-dev: > https://groups.google.com/a/chromium.org/forum/?fromgroups=#%21topic/chromium... > > Could you ask oshima about this?
drive-by comment Cheers, Gab http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:134: void UpdateChromeDesktopShortcutForProfile( You do not need a whole new function to do this, simply change CreateChromeDesktopShortcutForProfile to CreateOrUpdateChromeDesktopShortcutForProfile and have it take a boolean "create" or something, than based on "create", pass in the "ShellUtil::SHORTCUT_CREATE_ALWAYS" property to CreateChromeDesktopShortcut or not (I know the name is confusing, but I'm changing all of this in upcoming http://codereview.chromium.org/10836247/ and doing it this way will be easier to merge with my CL).
Looks good overall. I think the unit tests are getting a bit long and hard to read. It might be a good idea to refactor them. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager.h (right): http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager.h:19: // Must be called on UI thread. Probably don't need this comment http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:78: profile_shortcut_manager->DeleteChromeDesktopShortcut( how about moving everything from this line and below to a separate DeleteShortcut test? http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:98: EXPECT_EQ(ShellUtil::VERIFY_SHORTCUT_FAILURE_UNEXPECTED, after creating the shortcut you check that the .ico file exists but after deleting it you check that the shortcut file doesn't exist. maybe it should be something like this: verify .ico and shortcut do not exist create_shortcut verify .ico and shortcut do exist delete shortcut verify .ico and shortcut do not exist http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:149: profile_shortcut_manager->DeleteChromeDesktopShortcut( should check that this succeeds http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:161: ProfileShortcutManager* profile_shortcut_manager = all this code is repeated for each test. this should probably be moved to ProfileShortcutManagerTest::SetUp() http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:195: FilePath shortcut; name is not descriptive, declaration should be move to first use http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:196: string16 shortcut_name; same, declaration should be move to first use http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:206: EXPECT_EQ(ShellUtil::VERIFY_SHORTCUT_SUCCESS, currently this test looks like this: <SetUp>create shortcut <Invoke function>change profile name <Test Post condition> verify shortcut has new profile name normally a test should look like this <SetUp> <Test Precondition> <Invoke function> <Test Post condition> Also, when your test gets to be longer than 10 lines it a good idea to refactor so that all of the above 4 steps are clear http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:210: profile_shortcut_manager->DeleteChromeDesktopShortcut( same as above, should check that this succeeds http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:120: const string16& old_shortcut, variable names could be more descriptive (i.e. shortcut name, shortcut file name, shortcut directory, etc...) http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:128: file_util::Move(old_profile, new_profile); if this fails can you log the error http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:132: // Updates the arguments to a Chrome desktop shortcut for a profile. Must be Comment doesn't match the function. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:135: const string16& shortcut, same as above, need more descriptive name http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:136: const string16& arguments, same, maybe exe_arguments? http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:154: FilePath icon_path = avatar_bitmap.isNull() ? when can the avatar bitmap be null? can this be changed to a DCHECK() instead? http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:169: void CompleteProfileShortcutCreation( function level comments. also function name is not very descriptive http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:170: const FilePath& profile_path, const string16& profile_name, one line per parameter would be easier to read http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:180: string16 description; variable declaration can be combined with assignment below http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:234: cache.AddObserver(manager); you might also need a RemoveObserver() call somewhere http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:249: if (avatar_bitmap->deepCopyTo(&avatar_bitmap_copy, this should just be a DCHECK I think, otherwise this should be changed to an early return also, need a comment explaining why you're doing a copy http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:264: profile_name, &shortcut)) { is this indentation correct? http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:290: g_browser_process->profile_manager()->GetProfileInfoCache(); It's weird that you're accessing the cache through the profile manager here and in the constructor you're having it passed in. I think you should pick one and use that everywhere. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:334: cache.GetDefaultAvatarIconResourceIDAtIndex(new_icon_index)); indentation is wrong http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:338: if (avatar_bitmap->deepCopyTo(&avatar_bitmap_copy, same comment as the above deepCopyTo call
Also, in general it's a bad idea to have tests that create non-temporary state (i.e. desktop shortcuts). Creating non-temporary state leads to non-shardable tests and flakiness. You might say shell_util_unittests are currently doing this in CreateChromeDesktopShortcutTests :), but I'm removing that as we speak, my goal is to override paths in PathService's cache so that we can create shortcuts on fake desktops, quick launch folder, start menu folders, etc. without shell_util realizing it's getting different paths. I am not sure what to recommend you for now (as asking you to do this would be duplicate work...)... I guess it's ok to create them like this for now and we can just keep the same test later while mocking the paths... So overall, no action to be taken just yet, but something to keep in mind! Cheers, Gab
Hm... for some reason I thought these shortcuts were being created in a temp folder. If there's not then we should fix that. On 2012/08/23 19:01:02, gab wrote: > Also, in general it's a bad idea to have tests that create non-temporary state > (i.e. desktop shortcuts). > > Creating non-temporary state leads to non-shardable tests and flakiness. > > You might say shell_util_unittests are currently doing this in > CreateChromeDesktopShortcutTests :), but I'm removing that as we speak, my goal > is to override paths in PathService's cache so that we can create shortcuts on > fake desktops, quick launch folder, start menu folders, etc. without shell_util > realizing it's getting different paths. > > I am not sure what to recommend you for now (as asking you to do this would be > duplicate work...)... I guess it's ok to create them like this for now and we > can just keep the same test later while mocking the paths... > > So overall, no action to be taken just yet, but something to keep in mind! > > Cheers, > Gab
They're not for now, but as mentioned I'm working on fixing that for shell_util which will transitively work for you if you are willing to make a common test target and re-use my upcoming code: http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/installer/util/shel... On 2012/08/23 19:02:12, sail wrote: > Hm... for some reason I thought these shortcuts were being created in a temp > folder. If there's not then we should fix that. > > On 2012/08/23 19:01:02, gab wrote: > > Also, in general it's a bad idea to have tests that create non-temporary state > > (i.e. desktop shortcuts). > > > > Creating non-temporary state leads to non-shardable tests and flakiness. > > > > You might say shell_util_unittests are currently doing this in > > CreateChromeDesktopShortcutTests :), but I'm removing that as we speak, my > goal > > is to override paths in PathService's cache so that we can create shortcuts on > > fake desktops, quick launch folder, start menu folders, etc. without > shell_util > > realizing it's getting different paths. > > > > I am not sure what to recommend you for now (as asking you to do this would be > > duplicate work...)... I guess it's ok to create them like this for now and we > > can just keep the same test later while mocking the paths... > > > > So overall, no action to be taken just yet, but something to keep in mind! > > > > Cheers, > > Gab
http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager.h (right): http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager.h:19: // Must be called on UI thread. On 2012/08/23 18:46:44, sail wrote: > Probably don't need this comment Done. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:78: profile_shortcut_manager->DeleteChromeDesktopShortcut( On 2012/08/23 18:46:44, sail wrote: > how about moving everything from this line and below to a separate > DeleteShortcut test? Done. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:98: EXPECT_EQ(ShellUtil::VERIFY_SHORTCUT_FAILURE_UNEXPECTED, On 2012/08/23 18:46:44, sail wrote: > after creating the shortcut you check that the .ico file exists but after > deleting it you check that the shortcut file doesn't exist. > > maybe it should be something like this: > verify .ico and shortcut do not exist > create_shortcut > verify .ico and shortcut do exist > delete shortcut > verify .ico and shortcut do not exist Done. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:149: profile_shortcut_manager->DeleteChromeDesktopShortcut( On 2012/08/23 18:46:44, sail wrote: > should check that this succeeds Done. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:161: ProfileShortcutManager* profile_shortcut_manager = On 2012/08/23 18:46:44, sail wrote: > all this code is repeated for each test. this should probably be moved to > ProfileShortcutManagerTest::SetUp() Moving this code to SetUp() causes a problem in ProfileInfoCache::AddProfileToCache on the DictionaryPrefUpdate, it fails the thread DCHECK, suggestions? (: http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:195: FilePath shortcut; On 2012/08/23 18:46:44, sail wrote: > name is not descriptive, declaration should be move to first use Done. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:196: string16 shortcut_name; On 2012/08/23 18:46:44, sail wrote: > same, declaration should be move to first use Done. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:206: EXPECT_EQ(ShellUtil::VERIFY_SHORTCUT_SUCCESS, On 2012/08/23 18:46:44, sail wrote: > currently this test looks like this: > <SetUp>create shortcut > <Invoke function>change profile name > <Test Post condition> verify shortcut has new profile name > normally a test should look like this > <SetUp> > <Test Precondition> > <Invoke function> > <Test Post condition> > > Also, when your test gets to be longer than 10 lines it a good idea to refactor > so that all of the above 4 steps are clear Done. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:210: profile_shortcut_manager->DeleteChromeDesktopShortcut( On 2012/08/23 18:46:44, sail wrote: > same as above, should check that this succeeds Done. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:120: const string16& old_shortcut, On 2012/08/23 18:46:44, sail wrote: > variable names could be more descriptive (i.e. shortcut name, shortcut file > name, shortcut directory, etc...) Done. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:128: file_util::Move(old_profile, new_profile); On 2012/08/23 18:46:44, sail wrote: > if this fails can you log the error Done. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:132: // Updates the arguments to a Chrome desktop shortcut for a profile. Must be n/a function has been removed On 2012/08/23 18:46:44, sail wrote: > Comment doesn't match the function. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:134: void UpdateChromeDesktopShortcutForProfile( On 2012/08/23 18:32:39, gab wrote: > You do not need a whole new function to do this, simply change > CreateChromeDesktopShortcutForProfile to > CreateOrUpdateChromeDesktopShortcutForProfile and have it take a boolean > "create" or something, than based on "create", pass in the > "ShellUtil::SHORTCUT_CREATE_ALWAYS" property to CreateChromeDesktopShortcut or > not (I know the name is confusing, but I'm changing all of this in upcoming > http://codereview.chromium.org/10836247/ and doing it this way will be easier to > merge with my CL). Done. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:135: const string16& shortcut, n/a function has been removed On 2012/08/23 18:46:44, sail wrote: > same as above, need more descriptive name http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:136: const string16& arguments, n/a function has been removed On 2012/08/23 18:46:44, sail wrote: > same, maybe exe_arguments? http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:154: FilePath icon_path = avatar_bitmap.isNull() ? It should be DCHECK(!avatar_image.empty()) before calling ToSkBitmap() and then it shouldn't be null. Fixed for the create version of this and removed this function. On 2012/08/23 18:46:44, sail wrote: > when can the avatar bitmap be null? can this be changed to a DCHECK() instead? http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:169: void CompleteProfileShortcutCreation( On 2012/08/23 18:46:44, sail wrote: > function level comments. > also function name is not very descriptive Done. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:170: const FilePath& profile_path, const string16& profile_name, On 2012/08/23 18:46:44, sail wrote: > one line per parameter would be easier to read Done. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:180: string16 description; On 2012/08/23 18:46:44, sail wrote: > variable declaration can be combined with assignment below Done. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:234: cache.AddObserver(manager); On 2012/08/23 18:46:44, sail wrote: > you might also need a RemoveObserver() call somewhere Done. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:249: if (avatar_bitmap->deepCopyTo(&avatar_bitmap_copy, On 2012/08/23 18:46:44, sail wrote: > this should just be a DCHECK I think, > otherwise this should be changed to an early return > > also, need a comment explaining why you're doing a copy Done. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:264: profile_name, &shortcut)) { On 2012/08/23 18:46:44, sail wrote: > is this indentation correct? Done. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:290: g_browser_process->profile_manager()->GetProfileInfoCache(); On 2012/08/23 18:46:44, sail wrote: > It's weird that you're accessing the cache through the profile manager here and > in the constructor you're having it passed in. > > I think you should pick one and use that everywhere. Done. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:334: cache.GetDefaultAvatarIconResourceIDAtIndex(new_icon_index)); On 2012/08/23 18:46:44, sail wrote: > indentation is wrong Done. http://codereview.chromium.org/10837352/diff/12001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:338: if (avatar_bitmap->deepCopyTo(&avatar_bitmap_copy, On 2012/08/23 18:46:44, sail wrote: > same comment as the above deepCopyTo call Done.
Looks pretty good so far! http://codereview.chromium.org/10837352/diff/14004/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): http://codereview.chromium.org/10837352/diff/14004/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:27: bool IsShortcutForProfile(const string16& profile_name) { maybe ProfileShortcutExists or DoesProfileShortcutWithNameExist http://codereview.chromium.org/10837352/diff/14004/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:33: // Get the desktop path of the current user user -> user. http://codereview.chromium.org/10837352/diff/14004/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:42: if (ShellUtil::VERIFY_SHORTCUT_SUCCESS == don't need if statement http://codereview.chromium.org/10837352/diff/14004/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:56: file_thread_(BrowserThread::FILE, &message_loop_) { also need to initialize cache_ and profile_shortcut_manager_ http://codereview.chromium.org/10837352/diff/14004/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:65: TestingProfileManager profile_manager(browser_process); this needs to be class scope http://codereview.chromium.org/10837352/diff/14004/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:123: string16 new_profile_name = ASCIIToUTF16("My New Profile Name"); this should move to the location of first use http://codereview.chromium.org/10837352/diff/14004/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:124: FilePath dest_path = cache_->GetUserDataDir(); the other test uses temp_dir.GetPath(). Is there a reason this is different?
http://codereview.chromium.org/10837352/diff/14004/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): http://codereview.chromium.org/10837352/diff/14004/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:27: bool IsShortcutForProfile(const string16& profile_name) { On 2012/08/24 04:32:26, sail wrote: > maybe ProfileShortcutExists or DoesProfileShortcutWithNameExist Done. http://codereview.chromium.org/10837352/diff/14004/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:33: // Get the desktop path of the current user On 2012/08/24 04:32:26, sail wrote: > user -> user. Done. http://codereview.chromium.org/10837352/diff/14004/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:42: if (ShellUtil::VERIFY_SHORTCUT_SUCCESS == On 2012/08/24 04:32:26, sail wrote: > don't need if statement Whoops! Done. http://codereview.chromium.org/10837352/diff/14004/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:56: file_thread_(BrowserThread::FILE, &message_loop_) { On 2012/08/24 04:32:26, sail wrote: > also need to initialize cache_ and profile_shortcut_manager_ Done. http://codereview.chromium.org/10837352/diff/14004/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:65: TestingProfileManager profile_manager(browser_process); On 2012/08/24 04:32:26, sail wrote: > this needs to be class scope Done. http://codereview.chromium.org/10837352/diff/14004/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:123: string16 new_profile_name = ASCIIToUTF16("My New Profile Name"); On 2012/08/24 04:32:26, sail wrote: > this should move to the location of first use Done. http://codereview.chromium.org/10837352/diff/14004/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:124: FilePath dest_path = cache_->GetUserDataDir(); On 2012/08/24 04:32:26, sail wrote: > the other test uses temp_dir.GetPath(). Is there a reason this is different? The cache requires that the profile data is in a separate directory from the user data dir. I can have the other one use a separate directory for consistency.
http://codereview.chromium.org/10837352/diff/14004/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): http://codereview.chromium.org/10837352/diff/14004/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:124: FilePath dest_path = cache_->GetUserDataDir(); On 2012/08/24 17:23:56, Halli wrote: > On 2012/08/24 04:32:26, sail wrote: > > the other test uses temp_dir.GetPath(). Is there a reason this is different? > The cache requires that the profile data is in a separate directory from the > user data dir. I can have the other one use a separate directory for > consistency. Yea, I think using a separate directory in the other test would help. http://codereview.chromium.org/10837352/diff/12002/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): http://codereview.chromium.org/10837352/diff/12002/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:29: DCHECK(PathService::Get(base::FILE_EXE, &exe_path)); ASSERT_TRUE instead? http://codereview.chromium.org/10837352/diff/12002/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:56: profile_manager_(NULL), don't need this and the next
Hey Halli, could you post the backtrace of the DCHECK() you're getting? I just realized that I don't have a Windows build setup. I'll try investigating it on my Mac. Thanks.
http://codereview.chromium.org/10837352/diff/12002/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): http://codereview.chromium.org/10837352/diff/12002/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:29: DCHECK(PathService::Get(base::FILE_EXE, &exe_path)); On 2012/08/24 17:32:21, sail wrote: > ASSERT_TRUE instead? Gives me- browser\profiles\profile_shortcut_manager_unittest_win.cc(29): error C2440: 'return' : cannot convert from 'void' to 'bool' using CHECK() http://codereview.chromium.org/10837352/diff/12002/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:56: profile_manager_(NULL), On 2012/08/24 17:32:21, sail wrote: > don't need this and the next Done.
Cool. Everything looks good. The only remaining issue is the temp_dir.GetPath() vs cache_->GetUserDataDir()
On 2012/08/24 18:01:45, sail wrote: > Cool. Everything looks good. The only remaining issue is the temp_dir.GetPath() > vs cache_->GetUserDataDir() The Update test needs to use the user data dir in order to access the info cache. I changed the Create test to use this as well (it can use any temporary directory) for consistency.
LGTM!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hallielaine@chromium.org/10837352/15004
Key comment below. http://codereview.chromium.org/10837352/diff/15004/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): http://codereview.chromium.org/10837352/diff/15004/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:43: ShellUtil::VerifyChromeShortcut( I'm currently augmenting VerifyChromeShortcut with more properties and moving it in test_support_base. It should not be used here in non-test land (and I don't think it achieves what you want here anyways). I think a file_util::PathExists() and potentially a ShellUtil::ResolveShortcut to make sure it points to chrome.exe is sufficient. You could also create (or find an existing if there is, I'm not sure) a method that checks the arguments on the shortcut if you care (i.e. to double check which profile it's actually meant to open).
Ok went back over everything smoothly now (sorry thought you'd wait for my lgt-to-the-m :), but wanted to wait on main feedback being addressed first), my bad for freaking out originally, your usage of VerifyChromeShortcut *is* in test code so that's fine :). Couple comments below as I looked closer at the CL. Cheers, Gab http://codereview.chromium.org/10837352/diff/12002/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): http://codereview.chromium.org/10837352/diff/12002/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:29: DCHECK(PathService::Get(base::FILE_EXE, &exe_path)); On 2012/08/24 17:42:40, Halli wrote: > On 2012/08/24 17:32:21, sail wrote: > > ASSERT_TRUE instead? > Gives me- browser\profiles\profile_shortcut_manager_unittest_win.cc(29): error > C2440: 'return' : cannot convert from 'void' to 'bool' > using CHECK() Hmmm... it doesn't make any sense to me that you can CHECK(), but not ASSERT_TRUE() or EXPECT_TRUE()... http://codereview.chromium.org/10837352/diff/15004/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): http://codereview.chromium.org/10837352/diff/15004/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:43: ShellUtil::VerifyChromeShortcut( Wait, my bad, in my panic to review quick as I saw the commit go in I thought this was non-test code... it obviously is test code, so usage of VerifyChromeShortcut() here is fine. On 2012/08/24 18:37:53, gab wrote: > I'm currently augmenting VerifyChromeShortcut with more properties and moving it > in test_support_base. It should not be used here in non-test land (and I don't > think it achieves what you want here anyways). > > I think a file_util::PathExists() and potentially a ShellUtil::ResolveShortcut > to make sure it points to chrome.exe is sufficient. > > You could also create (or find an existing if there is, I'm not sure) a method > that checks the arguments on the shortcut if you care (i.e. to double check > which profile it's actually meant to open). http://codereview.chromium.org/10837352/diff/15004/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:82: dest_path = dest_path.Append(FILE_PATH_LITERAL("New Profile 1")); Common parts at the beginning and end of the shortcut tests should be in the test fixture's SetUp and TearDown methods imo. http://codereview.chromium.org/10837352/diff/15004/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:102: profile_shortcut_manager_->DeleteProfileDesktopShortcut( You should do this deletion as part of TearDown of your test fixture uniformly for all tests. http://codereview.chromium.org/10837352/diff/19007/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): http://codereview.chromium.org/10837352/diff/19007/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:27: bool ProfileShortcutExists(const string16& profile_name) { Rename this to VerifyProfileShortcut and have it return the VerifyShortcutStatus so that if it fails the EXPECT_EQ logs in your tests will be more precise on the failure than just "Expected true, but was false". http://codereview.chromium.org/10837352/diff/19007/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:39: &shortcut_name); nit: indent this to align with |dist|. http://codereview.chromium.org/10837352/diff/19007/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:108: (FILE_PATH_LITERAL("Google Profile.ico"))))); nit: Remove extra brackets around FILE_PATH_LITERAL() http://codereview.chromium.org/10837352/diff/19007/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): http://codereview.chromium.org/10837352/diff/19007/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:244: &ShellUtil::RemoveChromeDesktopShortcutsWithAppendedNames, Is RemoveChromeDesktopShortcutsWithAppendedNames() ever used now in its plural form now (i.e. with more than one element in the vector to delete)? If not it should be changed to a singular RemoveChromeDesktopShortcutsWithAppendedName as the implementation of it is currently overly complex for the singular usage.
straddling comments http://codereview.chromium.org/10837352/diff/19007/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): http://codereview.chromium.org/10837352/diff/19007/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:150: string16 description(WideToUTF16(dist->GetAppDescription())); dist->GetAppDescription() does return a string16, not sure why you are converting it here as well? http://codereview.chromium.org/10837352/diff/19007/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:153: ShellUtil::SHORTCUT_NO_OPTIONS; optional nit: Align with ShellUtil::SHORTCUT_CREATE_ALWAYS (I don't the style enforces that, but I find it easier to read...)
http://codereview.chromium.org/10837352/diff/15004/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): http://codereview.chromium.org/10837352/diff/15004/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:82: dest_path = dest_path.Append(FILE_PATH_LITERAL("New Profile 1")); On 2012/08/24 19:17:32, gab wrote: > Common parts at the beginning and end of the shortcut tests should be in the > test fixture's SetUp and TearDown methods imo. Done. http://codereview.chromium.org/10837352/diff/15004/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:102: profile_shortcut_manager_->DeleteProfileDesktopShortcut( On 2012/08/24 19:17:32, gab wrote: > You should do this deletion as part of TearDown of your test fixture uniformly > for all tests. Done. http://codereview.chromium.org/10837352/diff/19007/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): http://codereview.chromium.org/10837352/diff/19007/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:27: bool ProfileShortcutExists(const string16& profile_name) { On 2012/08/24 19:17:32, gab wrote: > Rename this to VerifyProfileShortcut and have it return the > VerifyShortcutStatus so that if it fails the EXPECT_EQ logs in your tests will > be more precise on the failure than just "Expected true, but was false". Done. http://codereview.chromium.org/10837352/diff/19007/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:39: &shortcut_name); On 2012/08/24 19:17:33, gab wrote: > nit: indent this to align with |dist|. Done. http://codereview.chromium.org/10837352/diff/19007/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:108: (FILE_PATH_LITERAL("Google Profile.ico"))))); On 2012/08/24 19:17:33, gab wrote: > nit: Remove extra brackets around FILE_PATH_LITERAL() Done. http://codereview.chromium.org/10837352/diff/19007/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): http://codereview.chromium.org/10837352/diff/19007/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:150: string16 description(WideToUTF16(dist->GetAppDescription())); On 2012/08/24 19:28:16, gab wrote: > dist->GetAppDescription() does return a string16, not sure why you are > converting it here as well? Done. http://codereview.chromium.org/10837352/diff/19007/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:153: ShellUtil::SHORTCUT_NO_OPTIONS; On 2012/08/24 19:28:16, gab wrote: > optional nit: Align with ShellUtil::SHORTCUT_CREATE_ALWAYS (I don't the style > enforces that, but I find it easier to read...) Done. http://codereview.chromium.org/10837352/diff/19007/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_win.cc:244: &ShellUtil::RemoveChromeDesktopShortcutsWithAppendedNames, On 2012/08/24 19:17:33, gab wrote: > Is RemoveChromeDesktopShortcutsWithAppendedNames() ever used now in its plural > form now (i.e. with more than one element in the vector to delete)? If not it > should be changed to a singular RemoveChromeDesktopShortcutsWithAppendedName as > the implementation of it is currently overly complex for the singular usage. This will be used soon to remove all Profile shortcuts on uninstall!
LGTM :)! http://codereview.chromium.org/10837352/diff/23010/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): http://codereview.chromium.org/10837352/diff/23010/chrome/browser/profiles/pr... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:78: EXPECT_EQ(ShellUtil::VERIFY_SHORTCUT_FAILURE_UNEXPECTED, Hmmm that makes me think we probably want a VERIFY_SHORTCUT_FAILURE_NOT_FOUND, but I'll take care of that since I'm in the middle of moving this code anyways.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hallielaine@chromium.org/10837352/23010
Change committed as 153333 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
