Add a startup check to create profile shortcuts for legacy Chrome profiles.

Add a startup check to create profile shortcuts for legacy Chrome profiles.

We do this by adding a per-profile pref that tracks whether or not a shortcut has been created for it. We check that flag at Profile startup and create a new shortcut if needed (while setting the flag to avoid creating future shortcuts).

BUG=109447
TEST=Start Chrome in v16 or v17 and create a new profile. Update Chrome to the latest version (including this patch) and ensure that starting a window with one of the two original profiles creates a desktop shortcut with that Profile's data. Ensure that if we then delete the shortcut for the profile, but start a new window for the profile, another shortcut is not created.

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

[SteveT, 2012-01-18 19:26:02]

PTAL.

I made the changes roughly as we had discussed - moved the Profile Shortcut
creation calls out of OnProfileAdded and into a new method that can be commonly
called from profile startup.

[sail, 2012-01-18 19:32:51]

https://chromiumcodereview.appspot.com/9253016/diff/1/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.h (right):

https://chromiumcodereview.appspot.com/9253016/diff/1/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.h:255: #if defined(OS_WIN)
instead of using #ifdef here you could instead just add stubs for other
platforms with a NOTREACHED().

Not sure which is the preferred implementation for this class.

https://chromiumcodereview.appspot.com/9253016/diff/1/chrome/browser/profiles...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://chromiumcodereview.appspot.com/9253016/diff/1/chrome/browser/profiles...
chrome/browser/profiles/profile_shortcut_manager_win.cc:216: void
ProfileShortcutManagerWin::AddProfileShortcut(
do you need to check kProfileShortcutCreated here?

https://chromiumcodereview.appspot.com/9253016/diff/1/chrome/browser/profiles...
chrome/browser/profiles/profile_shortcut_manager_win.cc:289: void
ProfileShortcutManagerWin::OnProfileAdded(
can this be removed?

[SteveT, 2012-01-18 19:46:01]

Responses inline, and no changes yet.

https://chromiumcodereview.appspot.com/9253016/diff/1/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.h (right):

https://chromiumcodereview.appspot.com/9253016/diff/1/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.h:255: #if defined(OS_WIN)
I've seen the rest of the ProfileShortcutManagerWin stuff in this class
surrounded by #if defined(OS_WIN), so I believe that's the way to go...

https://chromiumcodereview.appspot.com/9253016/diff/1/chrome/browser/profiles...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://chromiumcodereview.appspot.com/9253016/diff/1/chrome/browser/profiles...
chrome/browser/profiles/profile_shortcut_manager_win.cc:216: void
ProfileShortcutManagerWin::AddProfileShortcut(
This is a good question. I somewhat prefer keeping all the Prefs checking
outside of ProfileShortcutManagerWin and leave it in ProfileManager, where we
touch a lot of other prefs. I feel like this class doesn't need to get involved
in the decision making process, as that logic belongs in the actual Profiles
infrastructure. Adding it here would be extra safe, but also redundant and
couples it a bit more with the Profiles state.

Is that OK?

https://chromiumcodereview.appspot.com/9253016/diff/1/chrome/browser/profiles...
chrome/browser/profiles/profile_shortcut_manager_win.cc:289: void
ProfileShortcutManagerWin::OnProfileAdded(
Unfortunately this comes from ProfileInfoCacheObserver, which declared
OnProfileAdded as abstract, so we have to give it an empty body here.

[sail, 2012-01-18 19:53:35]

https://chromiumcodereview.appspot.com/9253016/diff/1/chrome/browser/profiles...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://chromiumcodereview.appspot.com/9253016/diff/1/chrome/browser/profiles...
chrome/browser/profiles/profile_shortcut_manager_win.cc:216: void
ProfileShortcutManagerWin::AddProfileShortcut(
On 2012/01/18 19:46:01, SteveT wrote:
> This is a good question. I somewhat prefer keeping all the Prefs checking
> outside of ProfileShortcutManagerWin and leave it in ProfileManager, where we
> touch a lot of other prefs. I feel like this class doesn't need to get
involved
> in the decision making process, as that logic belongs in the actual Profiles
> infrastructure. Adding it here would be extra safe, but also redundant and
> couples it a bit more with the Profiles state.
> 
> Is that OK?

I'm a bit confused. You added a new 'kProfileShortcutCreated' pref but you're
not using it anywhere. Were you planning to use it in this CL?

https://chromiumcodereview.appspot.com/9253016/diff/1/chrome/browser/profiles...
chrome/browser/profiles/profile_shortcut_manager_win.cc:289: void
ProfileShortcutManagerWin::OnProfileAdded(
On 2012/01/18 19:46:01, SteveT wrote:
> Unfortunately this comes from ProfileInfoCacheObserver, which declared
> OnProfileAdded as abstract, so we have to give it an empty body here.

Area any other observers using this? If not then can it be removed?

[SteveT, 2012-01-18 20:17:40]

Forgot profile_manager.cc, which is now uploaded. Sorry about that! This should
make a lot more sense.

https://chromiumcodereview.appspot.com/9253016/diff/1/chrome/browser/profiles...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://chromiumcodereview.appspot.com/9253016/diff/1/chrome/browser/profiles...
chrome/browser/profiles/profile_shortcut_manager_win.cc:216: void
ProfileShortcutManagerWin::AddProfileShortcut(
Oh my gosh, I totally forgot to upload profile_manager.cc, which is the crux of
this whole thing. kProfileShortcutCreated is checked and set there.

https://chromiumcodereview.appspot.com/9253016/diff/1/chrome/browser/profiles...
chrome/browser/profiles/profile_shortcut_manager_win.cc:289: void
ProfileShortcutManagerWin::OnProfileAdded(
Yes, BackgroundModeManager is also a ProfileInfoCacheObserver, so we can't
remove OnProfileAdded completely.

[sail, 2012-01-18 20:20:05]

LGTM!

Is there anyway to add a unit test for this behavior?

[SteveT, 2012-01-18 20:23:01]

Trying to figure that out while I work out the manual testing... I think that
this is something that we want to verify as part of our functional pyauto tests.
Now that crbug.com/108362 is addressed, this can be possible soon. I'll make
sure TestEng knows about this particular test case.

[sail, 2012-01-18 20:26:43]

On 2012/01/18 20:23:01, SteveT wrote:
> Trying to figure that out while I work out the manual testing... I think that
> this is something that we want to verify as part of our functional pyauto
tests.
> Now that crbug.com/108362 is addressed, this can be possible soon. I'll make
> sure TestEng knows about this particular test case.

Cool!

[commit-bot: I haz the power, 2012-01-19 15:23:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/9253016/9

[commit-bot: I haz the power, 2012-01-19 17:23:13]

Change committed as 118316