Create/Delete windows profile shortcuts

Create and delete desktop shortcuts in general.

Currently planning 6 steps for this feature:
1. (this step) Create/delete shortcuts in general (few edge cases)
2. Add functionality for the checkbox- allow the user to select whether they want a shortcut or not from the create/manage profile dialogs
3. Find shortcuts on the desktop by their arguments instead of the path.
4. Handle edge case of 1 to 2 and 2 to 1 profiles (adding/removing icon and name from the shortcut)
5. Update shortcuts when the user changes their name/icon
6. Repopulate shortcuts map when browser starts up (to allow delete/update on new browser instances) 

This CL depends on: Issue 10826188

BUG=133585
TEST=manual testing

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151974

[sail, 2012-08-08 20:06:15]

Hi Halli, normally we don't check in debug code.

When checking in partially completed work we put it behind a command line flag
and maintain the existing behavior when the command line is present.

For example, with this change creating a new profile won't work correctly for
Canary users.

[sail, 2012-08-08 20:06:46]

> When checking in partially completed work we put it behind a command line flag
> and maintain the existing behavior when the command line is present.

argh, that should be "when the command line is NOT present"

[SteveT, 2012-08-09 18:07:39]

A few drive by nits (I haven't really looked at the rest of the code... I'll
leave that to the main reviewers).

http://codereview.chromium.org/10823217/diff/6002/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main_win.cc (right):

http://codereview.chromium.org/10823217/diff/6002/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main_win.cc:139: }*/
Is this temporary? (will this code be uncommented when this patch is committed?)

We generally try to avoid hunks of commented-out code unless they're actually
for documentation.

http://codereview.chromium.org/10823217/diff/6002/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10823217/diff/6002/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:1970:
'browser/profiles/profile_shortcut_manager_stub.cc',
nit: indenting is wonky

http://codereview.chromium.org/10823217/diff/6002/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:5198: ['exclude',
'browser/profiles/profile_shortcut_manager_stub.cc'],
nit: indenting is wonky plus the line exceeds 80 chars.

http://codereview.chromium.org/10823217/diff/6002/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/10823217/diff/6002/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:1065: const char kProfileDesktopShortcuts[]    
  ="profile-desktop-shortcuts";
nit: space between the = and "

[Halli, 2012-08-09 18:12:30]

http://codereview.chromium.org/10823217/diff/6002/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main_win.cc (right):

http://codereview.chromium.org/10823217/diff/6002/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main_win.cc:139: }*/
I just missed this one when deleting old code, thanks!
On 2012/08/09 18:07:39, SteveT wrote:
> Is this temporary? (will this code be uncommented when this patch is
committed?)
> 
> We generally try to avoid hunks of commented-out code unless they're actually
> for documentation.

http://codereview.chromium.org/10823217/diff/6002/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10823217/diff/6002/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:1970:
'browser/profiles/profile_shortcut_manager_stub.cc',
On 2012/08/09 18:07:39, SteveT wrote:
> nit: indenting is wonky

Done.

http://codereview.chromium.org/10823217/diff/6002/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:5198: ['exclude',
'browser/profiles/profile_shortcut_manager_stub.cc'],
Didn't realize this file uses tabs (:
On 2012/08/09 18:07:39, SteveT wrote:
> nit: indenting is wonky plus the line exceeds 80 chars.

http://codereview.chromium.org/10823217/diff/6002/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/10823217/diff/6002/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:1065: const char kProfileDesktopShortcuts[]    
  ="profile-desktop-shortcuts";
On 2012/08/09 18:07:39, SteveT wrote:
> nit: space between the = and "

Done.

[Halli, 2012-08-09 18:19:14]

Back to spaces for the gyp...

[sail, 2012-08-10 18:01:14]

http://codereview.chromium.org/10823217/diff/6013/chrome/browser/profiles/ava...
File chrome/browser/profiles/avatar_menu_model.cc (right):

http://codereview.chromium.org/10823217/diff/6013/chrome/browser/profiles/ava...
chrome/browser/profiles/avatar_menu_model.cc:106: // Temporarily generate a
random icon/name pair to create the profile with
This will break creating a new profile.

Do you really need to check this in?

http://codereview.chromium.org/10823217/diff/6013/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/10823217/diff/6013/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:460:
cache.GetDefaultAvatarIconResourceIDAtIndex(icon_index))));
should be indented

http://codereview.chromium.org/10823217/diff/6013/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/10823217/diff/6013/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:154: if
(ProfileShortcutManager::IsFeatureEnabled()) {
This check should be done by the caller. This way you don't have to modify the
command line args in the unit test. This will also allow you to remove all the
#ifdefs from the unit tests.

For example:
ProfileShortcutManager* profile_shortcut_manager = NULL;
if (ProfileShortcutManager::IsFeatureEnabled())
  profile_shortcut_manager = ProfileShortcutManager::Create();
...
if (profile_shortcut_manager)
  profile_shortcut_manager->CreateDesktopShortcut();

http://codereview.chromium.org/10823217/diff/6013/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10823217/diff/6013/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:1970:
'browser/profiles/profile_shortcut_manager_stub.cc',
It looks like these files are missing from this CL

http://codereview.chromium.org/10823217/diff/6013/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/10823217/diff/6013/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:1065: const char kProfileDesktopShortcuts[]    
  = "profile-desktop-shortcuts";
maybe enable-profile-desktop-shortcuts?

[Halli, 2012-08-13 21:50:10]

http://codereview.chromium.org/10823217/diff/6013/chrome/browser/profiles/ava...
File chrome/browser/profiles/avatar_menu_model.cc (right):

http://codereview.chromium.org/10823217/diff/6013/chrome/browser/profiles/ava...
chrome/browser/profiles/avatar_menu_model.cc:106: // Temporarily generate a
random icon/name pair to create the profile with
Shortcuts can't be generated from the avatar menu without this. But other than
that it's fine to not be checked in. I can still create new profiles with this
code, I think it follows the same path as when you create from the Create Dialog
as if you accepted the randomized avatar/name choice. Does this affect the
logging/cache in some way?
On 2012/08/10 18:01:14, sail wrote:
> This will break creating a new profile.
> 
> Do you really need to check this in?

http://codereview.chromium.org/10823217/diff/6013/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/10823217/diff/6013/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:460:
cache.GetDefaultAvatarIconResourceIDAtIndex(icon_index))));
On 2012/08/10 18:01:14, sail wrote:
> should be indented

Done.

http://codereview.chromium.org/10823217/diff/6013/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/10823217/diff/6013/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:154: if
(ProfileShortcutManager::IsFeatureEnabled()) {
On 2012/08/10 18:01:14, sail wrote:
> This check should be done by the caller. This way you don't have to modify the
> command line args in the unit test. This will also allow you to remove all the
> #ifdefs from the unit tests.
> 
> For example:
> ProfileShortcutManager* profile_shortcut_manager = NULL;
> if (ProfileShortcutManager::IsFeatureEnabled())
>   profile_shortcut_manager = ProfileShortcutManager::Create();
> ...
> if (profile_shortcut_manager)
>   profile_shortcut_manager->CreateDesktopShortcut();

Done.

http://codereview.chromium.org/10823217/diff/6013/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10823217/diff/6013/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:1970:
'browser/profiles/profile_shortcut_manager_stub.cc',
On 2012/08/10 18:01:14, sail wrote:
> It looks like these files are missing from this CL

Done.

http://codereview.chromium.org/10823217/diff/6013/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/10823217/diff/6013/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:1065: const char kProfileDesktopShortcuts[]    
  = "profile-desktop-shortcuts";
On 2012/08/10 18:01:14, sail wrote:
> maybe enable-profile-desktop-shortcuts?

Done.

[sail, 2012-08-13 22:09:36]

http://codereview.chromium.org/10823217/diff/6013/chrome/browser/profiles/ava...
File chrome/browser/profiles/avatar_menu_model.cc (right):

http://codereview.chromium.org/10823217/diff/6013/chrome/browser/profiles/ava...
chrome/browser/profiles/avatar_menu_model.cc:106: // Temporarily generate a
random icon/name pair to create the profile with
On 2012/08/13 21:50:10, Halli wrote:
> Shortcuts can't be generated from the avatar menu without this. But other than
> that it's fine to not be checked in. I can still create new profiles with this
> code, I think it follows the same path as when you create from the Create
Dialog
> as if you accepted the randomized avatar/name choice. Does this affect the
> logging/cache in some way?
> On 2012/08/10 18:01:14, sail wrote:
> > This will break creating a new profile.
> > 
> > Do you really need to check this in?
> 

Yea, if you really need this then you should just do
ProfileShortcutManager::IsFeatureEnabled().

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:681: profile_shortcut_manager_ =
NULL;
this should be in the constructor's initializer list

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:682: if
(ProfileShortcutManager::IsFeatureEnabled()) {
don't need braces
also, this probably belongs in the constructor

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager_unittest.cc:58: ProfileShortcutManager*
profile_shortcut_manager;
you don't need a global for this, this can just be created inside the test

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager_unittest.cc:580:
TEST_F(ProfileManagerTest, WinDesktopShortcutsIconExists) {
This isn't related to profile manager so it should go inside
profile_shortcut_manager_unittest.cc

Also, most of this isn't windows specific so you don't need Win in the test
name.

The windows specific can probably be put into a #ifdef

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager.h (right):

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager.h:8: #include <map>
not needed?

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager.h:19: string16 profile_name,
const string16&

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager.h:20: const gfx::Image&
avatar_image);
this should be abstract (=0)
same below

The non abstract, non-static function should be ~ProfileShortcutManager() which
should be implemented in profile_shortcut_manager.cc

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_stub.cc (right):

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_stub.cc:7:
ProfileShortcutManager::ProfileShortcutManager() {
You don't need to implement any of the non static ProfileShortcutManager
functions in this file

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_stub.cc:17: bool
ProfileShortcutManager::IsFeatureEnabled() {
need "// static" in the line above
same below

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:135:
ProfileShortcutManager::ProfileShortcutManager() {
This should be a separate class, ProfileShortcutManagerWin, which derives from
ProfileShortcutManager

[sail, 2012-08-13 22:14:35]

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager_unittest.cc:596:
ASSERT_TRUE(file_util::ContainsPath(temp_dir_.path(), dest_path.Append(
maybe file_util::PathExists() instead of ContainsPath() ?

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager_unittest.cc:599:
profile_shortcut_manager->DeleteChromeDesktopShortcut(dest_path);
maybe test that the file no longer exists?

[Halli, 2012-08-14 18:25:56]

http://codereview.chromium.org/10823217/diff/6013/chrome/browser/profiles/ava...
File chrome/browser/profiles/avatar_menu_model.cc (right):

http://codereview.chromium.org/10823217/diff/6013/chrome/browser/profiles/ava...
chrome/browser/profiles/avatar_menu_model.cc:106: // Temporarily generate a
random icon/name pair to create the profile with
On 2012/08/13 22:09:36, sail wrote:
> On 2012/08/13 21:50:10, Halli wrote:
> > Shortcuts can't be generated from the avatar menu without this. But other
than
> > that it's fine to not be checked in. I can still create new profiles with
this
> > code, I think it follows the same path as when you create from the Create
> Dialog
> > as if you accepted the randomized avatar/name choice. Does this affect the
> > logging/cache in some way?
> > On 2012/08/10 18:01:14, sail wrote:
> > > This will break creating a new profile.
> > > 
> > > Do you really need to check this in?
> > 
> 
> Yea, if you really need this then you should just do
> ProfileShortcutManager::IsFeatureEnabled().

Done.

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:681: profile_shortcut_manager_ =
NULL;
On 2012/08/13 22:09:36, sail wrote:
> this should be in the constructor's initializer list

Done.

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:682: if
(ProfileShortcutManager::IsFeatureEnabled()) {
On 2012/08/13 22:09:36, sail wrote:
> don't need braces
> also, this probably belongs in the constructor

Done.

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager_unittest.cc:58: ProfileShortcutManager*
profile_shortcut_manager;
Kept as a global in the new test class profile_shortcut_manager_unittest since
all tests use it.
On 2012/08/13 22:09:36, sail wrote:
> you don't need a global for this, this can just be created inside the test

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager_unittest.cc:580:
TEST_F(ProfileManagerTest, WinDesktopShortcutsIconExists) {
On 2012/08/13 22:09:36, sail wrote:
> This isn't related to profile manager so it should go inside
> profile_shortcut_manager_unittest.cc
> 
> Also, most of this isn't windows specific so you don't need Win in the test
> name.
> 
> The windows specific can probably be put into a #ifdef

Done.

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager_unittest.cc:596:
ASSERT_TRUE(file_util::ContainsPath(temp_dir_.path(), dest_path.Append(
On 2012/08/13 22:14:35, sail wrote:
> maybe file_util::PathExists() instead of ContainsPath() ?

Done.

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager_unittest.cc:599:
profile_shortcut_manager->DeleteChromeDesktopShortcut(dest_path);
On 2012/08/13 22:14:35, sail wrote:
> maybe test that the file no longer exists?
TODO in separate CL

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager.h (right):

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager.h:8: #include <map>
Moved to profile_shortcut_manager_win.cc
On 2012/08/13 22:09:36, sail wrote:
> not needed?

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager.h:19: string16 profile_name,
On 2012/08/13 22:09:36, sail wrote:
> const string16&

Done.

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager.h:20: const gfx::Image&
avatar_image);
On 2012/08/13 22:09:36, sail wrote:
> this should be abstract (=0)
> same below
> 
> The non abstract, non-static function should be ~ProfileShortcutManager()
which
> should be implemented in profile_shortcut_manager.cc

Done.

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_stub.cc (right):

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_stub.cc:7:
ProfileShortcutManager::ProfileShortcutManager() {
On 2012/08/13 22:09:36, sail wrote:
> You don't need to implement any of the non static ProfileShortcutManager
> functions in this file

Done.

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_stub.cc:17: bool
ProfileShortcutManager::IsFeatureEnabled() {
On 2012/08/13 22:09:36, sail wrote:
> need "// static" in the line above
> same below

Done.

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/10823217/diff/9006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:135:
ProfileShortcutManager::ProfileShortcutManager() {
On 2012/08/13 22:09:36, sail wrote:
> This should be a separate class, ProfileShortcutManagerWin, which derives from
> ProfileShortcutManager

Done.

[sail, 2012-08-14 18:40:41]

http://codereview.chromium.org/10823217/diff/8008/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.h (right):

http://codereview.chromium.org/10823217/diff/8008/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.h:297: ProfileShortcutManager*
profile_shortcut_manager_;
should be scoped_ptr

http://codereview.chromium.org/10823217/diff/8008/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager.h (right):

http://codereview.chromium.org/10823217/diff/8008/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager.h:13: ProfileShortcutManager();
normally this ordered like this

static

constructor
destructor

remaining functions

http://codereview.chromium.org/10823217/diff/8008/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_unittest.cc (right):

http://codereview.chromium.org/10823217/diff/8008/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_unittest.cc:29: // Profile
shortcut manager for desktop shortcuts
this should just be declared inside the test directly, you don't need a global

http://codereview.chromium.org/10823217/diff/8008/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_unittest.cc:36: class
ProfileManager : public ::ProfileManagerWithoutInit {
you don't actually need a profile manager or a profile info cache to test the
shortcut code.

The shortcut code can take any arbitrary image or name. So you could do
something like this:
  image = GetNativeImageNamed(IDR_PROFILE_AVATAR_0)
  name = ASCIIToUTF16("My Profile");
  Create(image, name)

http://codereview.chromium.org/10823217/diff/8008/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_unittest.cc:67:
extension_event_router_forwarder_(
I don't think you need most of this. You probably need the file_thread_, the
RunAllPending() call, and the temp_dir_. Everything else can be removed (I
think).

[Halli, 2012-08-14 20:38:46]

http://codereview.chromium.org/10823217/diff/8008/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.h (right):

http://codereview.chromium.org/10823217/diff/8008/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.h:297: ProfileShortcutManager*
profile_shortcut_manager_;
On 2012/08/14 18:40:41, sail wrote:
> should be scoped_ptr

Done.

http://codereview.chromium.org/10823217/diff/8008/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager.h (right):

http://codereview.chromium.org/10823217/diff/8008/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager.h:13: ProfileShortcutManager();
On 2012/08/14 18:40:41, sail wrote:
> normally this ordered like this
> 
> static
> 
> constructor
> destructor
> 
> remaining functions

Done.

http://codereview.chromium.org/10823217/diff/8008/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_unittest.cc (right):

http://codereview.chromium.org/10823217/diff/8008/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_unittest.cc:29: // Profile
shortcut manager for desktop shortcuts
On 2012/08/14 18:40:41, sail wrote:
> this should just be declared inside the test directly, you don't need a global

Done.

http://codereview.chromium.org/10823217/diff/8008/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_unittest.cc:36: class
ProfileManager : public ::ProfileManagerWithoutInit {
On 2012/08/14 18:40:41, sail wrote:
> you don't actually need a profile manager or a profile info cache to test the
> shortcut code.
> 
> The shortcut code can take any arbitrary image or name. So you could do
> something like this:
>   image = GetNativeImageNamed(IDR_PROFILE_AVATAR_0)
>   name = ASCIIToUTF16("My Profile");
>   Create(image, name)

Done.

http://codereview.chromium.org/10823217/diff/8008/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_unittest.cc:67:
extension_event_router_forwarder_(
On 2012/08/14 18:40:41, sail wrote:
> I don't think you need most of this. You probably need the file_thread_, the
> RunAllPending() call, and the temp_dir_. Everything else can be removed (I
> think).

Done.

[sail, 2012-08-14 20:53:17]

http://codereview.chromium.org/10823217/diff/10008/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_shortcut_manager_unittest.cc (right):

http://codereview.chromium.org/10823217/diff/10008/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_shortcut_manager_unittest.cc:64:
profile_shortcut_manager->DeleteChromeDesktopShortcut(dest_path);
maybe add a TODO to verify deletion?

http://codereview.chromium.org/10823217/diff/10008/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_shortcut_manager_unittest.cc:100:
shortcut.value(), dist->GetAppDescription(), 0));
this should be indented 4 spaces

http://codereview.chromium.org/10823217/diff/10008/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/10823217/diff/10008/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_shortcut_manager_win.cc:60: ProfileShortcutsMap
profile_shortcuts_;
this shouldn't be a global variable
Also, I don't think you actually need this.

This is being used to look up previously created profiles. But that only works
for shortcuts created in this session (since Chrome was launched).

If a shortcut was created earlier then you won't be able to delete it.

http://codereview.chromium.org/10823217/diff/10008/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_shortcut_manager_win.cc:145: const gfx::Image&
avatar_image);
need OVERRIDE here and below

http://codereview.chromium.org/10823217/diff/10008/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_shortcut_manager_win.cc:182: else
no need for else

http://codereview.chromium.org/10823217/diff/10008/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_shortcut_manager_win.cc:208: if
(profile_shortcuts_.count(profile_path)) {
I don't think you need this if statement at all, just delete the shortcut if you
can find it

[Halli, 2012-08-14 21:24:19]

http://codereview.chromium.org/10823217/diff/10008/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_shortcut_manager_unittest.cc (right):

http://codereview.chromium.org/10823217/diff/10008/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_shortcut_manager_unittest.cc:64:
profile_shortcut_manager->DeleteChromeDesktopShortcut(dest_path);
On 2012/08/14 20:53:17, sail wrote:
> maybe add a TODO to verify deletion?

Done.

http://codereview.chromium.org/10823217/diff/10008/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_shortcut_manager_unittest.cc:100:
shortcut.value(), dist->GetAppDescription(), 0));
On 2012/08/14 20:53:17, sail wrote:
> this should be indented 4 spaces

Done.

http://codereview.chromium.org/10823217/diff/10008/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/10823217/diff/10008/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_shortcut_manager_win.cc:60: ProfileShortcutsMap
profile_shortcuts_;
Added TODO for map and moved profile_shortcuts_ into ProfileShortcutManagerWin 
On 2012/08/14 20:53:17, sail wrote:
> this shouldn't be a global variable
> Also, I don't think you actually need this.
> 
> This is being used to look up previously created profiles. But that only works
> for shortcuts created in this session (since Chrome was launched).
> 
> If a shortcut was created earlier then you won't be able to delete it.

http://codereview.chromium.org/10823217/diff/10008/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_shortcut_manager_win.cc:145: const gfx::Image&
avatar_image);
On 2012/08/14 20:53:17, sail wrote:
> need OVERRIDE here and below

Done.

http://codereview.chromium.org/10823217/diff/10008/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_shortcut_manager_win.cc:182: else
On 2012/08/14 20:53:17, sail wrote:
> no need for else

Done.

http://codereview.chromium.org/10823217/diff/10008/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_shortcut_manager_win.cc:208: if
(profile_shortcuts_.count(profile_path)) {
On 2012/08/14 20:53:17, sail wrote:
> I don't think you need this if statement at all, just delete the shortcut if
you
> can find it

Done.

[sail, 2012-08-14 21:34:57]

LGTM!

http://codereview.chromium.org/10823217/diff/27/chrome/browser/profiles/profi...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/10823217/diff/27/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_shortcut_manager_win.cc:137:
ProfileShortcutInfo() : flags(string16()), profile_name(string16()),
I don't think you need these empty constructors

http://codereview.chromium.org/10823217/diff/27/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_shortcut_manager_win.cc:142: string16 p_flags,
string16 p_profile_name, gfx::Image p_avatar_image)
this would be easier to read if the arguments were all on separate lines, same
with the initializers

http://codereview.chromium.org/10823217/diff/27/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_shortcut_manager_win.cc:193: avatar_image)));
indent by 4 more

http://codereview.chromium.org/10823217/diff/27/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_shortcut_manager_win.cc:219:
&ShellUtil::RemoveChromeDesktopShortcutsWithAppendedNames,
need to indent by 4 more, next line as well

[Halli, 2012-08-14 22:19:47]

ben@ added for OWNERS approval for chrome and chrome/test

http://codereview.chromium.org/10823217/diff/27/chrome/browser/profiles/profi...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/10823217/diff/27/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_shortcut_manager_win.cc:137:
ProfileShortcutInfo() : flags(string16()), profile_name(string16()),
I get the following error without the default constructor:
error C2512:
'ProfileShortcutManagerWin::ProfileShortcutInfo::ProfileShortcutInfo' : no
appropriate default constructor available
3>          c:\Program Files (x86)\Microsoft Visual Studio
10.0\VC\include\map(210) : while compiling class template member function
'ProfileShortcutManagerWin::ProfileShortcutInfo &std::map<_Kty,_Ty>::operator
[](const FilePath &)'
3>          with
3>          [
3>              _Kty=FilePath,
3>              _Ty=ProfileShortcutManagerWin::ProfileShortcutInfo
3>          ]
3>          browser\profiles\profile_shortcut_manager_win.cc(149) : see
reference to class template instantiation 'std::map<_Kty,_Ty>' being compiled
3>          with
3>          [
3>              _Kty=FilePath,
3>              _Ty=ProfileShortcutManagerWin::ProfileShortcutInfo
3>          ]
On 2012/08/14 21:34:57, sail wrote:
> I don't think you need these empty constructors

http://codereview.chromium.org/10823217/diff/27/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_shortcut_manager_win.cc:142: string16 p_flags,
string16 p_profile_name, gfx::Image p_avatar_image)
On 2012/08/14 21:34:57, sail wrote:
> this would be easier to read if the arguments were all on separate lines, same
> with the initializers

Done.

http://codereview.chromium.org/10823217/diff/27/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_shortcut_manager_win.cc:193: avatar_image)));
On 2012/08/14 21:34:57, sail wrote:
> indent by 4 more

Done.

http://codereview.chromium.org/10823217/diff/27/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_shortcut_manager_win.cc:219:
&ShellUtil::RemoveChromeDesktopShortcutsWithAppendedNames,
On 2012/08/14 21:34:57, sail wrote:
> need to indent by 4 more, next line as well

Done.

[Ben Goodger (Google), 2012-08-14 22:43:19]

LGTM for chrome bits modulo my nits below:

http://codereview.chromium.org/10823217/diff/3015/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager.h (right):

http://codereview.chromium.org/10823217/diff/3015/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager.h:16: ProfileShortcutManager();
This class looks a lot like an interface. As such it's a bit unusual to have a
constructor here, especially since you can never make one of these publicly via
this ctor (you have to use Create() to make the right impl).

I would remove this ctor. If you ever need to initialize state shared by cross
platform subclasses, you can add a protected ctor later.

http://codereview.chromium.org/10823217/diff/3015/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager.h:17:
~ProfileShortcutManager();
Constructors and destructors should be listed before the static methods above.

[Halli, 2012-08-15 18:13:53]

http://codereview.chromium.org/10823217/diff/3015/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager.h (right):

http://codereview.chromium.org/10823217/diff/3015/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager.h:16: ProfileShortcutManager();
I get the following error when I remove the constructor:
146>browser\profiles\profile_shortcut_manager_win.cc(172): error C2512:
'ProfileShortcutManager' : no appropriate default constructor available

On 2012/08/14 22:43:19, Ben Goodger (Google) wrote:
> This class looks a lot like an interface. As such it's a bit unusual to have a
> constructor here, especially since you can never make one of these publicly
via
> this ctor (you have to use Create() to make the right impl).
> 
> I would remove this ctor. If you ever need to initialize state shared by cross
> platform subclasses, you can add a protected ctor later.

http://codereview.chromium.org/10823217/diff/3015/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager.h:17:
~ProfileShortcutManager();
On 2012/08/14 22:43:19, Ben Goodger (Google) wrote:
> Constructors and destructors should be listed before the static methods above.

Done.

[Ben Goodger (Google), 2012-08-15 18:18:49]

Weird (re: the error). That shouldn't be necessary (or at least isn't in the
various other interfaces we have that do this sans ctor).

LGTM.

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:123:
~ProfileShortcutManagerWin();
Any time you have any virtual methods, your destructor should be virtual.

[SteveT, 2012-08-15 18:29:41]

Drive by 2!

Various formatting things but looking pretty good...

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main_win.cc (left):

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main_win.cc:138: VLOG(1) << "Failed to delete
desktop profiles shortcuts.";
Wait, so what's our cleanup plan now that this gone?

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/ava...
File chrome/browser/profiles/avatar_menu_model.cc (right):

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/ava...
chrome/browser/profiles/avatar_menu_model.cc:107: // instead of opening a dialog
for creating a profile
nit: profile -> profile.

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/ava...
chrome/browser/profiles/avatar_menu_model.cc:108: // Remove on completion of BUG
134904
nit: 134904 -> 134904.

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:272:
profile_shortcut_manager_.reset(ProfileShortcutManager::Create());
Should be 2 space indent after the if.

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:457: if (profile_shortcut_manager_ !=
NULL && !name.empty() &&
nit:
if (profile_shortcut_manager_ != NULL && ...
->
if (profile_shortcut_manager_.get() && ...

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:954: if (profile_shortcut_manager_ !=
NULL)
nit:
if (profile_shortcut_manager_ != NULL)
->
if (profile_shortcut_manager_.get())

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:955:
profile_shortcut_manager_->DeleteChromeDesktopShortcut(profile_dir);
Two space indent after if.

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager_unittest.cc:119: CommandLine* cl =
CommandLine::ForCurrentProcess();
Thank you :)

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:120: class
ProfileShortcutManagerWin : public ProfileShortcutManager {
Indentation is a bit funky in this class declaration. public and private should
be indented one space. Everything else should start at 2 spaces.

http://codereview.chromium.org/10823217/diff/7027/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/10823217/diff/7027/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:1064: // Enables the windows profile desktop
shortcuts feature.
nit: windows -> Windows

[Halli, 2012-08-15 19:29:59]

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main_win.cc (left):

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main_win.cc:138: VLOG(1) << "Failed to delete
desktop profiles shortcuts.";
Different CL. I'll make a note.
On 2012/08/15 18:29:41, SteveT wrote:
> Wait, so what's our cleanup plan now that this gone?

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/ava...
File chrome/browser/profiles/avatar_menu_model.cc (right):

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/ava...
chrome/browser/profiles/avatar_menu_model.cc:107: // instead of opening a dialog
for creating a profile
On 2012/08/15 18:29:41, SteveT wrote:
> nit: profile -> profile.

Done.

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/ava...
chrome/browser/profiles/avatar_menu_model.cc:108: // Remove on completion of BUG
134904
On 2012/08/15 18:29:41, SteveT wrote:
> nit: 134904 -> 134904.

Done.

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:272:
profile_shortcut_manager_.reset(ProfileShortcutManager::Create());
On 2012/08/15 18:29:41, SteveT wrote:
> Should be 2 space indent after the if.

Done.

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:457: if (profile_shortcut_manager_ !=
NULL && !name.empty() &&
On 2012/08/15 18:29:41, SteveT wrote:
> nit:
> if (profile_shortcut_manager_ != NULL && ...
> ->
> if (profile_shortcut_manager_.get() && ...

Done.

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:954: if (profile_shortcut_manager_ !=
NULL)
On 2012/08/15 18:29:41, SteveT wrote:
> nit:
> if (profile_shortcut_manager_ != NULL)
> ->
> if (profile_shortcut_manager_.get())

Done.

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:955:
profile_shortcut_manager_->DeleteChromeDesktopShortcut(profile_dir);
On 2012/08/15 18:29:41, SteveT wrote:
> Two space indent after if.

Done.

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager_unittest.cc:119: CommandLine* cl =
CommandLine::ForCurrentProcess();
(:
On 2012/08/15 18:29:41, SteveT wrote:
> Thank you :)

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:120: class
ProfileShortcutManagerWin : public ProfileShortcutManager {
On 2012/08/15 18:29:41, SteveT wrote:
> Indentation is a bit funky in this class declaration. public and private
should
> be indented one space. Everything else should start at 2 spaces.

Done.

http://codereview.chromium.org/10823217/diff/7027/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:123:
~ProfileShortcutManagerWin();
On 2012/08/15 18:18:49, Ben Goodger (Google) wrote:
> Any time you have any virtual methods, your destructor should be virtual.

Done.

http://codereview.chromium.org/10823217/diff/7027/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/10823217/diff/7027/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:1064: // Enables the windows profile desktop
shortcuts feature.
On 2012/08/15 18:29:41, SteveT wrote:
> nit: windows -> Windows

Done.

[SteveT, 2012-08-15 19:49:31]

lgtm after you address my comment inline (which might apply to a few other
places where I mentioned the similar comment).

http://codereview.chromium.org/10823217/diff/15019/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/10823217/diff/15019/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_manager.cc:272:
profile_shortcut_manager_.reset(ProfileShortcutManager::Create());
Oops - my bad. I didn't describe this properly. I meant that lines within an IF
block should be indented 2 spaces, not 4.

So the above should be something like...

if (ProfileShortcutManager::IsFeatureEnabled())
  profile_shortcut_manager_.reset(...

There may have been some other places where I mentioned this.

[Halli, 2012-08-15 19:51:57]

http://codereview.chromium.org/10823217/diff/15019/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/10823217/diff/15019/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_manager.cc:272:
profile_shortcut_manager_.reset(ProfileShortcutManager::Create());
On 2012/08/15 19:49:31, SteveT wrote:
> Oops - my bad. I didn't describe this properly. I meant that lines within an
IF
> block should be indented 2 spaces, not 4.
> 
> So the above should be something like...
> 
> if (ProfileShortcutManager::IsFeatureEnabled())
>   profile_shortcut_manager_.reset(...
> 
> There may have been some other places where I mentioned this.

Done.

[commit-bot: I haz the power, 2012-08-15 19:52:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hallielaine@chromium.org/10823217/38

[commit-bot: I haz the power, 2012-08-15 21:41:05]

Try job failure for 10823217-38 (retry) on mac for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2012-08-16 19:14:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hallielaine@chromium.org/10823217/8063

[commit-bot: I haz the power, 2012-08-16 21:36:55]

Change committed as 151974