Create profile .ico file on profile creation

Create profile .ico file on profile creation

Previously the icon was only created on desktop shortcut creation/update.
This is a stepping stone to getting pinned profiles to work.

BUG=167984
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212569

[calamity, 2013-04-26 09:06:59]

Hey,

I have no idea where to do the "create .ico files for profiles that don't
already have them on update" part of the bug.

[Alexei Svitkine (slow), 2013-04-26 15:47:13]

Do you need to update the tests?

Either way, can you add a test to ensure deleting a profile's shortcuts (using
the API) keeps the file on disk.

As for how to upgrade existing profiles, perhaps we can have a per-profile pref
that mentions the icon was created? Then, when a profile is first loaded, if the
pref is not set (which can be checked on the UI thread), post a task to create
the icon and then return to the UI thread to set the pref?

[Alexei Svitkine (slow), 2013-04-26 21:16:23]

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

https://chromiumcodereview.appspot.com/14137032/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile_shortcut_manager_win.cc:164: LOG(ERROR) <<
"Failed to create icon at " << icon_path.value();
Just make it a NOTREACHED()?

Also, please add {}'s to the if.

https://chromiumcodereview.appspot.com/14137032/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile_shortcut_manager_win.cc:353: if
(file_util::PathExists(shortcut_icon))
When would this be the case?

Wouldn't we want to re-create the icon if it doesn't exist?

https://chromiumcodereview.appspot.com/14137032/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile_shortcut_manager_win.cc:437:
file_util::Delete(icon_path, false);
I think you need to remove this line, else when someone deletes their shortcut
(from the chrome://setting user editor), it will delete the icon.

Can you add a test that verifies this doesn't happen?

https://chromiumcodereview.appspot.com/14137032/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile_shortcut_manager_win.cc:620:
CreateOrUpdateShortcutIconForProfileAtPath(profile_path);
Add a comment mentioning that this is needed to "unbadge" the icon.

https://chromiumcodereview.appspot.com/14137032/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile_shortcut_manager_win.cc:642:
CreateOrUpdateShortcutsForProfileAtPath(profile_path, UPDATE_EXISTING_ONLY,
Is this call necessary if we're already calling
CreateOrUpdateShortcutIconForProfileAtPath() to update the image with the
avatar? (please test this)

If yes, please add a comment why.

[calamity, 2013-04-30 06:45:41]

Hey, I decided to roll back the refactor because it was making it slightly
trickier to recreate the icon when making the shortcut. Also, I was a little
worried that there would be concurrency issues with posting 2 tasks that were
sort of interdependent.

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

https://chromiumcodereview.appspot.com/14137032/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile_shortcut_manager_win.cc:164: LOG(ERROR) <<
"Failed to create icon at " << icon_path.value();
On 2013/04/26 21:16:23, Alexei Svitkine wrote:
> Just make it a NOTREACHED()?
> 
> Also, please add {}'s to the if.

Done.

https://chromiumcodereview.appspot.com/14137032/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile_shortcut_manager_win.cc:353: if
(file_util::PathExists(shortcut_icon))
On 2013/04/26 21:16:23, Alexei Svitkine wrote:
> When would this be the case?
> 
> Wouldn't we want to re-create the icon if it doesn't exist?

Rolled the 2 functions back together and created the icon at the top of this
function.

https://chromiumcodereview.appspot.com/14137032/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile_shortcut_manager_win.cc:437:
file_util::Delete(icon_path, false);
On 2013/04/26 21:16:23, Alexei Svitkine wrote:
> I think you need to remove this line, else when someone deletes their shortcut
> (from the chrome://setting user editor), it will delete the icon.
> 
> Can you add a test that verifies this doesn't happen?

Done.

The change I've made to the tests assert that the icon file remains as a
post-condition at cleanup of each test. Do you want an explicit test for this?

https://chromiumcodereview.appspot.com/14137032/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile_shortcut_manager_win.cc:620:
CreateOrUpdateShortcutIconForProfileAtPath(profile_path);
On 2013/04/26 21:16:23, Alexei Svitkine wrote:
> Add a comment mentioning that this is needed to "unbadge" the icon.

Done. I actually want to make an unbadged icon that overwrites the profile icon.
Should I do this in this patch? (It currently crashes on the no badge case, the
changes are above in CreateOrUpdateShortcutIconForProfile)

If you're interested, the reason I need an unbadged icon to overwrite the
profile icon is that if the user pins a profile shortcut, that shortcut points
to the profile icon. So we can either make the profile icon an unbadged chrome
icon in the final delete case or we can try to find every profile shortcut and
change the icon to point back at chrome.exe. I think the old code explicitly set
no icon which made it point to chrome.exe by default but only had to deal with
the desktop shortcuts. Let me know which you think is better.

https://chromiumcodereview.appspot.com/14137032/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile_shortcut_manager_win.cc:642:
CreateOrUpdateShortcutsForProfileAtPath(profile_path, UPDATE_EXISTING_ONLY,
On 2013/04/26 21:16:23, Alexei Svitkine wrote:
> Is this call necessary if we're already calling
> CreateOrUpdateShortcutIconForProfileAtPath() to update the image with the
> avatar? (please test this)
> 
> If yes, please add a comment why.

Not updating the shortcut seems to be fine.

[calamity, 2013-05-01 05:13:38]

On 2013/04/30 06:45:41, calamity wrote:
> Hey, I decided to roll back the refactor because it was making it slightly
> trickier to recreate the icon when making the shortcut. Also, I was a little
> worried that there would be concurrency issues with posting 2 tasks that were
> sort of interdependent.
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/1/chrome/browser/profile...
> File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/1/chrome/browser/profile...
> chrome/browser/profiles/profile_shortcut_manager_win.cc:164: LOG(ERROR) <<
> "Failed to create icon at " << icon_path.value();
> On 2013/04/26 21:16:23, Alexei Svitkine wrote:
> > Just make it a NOTREACHED()?
> > 
> > Also, please add {}'s to the if.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/1/chrome/browser/profile...
> chrome/browser/profiles/profile_shortcut_manager_win.cc:353: if
> (file_util::PathExists(shortcut_icon))
> On 2013/04/26 21:16:23, Alexei Svitkine wrote:
> > When would this be the case?
> > 
> > Wouldn't we want to re-create the icon if it doesn't exist?
> 
> Rolled the 2 functions back together and created the icon at the top of this
> function.
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/1/chrome/browser/profile...
> chrome/browser/profiles/profile_shortcut_manager_win.cc:437:
> file_util::Delete(icon_path, false);
> On 2013/04/26 21:16:23, Alexei Svitkine wrote:
> > I think you need to remove this line, else when someone deletes their
shortcut
> > (from the chrome://setting user editor), it will delete the icon.
> > 
> > Can you add a test that verifies this doesn't happen?
> 
> Done.
> 
> The change I've made to the tests assert that the icon file remains as a
> post-condition at cleanup of each test. Do you want an explicit test for this?
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/1/chrome/browser/profile...
> chrome/browser/profiles/profile_shortcut_manager_win.cc:620:
> CreateOrUpdateShortcutIconForProfileAtPath(profile_path);
> On 2013/04/26 21:16:23, Alexei Svitkine wrote:
> > Add a comment mentioning that this is needed to "unbadge" the icon.
> 
> Done. I actually want to make an unbadged icon that overwrites the profile
icon.
> Should I do this in this patch? (It currently crashes on the no badge case,
the
> changes are above in CreateOrUpdateShortcutIconForProfile)
> 
> If you're interested, the reason I need an unbadged icon to overwrite the
> profile icon is that if the user pins a profile shortcut, that shortcut points
> to the profile icon. So we can either make the profile icon an unbadged chrome
> icon in the final delete case or we can try to find every profile shortcut and
> change the icon to point back at chrome.exe. I think the old code explicitly
set
> no icon which made it point to chrome.exe by default but only had to deal with
> the desktop shortcuts. Let me know which you think is better.
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/1/chrome/browser/profile...
> chrome/browser/profiles/profile_shortcut_manager_win.cc:642:
> CreateOrUpdateShortcutsForProfileAtPath(profile_path, UPDATE_EXISTING_ONLY,
> On 2013/04/26 21:16:23, Alexei Svitkine wrote:
> > Is this call necessary if we're already calling
> > CreateOrUpdateShortcutIconForProfileAtPath() to update the image with the
> > avatar? (please test this)
> > 
> > If yes, please add a comment why.
> 
> Not updating the shortcut seems to be fine.

Also, with the icon creation on update, where would that preference be checked?
I can only think of doing it in browser init? Is that the right place?

[Alexei Svitkine (slow), 2013-05-01 05:39:17]

Sorry, I haven't had the chance to look at the latest incarnation of the
CL, yet.


On Wed, May 1, 2013 at 1:13 AM, <calamity@chromium.org> wrote:
>
> Also, with the icon creation on update, where would that preference be
> checked?
> I can only think of doing it in browser init? Is that the right place?
>

I think it should be done when the profile is loaded, per-profile.



>
>
https://chromiumcodereview.**appspot.com/14137032/<https://chromiumcodereview...
>

[Alexei Svitkine (slow), 2013-05-01 15:27:30]

Again, thanks for working on this. Here are some comments on your latest patch.

https://codereview.chromium.org/14137032/diff/1/chrome/browser/profiles/profi...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/1/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_shortcut_manager_win.cc:620:
CreateOrUpdateShortcutIconForProfileAtPath(profile_path);
On 2013/04/30 06:45:41, calamity wrote:
> On 2013/04/26 21:16:23, Alexei Svitkine wrote:
> > Add a comment mentioning that this is needed to "unbadge" the icon.
> 
> Done. I actually want to make an unbadged icon that overwrites the profile
icon.
> Should I do this in this patch? (It currently crashes on the no badge case,
the
> changes are above in CreateOrUpdateShortcutIconForProfile)
> 
> If you're interested, the reason I need an unbadged icon to overwrite the
> profile icon is that if the user pins a profile shortcut, that shortcut points
> to the profile icon. So we can either make the profile icon an unbadged chrome
> icon in the final delete case or we can try to find every profile shortcut and
> change the icon to point back at chrome.exe. I think the old code explicitly
set
> no icon which made it point to chrome.exe by default but only had to deal with
> the desktop shortcuts. Let me know which you think is better.

I think it's fine to create an un-badged icon for the profile, in that case. At
the same time, I think we should continue to create "vanilla" shortcuts when
deleting down to 1 profile. The unbadged icon would just be there for the case
of pinned shortcuts or shortcuts in other locations that we don't look for as
well as for the window decoration (which will be associated with the icon now).

https://codereview.chromium.org/14137032/diff/12001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/12001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.cc:142: // given bitmaps
are empty, an unbadged icon is created.
Please add a TODO that ideally we'd just want to copy the app icon verbatim from
the exe's resources in this case.

https://codereview.chromium.org/14137032/diff/12001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.cc:147:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
Add: DCHECK_EQ(avatar_bitmap_1x.empty(), avatar_bitmap_1x.empty());

https://codereview.chromium.org/14137032/diff/12001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.cc:152: SkBitmap
badged_bitmap;
How about:

SkBitmap badged_bitmap = avatar_bitmap_1x.empty() ? *app_icon_bitmap :
                                            BadgeIcon(*app_icon_bitmap,
avatar_bitmap_1x, 1);

Same thing for large_badged_bitmap.

Then you can avoid the if later on.

https://codereview.chromium.org/14137032/diff/12001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.cc:174: NOTREACHED();
Re-add the return base::FilePath() to propagate the failure, just in case.

https://codereview.chromium.org/14137032/diff/12001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.cc:340: NOTREACHED();
Add a return.

https://codereview.chromium.org/14137032/diff/12001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.cc:342: if (create_mode ==
ProfileShortcutManagerWin::CREATE_ICON_ONLY) {
No need for {}'s.

https://codereview.chromium.org/14137032/diff/12001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

https://codereview.chromium.org/14137032/diff/12001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.h:37: // Specifies whether
a new shortcut should be created if none exist.
Update the comment.

[calamity, 2013-05-03 04:40:28]

Hey,

I've also added the profile icon creation on first run past this change as
discussed.

https://chromiumcodereview.appspot.com/14137032/diff/12001/chrome/browser/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/12001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:142: // given bitmaps
are empty, an unbadged icon is created.
On 2013/05/01 15:27:30, Alexei Svitkine wrote:
> Please add a TODO that ideally we'd just want to copy the app icon verbatim
from
> the exe's resources in this case.

Done.

https://chromiumcodereview.appspot.com/14137032/diff/12001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:147:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
On 2013/05/01 15:27:30, Alexei Svitkine wrote:
> Add: DCHECK_EQ(avatar_bitmap_1x.empty(), avatar_bitmap_1x.empty());

I'm assuming this is to facilitate the change below. If so, this may be voided.

https://chromiumcodereview.appspot.com/14137032/diff/12001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:152: SkBitmap
badged_bitmap;
I think that making these changes decreases the robustness of this icon
creation? What should happen if only the 2x bitmap is available? The ImageFamily
icon creation should handle only having one size of icon to work with.

I've refactored this a little so the logic is clearer. Let me know if this is
sufficient.

On 2013/05/01 15:27:30, Alexei Svitkine wrote:
> How about:
> 
> SkBitmap badged_bitmap = avatar_bitmap_1x.empty() ? *app_icon_bitmap :
>                                             BadgeIcon(*app_icon_bitmap,
> avatar_bitmap_1x, 1);
> 
> Same thing for large_badged_bitmap.
> 
> Then you can avoid the if later on.

https://chromiumcodereview.appspot.com/14137032/diff/12001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:174: NOTREACHED();
On 2013/05/01 15:27:30, Alexei Svitkine wrote:
> Re-add the return base::FilePath() to propagate the failure, just in case.

Done.

https://chromiumcodereview.appspot.com/14137032/diff/12001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:340: NOTREACHED();
On 2013/05/01 15:27:30, Alexei Svitkine wrote:
> Add a return.

Done.

https://chromiumcodereview.appspot.com/14137032/diff/12001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:342: if (create_mode ==
ProfileShortcutManagerWin::CREATE_ICON_ONLY) {
On 2013/05/01 15:27:30, Alexei Svitkine wrote:
> No need for {}'s.

Done.

https://chromiumcodereview.appspot.com/14137032/diff/12001/chrome/browser/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

https://chromiumcodereview.appspot.com/14137032/diff/12001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.h:37: // Specifies whether
a new shortcut should be created if none exist.
On 2013/05/01 15:27:30, Alexei Svitkine wrote:
> Update the comment.

Done.

[Alexei Svitkine (slow), 2013-05-03 17:20:47]

Thanks, a few more comments your way.

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:757:
profile_shortcut_manager->CreateProfileIcon(GetPath());
Please check that profile_shorcut_manager is not NULL. It can be NULL if
ProfileShortcutManager::IsFeatureEnabled() is false.

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:758:
prefs_->SetBoolean(prefs::kProfileIconCreated, true);
I think this should be set in a callback as a result of CreateProfileIcon
succeeding.

Which probably means that CreateProfileIcon should take a callback, similar to
what HasProfileShortcuts() does.

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.cc:144: // TODO(calamity):
ideally we'd just copy the app icon verbatim from the exe's
Nit: capitalize 'ideally'

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.cc:184:
SHChangeNotify(SHCNE_ASSOCCHANGED, SHCNF_IDLIST | SHCNF_FLUSHNOWAIT,
Add a comment mentioning why this is necessary. (To update any existing
shortcuts using the icon.)

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.cc:343: base::FilePath
shortcut_icon =
Add a TODO that we should only update the icon if it doesn't exist or actually
needs to be updated (image change)?

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.cc:440: // corresponding
icon file. If |ensure_shortcuts_remain| is true, then a regular
Update this comment.

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.h:56: const base::FilePath&
profile_path) OVERRIDE;
Please add a unit test for this. (i.e. create a profile, ensure that an icon got
created, then delete it and call this function and ensure it got created again.)

[calamity, 2013-05-06 03:40:17]

Hey,

Is there any reason not to always check if the profile icon exists on startup
and to create it if it doesn't? Since this will essentially be hooked up to the
taskbar icon, we don't want a situation where say, a write fails on unclean
shutdown and we lose the taskbar icon for chrome.

[Alexei Svitkine (slow), 2013-05-06 04:51:09]

On Sun, May 5, 2013 at 11:40 PM, <calamity@chromium.org> wrote:

> Hey,
>
> Is there any reason not to always check if the profile icon exists on
> startup
> and to create it if it doesn't? Since this will essentially be hooked up
> to the
> taskbar icon, we don't want a situation where say, a write fails on unclean
> shutdown and we lose the taskbar icon for chrome.
>

Hmm, that's a good point. My only worry is that it may slow down startup.

Are you proposing to do this synchronously on startup for the first profile
being loaded (and then synchronously on the FILE thread for each new
profile that gets loaded), or are you thinking of posting a task on startup
asynchronously?

I can see pros and cons to both approaches.


Could we perhaps also solve it by specifically detected the "write fail on
unclean shutdown" case, by for e.g. setting a pref before the write to say
"write in progress" and then setting it back after the write completes?

Though, I'm not convinced that will work, since local state is only written
every 10s iirc, so the pref may be stale too...

Actually, now that I think about it, I think the right approach is to
use base/files/important_file_writer.h here, which should give a better
guarantee against the file not being written. I guess this will require
changes to icon_util.cc, though.

[calamity, 2013-05-07 07:47:20]

I was thinking of doing the icon stuff synchronously. I don't think we'd want a
blank taskbar icon for perhaps several seconds which could happen with delayed
creation. If the icon is already there, checking for its existence shouldn't
delay startup by too long? I really don't know enough to make an informed
decision on this.

important_file_writer looks good. It has a synchronous API so modifying
icon_util.cc to use it shouldn't be too difficult.

[gab, 2013-05-07 11:33:43]

On 2013/05/07 07:47:20, calamity wrote:
> I was thinking of doing the icon stuff synchronously. I don't think we'd want
a
> blank taskbar icon for perhaps several seconds which could happen with delayed
> creation. If the icon is already there, checking for its existence shouldn't
> delay startup by too long? I really don't know enough to make an informed
> decision on this.
> 
> important_file_writer looks good. It has a synchronous API so modifying
> icon_util.cc to use it shouldn't be too difficult.

If doing it asynchronously means you have a blank icon for several seconds: that
also means that doing it synchronously delays startup by several seconds if it
needs to be created, no? If so, that's not good. It's preferable to have a blank
icon for a few seconds (once at profile creation -- doesn't the icon default to
the unbadged chrome.exe icon anyways when one isn't provided at first?), then to
synchronously delay Chrome first run/profile creation.

Many things in Chrome are asynchronous although if they were to happen *visibly
out of sync* it would look weird, but it's always preferable in case of jank to
have a feature look weird than have the whole browser get caught up.

Cheers!
Gab

[gab, 2013-05-07 12:38:39]

Some comments below.

In general I don't like that this be tied to the profile shortcuts feature which
can still be disabled. Badged taskbar icons worked way before profile shortcuts.
I agree however that the profile shortcuts code is probably the best place to
put this new logic... Can we remove the logic that allows disabling of profile
shortcuts, making this feature a permanent one? Otherwise I'd at least like the
approval of a PM (mgaba@?) to tie a long existing functionality to this new,
still disableable, feature.

Cheers!
Gab

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:757:
profile_shortcut_manager->CreateProfileIcon(GetPath());
On 2013/05/03 17:20:47, Alexei Svitkine wrote:
> Please check that profile_shorcut_manager is not NULL. It can be NULL if
> ProfileShortcutManager::IsFeatureEnabled() is false.

So this means that profile taskbar icons (pinned or not) will now depend on
ProfileShortcuts being enabled? That sounds wrong to me (i.e., taskbar icons
have always been badged, way before profile shortcuts came around). I'm only
comfortable with this if the code to disable profile shortcuts is removed (I'm
not sure what the team's policy is to remove feature flags, but it feels like
once profile shortcuts ship to stable we could remove this? i.e., was the flag
only to be able to turn it on/off for individual milestones during
development?).

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:78:
ASSERT_TRUE(file_util::Delete(icon_path, false));
Use base::win::TaskbarUnpinShortcutLink(icon_path.c_str()) instead of
file_util::Delete(...) to delete pinned-to-taskbar shortcuts (otherwise the
Windows shell gets confused and leaves a blank icon in the taskbar).

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.cc:140: // badging the
browser distribution icon with the profile avatar.
Why is "browser distribution" mentioned in this comment? It isn't used in this
method at all...

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.cc:184:
SHChangeNotify(SHCNE_ASSOCCHANGED, SHCNF_IDLIST | SHCNF_FLUSHNOWAIT,
FYI, I have had issues with SHCNF_FLUSHNOWAIT in the past
(http://crrev.com/145987) where my guess it that: the flush would happen before
the Windows Shell was even notified of the shortcut creation (or maybe in my
specific case before the new PropertyStore was flushed out; which would result
in the shortcut appearing, but without the properties being written to the
PropertyStore...).

Perhaps notifying the shell first with SHChangeNotify(SHCNE_CREATE, SHCNF_PATH,
icon_path.c_str(), NULL) is safer here (this only makes sense if
CreateIconFileFromImageFamily() is synchronous, is it?)? (Or with
SHCNE_UPDATEITEM if the icon is being overwritten rather than created).

An SHCNE_ASSOCCHANGED notification should only be required imo if Chrome's
window is already telling the taskbar to use this icon (i.e., before it might
even exist); is that the case? Otherwise it's preferable to shy away from
SHCNE_ASSOCCHANGED as it results in a full desktop/taskbar icons flash.

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.cc:444: bool
ensure_shortcuts_remain) {
nit: Indent.

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.cc:551: return
profile_path.AppendASCII(profiles::internal::kProfileIconFileName);
nit: Remove profiles::internal:: namespace as this code is already in it.

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.cc:554: const char
kProfileIconFileName[] = "Google Profile.ico";
nit: constants should remain at the top of the namespace

Also, since this is constant no longer needs to be accessed from outside of this
file, move it up into the unnamed namespace of this file.

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.cc:664:
CreateOrUpdateShortcutsForProfileAtPath(profile_path,
Why was this un-inlined? |profile_path| only seems to be used once still...

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.cc:684:
CreateOrUpdateShortcutsForProfileAtPath(profile_path, CREATE_ICON_ONLY,
Should still update the desktop shortcut or will updating the icon and notifying
the shell result in an updated shortcut?

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.h:37: // Specifies whether
the only existing shortcuts should be updated, a new
s/the only/only the

https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.h:39: // should be updated.
s/updated./created in the profile directory.

[Alexei Svitkine (slow), 2013-05-07 13:19:05]

Gab:

There's no feature flag for profile shortcuts. It's only disabled if:

1. you're ChromeFrame (or some other browser distribution that doesn't
support shortcuts)
2. you're the app v2 applist process (i.e. not the Chrome browser)
3. you specified --user-data-dir

Only the 3rd one is really problematic, though it's kind of an edge case.

One way we can work around this is by making the new method static, so that
a ProfileShortcutManager instance doesn't need to exist to use it.


On Tue, May 7, 2013 at 8:38 AM, <gab@chromium.org> wrote:

> Some comments below.
>
> In general I don't like that this be tied to the profile shortcuts feature
> which
> can still be disabled. Badged taskbar icons worked way before profile
> shortcuts.
> I agree however that the profile shortcuts code is probably the best place
> to
> put this new logic... Can we remove the logic that allows disabling of
> profile
> shortcuts, making this feature a permanent one? Otherwise I'd at least
> like the
> approval of a PM (mgaba@?) to tie a long existing functionality to this
> new,
> still disableable, feature.
>
> Cheers!
> Gab
>
>
>
> https://codereview.chromium.**org/14137032/diff/28001/**
>
chrome/browser/profiles/**profile_impl.cc<https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/profile_impl.cc>
> File chrome/browser/profiles/**profile_impl.cc (right):
>
> https://codereview.chromium.**org/14137032/diff/28001/**
>
chrome/browser/profiles/**profile_impl.cc#newcode757<https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/profile_impl.cc#newcode757>
> chrome/browser/profiles/**profile_impl.cc:757:
> profile_shortcut_manager->**CreateProfileIcon(GetPath());
> On 2013/05/03 17:20:47, Alexei Svitkine wrote:
>
>> Please check that profile_shorcut_manager is not NULL. It can be NULL
>>
> if
>
>> ProfileShortcutManager::**IsFeatureEnabled() is false.
>>
>
> So this means that profile taskbar icons (pinned or not) will now depend
> on ProfileShortcuts being enabled? That sounds wrong to me (i.e.,
> taskbar icons have always been badged, way before profile shortcuts came
> around). I'm only comfortable with this if the code to disable profile
> shortcuts is removed (I'm not sure what the team's policy is to remove
> feature flags, but it feels like once profile shortcuts ship to stable
> we could remove this? i.e., was the flag only to be able to turn it
> on/off for individual milestones during development?).
>
> https://codereview.chromium.**org/14137032/diff/28001/**
>
chrome/browser/profiles/**profile_shortcut_manager_**unittest_win.cc<https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc>
> File chrome/browser/profiles/**profile_shortcut_manager_**unittest_win.cc
> (right):
>
> https://codereview.chromium.**org/14137032/diff/28001/**
> chrome/browser/profiles/**profile_shortcut_manager_**
>
unittest_win.cc#newcode78<https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc#newcode78>
> chrome/browser/profiles/**profile_shortcut_manager_**unittest_win.cc:78:
> ASSERT_TRUE(file_util::Delete(**icon_path, false));
> Use base::win::**TaskbarUnpinShortcutLink(icon_**path.c_str()) instead of
> file_util::Delete(...) to delete pinned-to-taskbar shortcuts (otherwise
> the Windows shell gets confused and leaves a blank icon in the taskbar).
>
>
> https://codereview.chromium.**org/14137032/diff/28001/**
>
chrome/browser/profiles/**profile_shortcut_manager_win.**cc<https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/profile_shortcut_manager_win.cc>
> File chrome/browser/profiles/**profile_shortcut_manager_win.**cc (right):
>
> https://codereview.chromium.**org/14137032/diff/28001/**
>
chrome/browser/profiles/**profile_shortcut_manager_win.**cc#newcode140<https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/profile_shortcut_manager_win.cc#newcode140>
> chrome/browser/profiles/**profile_shortcut_manager_win.**cc:140: //
> badging
>
> the browser distribution icon with the profile avatar.
> Why is "browser distribution" mentioned in this comment? It isn't used
> in this method at all...
>
>
> https://codereview.chromium.**org/14137032/diff/28001/**
>
chrome/browser/profiles/**profile_shortcut_manager_win.**cc#newcode184<https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/profile_shortcut_manager_win.cc#newcode184>
> chrome/browser/profiles/**profile_shortcut_manager_win.**cc:184:
> SHChangeNotify(SHCNE_**ASSOCCHANGED, SHCNF_IDLIST | SHCNF_FLUSHNOWAIT,
> FYI, I have had issues with SHCNF_FLUSHNOWAIT in the past
> (http://crrev.com/145987) where my guess it that: the flush would happen
> before the Windows Shell was even notified of the shortcut creation (or
> maybe in my specific case before the new PropertyStore was flushed out;
> which would result in the shortcut appearing, but without the properties
> being written to the PropertyStore...).
>
> Perhaps notifying the shell first with SHChangeNotify(SHCNE_CREATE,
> SHCNF_PATH, icon_path.c_str(), NULL) is safer here (this only makes
> sense if CreateIconFileFromImageFamily(**) is synchronous, is it?)? (Or
> with SHCNE_UPDATEITEM if the icon is being overwritten rather than
> created).
>
> An SHCNE_ASSOCCHANGED notification should only be required imo if
> Chrome's window is already telling the taskbar to use this icon (i.e.,
> before it might even exist); is that the case? Otherwise it's preferable
> to shy away from SHCNE_ASSOCCHANGED as it results in a full
> desktop/taskbar icons flash.
>
> https://codereview.chromium.**org/14137032/diff/28001/**
>
chrome/browser/profiles/**profile_shortcut_manager_win.**cc#newcode444<https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/profile_shortcut_manager_win.cc#newcode444>
> chrome/browser/profiles/**profile_shortcut_manager_win.**cc:444: bool
> ensure_shortcuts_remain) {
> nit: Indent.
>
> https://codereview.chromium.**org/14137032/diff/28001/**
>
chrome/browser/profiles/**profile_shortcut_manager_win.**cc#newcode551<https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/profile_shortcut_manager_win.cc#newcode551>
> chrome/browser/profiles/**profile_shortcut_manager_win.**cc:551: return
> profile_path.AppendASCII(**profiles::internal::**kProfileIconFileName);
> nit: Remove profiles::internal:: namespace as this code is already in
> it.
>
> https://codereview.chromium.**org/14137032/diff/28001/**
>
chrome/browser/profiles/**profile_shortcut_manager_win.**cc#newcode554<https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/profile_shortcut_manager_win.cc#newcode554>
> chrome/browser/profiles/**profile_shortcut_manager_win.**cc:554: const
> char
> kProfileIconFileName[] = "Google Profile.ico";
> nit: constants should remain at the top of the namespace
>
> Also, since this is constant no longer needs to be accessed from outside
> of this file, move it up into the unnamed namespace of this file.
>
> https://codereview.chromium.**org/14137032/diff/28001/**
>
chrome/browser/profiles/**profile_shortcut_manager_win.**cc#newcode664<https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/profile_shortcut_manager_win.cc#newcode664>
> chrome/browser/profiles/**profile_shortcut_manager_win.**cc:664:
> CreateOrUpdateShortcutsForProf**ileAtPath(profile_path,
> Why was this un-inlined? |profile_path| only seems to be used once
> still...
>
> https://codereview.chromium.**org/14137032/diff/28001/**
>
chrome/browser/profiles/**profile_shortcut_manager_win.**cc#newcode684<https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/profile_shortcut_manager_win.cc#newcode684>
> chrome/browser/profiles/**profile_shortcut_manager_win.**cc:684:
> CreateOrUpdateShortcutsForProf**ileAtPath(profile_path, CREATE_ICON_ONLY,
> Should still update the desktop shortcut or will updating the icon and
> notifying the shell result in an updated shortcut?
>
>
> https://codereview.chromium.**org/14137032/diff/28001/**
>
chrome/browser/profiles/**profile_shortcut_manager_win.h<https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/profile_shortcut_manager_win.h>
> File chrome/browser/profiles/**profile_shortcut_manager_win.h (right):
>
> https://codereview.chromium.**org/14137032/diff/28001/**
>
chrome/browser/profiles/**profile_shortcut_manager_win.**h#newcode37<https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/profile_shortcut_manager_win.h#newcode37>
> chrome/browser/profiles/**profile_shortcut_manager_win.**h:37: //
> Specifies
> whether the only existing shortcuts should be updated, a new
> s/the only/only the
>
> https://codereview.chromium.**org/14137032/diff/28001/**
>
chrome/browser/profiles/**profile_shortcut_manager_win.**h#newcode39<https://codereview.chromium.org/14137032/diff/28001/chrome/browser/profiles/profile_shortcut_manager_win.h#newcode39>
> chrome/browser/profiles/**profile_shortcut_manager_win.**h:39: // should
> be
> updated.
> s/updated./created in the profile directory.
>
>
https://codereview.chromium.**org/14137032/<https://codereview.chromium.org/1...
>

[Alexei Svitkine (slow), 2013-05-07 13:35:18]

On 2013/05/07 07:47:20, calamity wrote:
> I was thinking of doing the icon stuff synchronously. I don't think we'd want
a
> blank taskbar icon for perhaps several seconds which could happen with delayed
> creation. If the icon is already there, checking for its existence shouldn't
> delay startup by too long? I really don't know enough to make an informed
> decision on this.

I think you can do it async and as long as you don't set the icon based on the
pref state, it will be the unbadged Chrome icon, rather than a blank one. (Then
you'd have to monitor the pref for when it changes, to update it.)

> 
> important_file_writer looks good. It has a synchronous API so modifying
> icon_util.cc to use it shouldn't be too difficult.

I was thinking it may be nicer to make icon_util.cc support returning the .ico
file contents as raw bytes instead of writing them to file, so that the
flexibility of what to do with those bytes is up to the caller.

[calamity, 2013-05-08 08:15:41]

Summary:
-rework
-moved icon hashing from web_app_win to icon_util
-made windows .ico file creation use important_file_handler

Hey, benwells@ mentioned that the v2 app list disabling these profile shortcuts
is wrong. In particular, if you launch the app list and then launch chrome from
there, profile shortcuts will currently be disabled. I've filed another bug for
this. http://crbug.com/238942.

Can we just remove the app list condition from IsFeatureEnabled()?



It feels like returning the raw bytes of an image would only be useful if the
caller either wanted them explicitly for some reason or didn't want to write the
file written as an important file and I'm not sure when we would want that since
icon files are probably always "important".

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:757:
profile_shortcut_manager->CreateProfileIcon(GetPath());
On 2013/05/07 12:38:39, gab wrote:
> On 2013/05/03 17:20:47, Alexei Svitkine wrote:
> > Please check that profile_shorcut_manager is not NULL. It can be NULL if
> > ProfileShortcutManager::IsFeatureEnabled() is false.
> 
> So this means that profile taskbar icons (pinned or not) will now depend on
> ProfileShortcuts being enabled? That sounds wrong to me (i.e., taskbar icons
> have always been badged, way before profile shortcuts came around). I'm only
> comfortable with this if the code to disable profile shortcuts is removed (I'm
> not sure what the team's policy is to remove feature flags, but it feels like
> once profile shortcuts ship to stable we could remove this? i.e., was the flag
> only to be able to turn it on/off for individual milestones during
> development?).

Hmm. I hadn't realized that this could be a problem. I think making
CreateProfileIcon a static method is a bit messy since it shares the majority of
its implementation with CreateProfileShortcuts.

Can we just change IsFeatureEnabled to ShouldCreateShortcuts? This method is
mainly used to decide whether or not to create profile_shortcut_manager_ in
profile_manager, but we could just always create it and then
DCHECK(ShouldCreateShortcuts()) at the top of shortcut creation method calls.

I haven't noticed any problems locally except that the profile icon isn't set
the first time the new profile is created. A subsequent patch to browser should
fix that.

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:758:
prefs_->SetBoolean(prefs::kProfileIconCreated, true);
On 2013/05/03 17:20:47, Alexei Svitkine wrote:
> I think this should be set in a callback as a result of CreateProfileIcon
> succeeding.
> 
> Which probably means that CreateProfileIcon should take a callback, similar to
> what HasProfileShortcuts() does.

We won't need this pref if we are checking the profile icon every startup.

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:78:
ASSERT_TRUE(file_util::Delete(icon_path, false));
On 2013/05/07 12:38:39, gab wrote:
> Use base::win::TaskbarUnpinShortcutLink(icon_path.c_str()) instead of
> file_util::Delete(...) to delete pinned-to-taskbar shortcuts (otherwise the
> Windows shell gets confused and leaves a blank icon in the taskbar).

I don't think this unit test pins shortcuts? Also icon_path leads to an icon,
not a shortcut?

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:140: // badging the
browser distribution icon with the profile avatar.
On 2013/05/07 12:38:39, gab wrote:
> Why is "browser distribution" mentioned in this comment? It isn't used in this
> method at all...

I think it means the blue chromium icon vs the google chrome vs the canary icon.

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:144: // TODO(calamity):
ideally we'd just copy the app icon verbatim from the exe's
On 2013/05/03 17:20:47, Alexei Svitkine wrote:
> Nit: capitalize 'ideally'

Done.

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:184:
SHChangeNotify(SHCNE_ASSOCCHANGED, SHCNF_IDLIST | SHCNF_FLUSHNOWAIT,
On 2013/05/07 12:38:39, gab wrote:
> FYI, I have had issues with SHCNF_FLUSHNOWAIT in the past
> (http://crrev.com/145987) where my guess it that: the flush would happen
before
> the Windows Shell was even notified of the shortcut creation (or maybe in my
> specific case before the new PropertyStore was flushed out; which would result
> in the shortcut appearing, but without the properties being written to the
> PropertyStore...).
> 
> Perhaps notifying the shell first with SHChangeNotify(SHCNE_CREATE,
SHCNF_PATH,
> icon_path.c_str(), NULL) is safer here (this only makes sense if
> CreateIconFileFromImageFamily() is synchronous, is it?)? (Or with
> SHCNE_UPDATEITEM if the icon is being overwritten rather than created).

CreateIconFileFromImageFamily is synchronous. I don't think there's an issue
with shortcut updates here because the icon file is maintained separately from
the shortcut creation. Does it make sense to notify the shell when the shortcut
hasn't been created? Should I try to create the shortcut ahead of the icon file?
 
> An SHCNE_ASSOCCHANGED notification should only be required imo if Chrome's
> window is already telling the taskbar to use this icon (i.e., before it might
> even exist); is that the case? Otherwise it's preferable to shy away from
> SHCNE_ASSOCCHANGED as it results in a full desktop/taskbar icons flash.

This is possible on avatar change. The taskbar will already have the icon file
and we need to update it. I think SHCNE_ASSOCCHANGED is needed to invalidate the
icon cache and update the taskbar icon.

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:343: base::FilePath
shortcut_icon =
On 2013/05/03 17:20:47, Alexei Svitkine wrote:
> Add a TODO that we should only update the icon if it doesn't exist or actually
> needs to be updated (image change)?

Code to do this existed in web_app_win. I moved it to icon_util and used it
around the icon creation so that I could just call CreateProfileIcon on every
startup without having to worry about the SHChangeNotify unnecessarily flashing
the desktop.

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:440: // corresponding
icon file. If |ensure_shortcuts_remain| is true, then a regular
On 2013/05/03 17:20:47, Alexei Svitkine wrote:
> Update this comment.

Done.

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:444: bool
ensure_shortcuts_remain) {
On 2013/05/07 12:38:39, gab wrote:
> nit: Indent.

Done.

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:551: return
profile_path.AppendASCII(profiles::internal::kProfileIconFileName);
On 2013/05/07 12:38:39, gab wrote:
> nit: Remove profiles::internal:: namespace as this code is already in it.

Done.

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:554: const char
kProfileIconFileName[] = "Google Profile.ico";
On 2013/05/07 12:38:39, gab wrote:
> nit: constants should remain at the top of the namespace
> 
> Also, since this is constant no longer needs to be accessed from outside of
this
> file, move it up into the unnamed namespace of this file.

Done.

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:664:
CreateOrUpdateShortcutsForProfileAtPath(profile_path,
On 2013/05/07 12:38:39, gab wrote:
> Why was this un-inlined? |profile_path| only seems to be used once still...

Done.

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:684:
CreateOrUpdateShortcutsForProfileAtPath(profile_path, CREATE_ICON_ONLY,
On 2013/05/07 12:38:39, gab wrote:
> Should still update the desktop shortcut or will updating the icon and
notifying
> the shell result in an updated shortcut?

Updating and notifying refreshes the shortcut.

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.h:37: // Specifies whether
the only existing shortcuts should be updated, a new
On 2013/05/07 12:38:39, gab wrote:
> s/the only/only the

Done.

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.h:39: // should be updated.
On 2013/05/07 12:38:39, gab wrote:
> s/updated./created in the profile directory.

Done.

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.h:56: const base::FilePath&
profile_path) OVERRIDE;
On 2013/05/03 17:20:47, Alexei Svitkine wrote:
> Please add a unit test for this. (i.e. create a profile, ensure that an icon
got
> created, then delete it and call this function and ensure it got created
again.)

Done.

[gab, 2013-05-08 13:01:07]

This is looking great :)!

Just some concerns still below with depending on ProfileShortcuts as is.

Cheers!
Gab

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:757:
profile_shortcut_manager->CreateProfileIcon(GetPath());
On 2013/05/08 08:15:42, calamity wrote:
> On 2013/05/07 12:38:39, gab wrote:
> > On 2013/05/03 17:20:47, Alexei Svitkine wrote:
> > > Please check that profile_shorcut_manager is not NULL. It can be NULL if
> > > ProfileShortcutManager::IsFeatureEnabled() is false.
> > 
> > So this means that profile taskbar icons (pinned or not) will now depend on
> > ProfileShortcuts being enabled? That sounds wrong to me (i.e., taskbar icons
> > have always been badged, way before profile shortcuts came around). I'm only
> > comfortable with this if the code to disable profile shortcuts is removed
(I'm
> > not sure what the team's policy is to remove feature flags, but it feels
like
> > once profile shortcuts ship to stable we could remove this? i.e., was the
flag
> > only to be able to turn it on/off for individual milestones during
> > development?).
> 
> Hmm. I hadn't realized that this could be a problem. I think making
> CreateProfileIcon a static method is a bit messy since it shares the majority
of
> its implementation with CreateProfileShortcuts.
> 
> Can we just change IsFeatureEnabled to ShouldCreateShortcuts? This method is
> mainly used to decide whether or not to create profile_shortcut_manager_ in
> profile_manager, but we could just always create it and then
> DCHECK(ShouldCreateShortcuts()) at the top of shortcut creation method calls.
> 
> I haven't noticed any problems locally except that the profile icon isn't set
> the first time the new profile is created. A subsequent patch to browser
should
> fix that.

Even if we remove app_list from the condition, I'm still uncomfortable until:

1) BrowserDistribution::CanCreateDesktopShortcuts() is changed for a generic
BrowserDistribution::CanCreateShortcuts() -- I think it's basically the same
rules (i.e. distributions that can create desktop shortcuts are the ones that
can create shortcuts in general), but it's worth looking through the codebase to
make sure.

2) Profile shortcuts are made to work with custom user-data-dir (why doesn't
this work? seems like it shouldn't be too hard... is it just a matter of reading
the switch and writing it back to the shortcuts created for a custom
user-data-dir (and perhaps giving those shortcuts a custom name based on the
user-data-dir to avoid naming conflicts)?)


Another option perhaps is to build a resource file of all the possible badged
Chrome icons and index in that file for the correct icon at runtime (i.e.,
multiple user-data-dirs could use the same resource file with no problem without
having to make profile shortcuts compatible with user-data-dirs :))

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:758:
prefs_->SetBoolean(prefs::kProfileIconCreated, true);
On 2013/05/08 08:15:42, calamity wrote:
> On 2013/05/03 17:20:47, Alexei Svitkine wrote:
> > I think this should be set in a callback as a result of CreateProfileIcon
> > succeeding.
> > 
> > Which probably means that CreateProfileIcon should take a callback, similar
to
> > what HasProfileShortcuts() does.
> 
> We won't need this pref if we are checking the profile icon every startup.

Haven't looked deeply in the pref part of this, but it seems checking a pref is
preferable then doing I/O every startup.

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:78:
ASSERT_TRUE(file_util::Delete(icon_path, false));
On 2013/05/08 08:15:42, calamity wrote:
> On 2013/05/07 12:38:39, gab wrote:
> > Use base::win::TaskbarUnpinShortcutLink(icon_path.c_str()) instead of
> > file_util::Delete(...) to delete pinned-to-taskbar shortcuts (otherwise the
> > Windows shell gets confused and leaves a blank icon in the taskbar).
> 
> I don't think this unit test pins shortcuts? Also icon_path leads to an icon,
> not a shortcut?

Oh whoops, my bad :)!

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:184:
SHChangeNotify(SHCNE_ASSOCCHANGED, SHCNF_IDLIST | SHCNF_FLUSHNOWAIT,
On 2013/05/08 08:15:42, calamity wrote:
> On 2013/05/07 12:38:39, gab wrote:
> > FYI, I have had issues with SHCNF_FLUSHNOWAIT in the past
> > (http://crrev.com/145987) where my guess it that: the flush would happen
> before
> > the Windows Shell was even notified of the shortcut creation (or maybe in my
> > specific case before the new PropertyStore was flushed out; which would
result
> > in the shortcut appearing, but without the properties being written to the
> > PropertyStore...).
> > 
> > Perhaps notifying the shell first with SHChangeNotify(SHCNE_CREATE,
> SHCNF_PATH,
> > icon_path.c_str(), NULL) is safer here (this only makes sense if
> > CreateIconFileFromImageFamily() is synchronous, is it?)? (Or with
> > SHCNE_UPDATEITEM if the icon is being overwritten rather than created).
> 
> CreateIconFileFromImageFamily is synchronous. I don't think there's an issue
> with shortcut updates here because the icon file is maintained separately from
> the shortcut creation. Does it make sense to notify the shell when the
shortcut
> hasn't been created? Should I try to create the shortcut ahead of the icon
file?

SHChangeNotify(SHCNE_CREATE, SHCNF_PATH, icon_path.c_str(), NULL);
will be sufficient here just so that the shell knows the file exists when it is
used later.

In the startup sequence, is the icon created before SetAppIconForWindow ([1]) is
called? Otherwise the icon is indeed used before which forces us to do a
SHCNE_ASSOCCHANGED here :(...

We should also remove the runtime badging logic if this explicit icon is to
replace it (but I guess you planned to do this in a follow-up CL since I don't
see this icon being used yet?)

[1]
https://code.google.com/p/chromium/codesearch#search/&q=SetAppIconForWindow&s...
>  
> > An SHCNE_ASSOCCHANGED notification should only be required imo if Chrome's
> > window is already telling the taskbar to use this icon (i.e., before it
might
> > even exist); is that the case? Otherwise it's preferable to shy away from
> > SHCNE_ASSOCCHANGED as it results in a full desktop/taskbar icons flash.
> 
> This is possible on avatar change. The taskbar will already have the icon file
> and we need to update it. I think SHCNE_ASSOCCHANGED is needed to invalidate
the
> icon cache and update the taskbar icon.

True, do we have a way to tell when this is the case? Perhaps rely on whether
the icon file existed already or not? (Ideally we only use SHCNE_ASSOCCHANGED
when absolutely necessary (and following user action since it is rather
disturbing, but slightly more acceptable if the user knows what triggered it)).

https://chromiumcodereview.appspot.com/14137032/diff/41003/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/41003/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:756:
profile_shortcut_manager->CreateProfileIcon(GetPath());
Why call this from here when it was already called from CreateProfile() in
chrome_browser_main?

https://chromiumcodereview.appspot.com/14137032/diff/41003/chrome/browser/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/41003/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:184:
SHChangeNotify(SHCNE_ASSOCCHANGED, SHCNF_IDLIST | SHCNF_FLUSHNOWAIT,
Awesome, that's much better :) -- I didn't know we had such code and had been
thinking of writing something similar myself! I've been trying to find a way to
avoid a SHCNE_ASSOCCHANGED every Chrome auto-update and this is a prerequisite
:).

https://chromiumcodereview.appspot.com/14137032/diff/41003/chrome/browser/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

https://chromiumcodereview.appspot.com/14137032/diff/41003/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.h:17: extern const char
kProfileIconFileName[];
Remove this from the header (and move it to the unnamed namespace in the .cc
file).

https://chromiumcodereview.appspot.com/14137032/diff/41003/ui/gfx/icon_util.h
File ui/gfx/icon_util.h (right):

https://chromiumcodereview.appspot.com/14137032/diff/41003/ui/gfx/icon_util.h...
ui/gfx/icon_util.h:143: base::MD5Digest* digest);
Forward-declare base::MD5Digest instead of including "base/md5.h" here.

[Alexei Svitkine (slow), 2013-05-08 15:12:04]

(Haven't looked at the updated code yet, just replying to comments.)

On 2013/05/08 08:15:41, calamity wrote:
> Summary:
> -rework
> -moved icon hashing from web_app_win to icon_util
> -made windows .ico file creation use important_file_handler
> 
> Hey, benwells@ mentioned that the v2 app list disabling these profile
shortcuts
> is wrong. In particular, if you launch the app list and then launch chrome
from
> there, profile shortcuts will currently be disabled. I've filed another bug
for
> this. http://crbug.com/238942.

I mentioned this concern on https://chromiumcodereview.appspot.com/12570009/

> 
> Can we just remove the app list condition from IsFeatureEnabled()?

If you just remove it, you'll probably regress: http://crbug.com/174849

So you need a different way to fix that, when you remove the check from
IsFeatureEnabled().
> 
> 
> 
> It feels like returning the raw bytes of an image would only be useful if the
> caller either wanted them explicitly for some reason or didn't want to write
the
> file written as an important file and I'm not sure when we would want that
since
> icon files are probably always "important".

Okay, that's fair. I can see cases where we may want the bytes, but I agree that
currently we don't need them. So I think it's fine to just use important file
writer by default.

Can you split the important file writer change to a separate CL, since that's a
useful change on its own and can be landed separately of these changes (while
these bigger changes are still being reviewed)? Thanks.

I think the hashing stuff should also be in a separate CL.

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:757:
profile_shortcut_manager->CreateProfileIcon(GetPath());
On 2013/05/08 13:01:08, gab wrote:
> On 2013/05/08 08:15:42, calamity wrote:
> > On 2013/05/07 12:38:39, gab wrote:
> > > On 2013/05/03 17:20:47, Alexei Svitkine wrote:
> > > > Please check that profile_shorcut_manager is not NULL. It can be NULL if
> > > > ProfileShortcutManager::IsFeatureEnabled() is false.
> > > 
> > > So this means that profile taskbar icons (pinned or not) will now depend
on
> > > ProfileShortcuts being enabled? That sounds wrong to me (i.e., taskbar
icons
> > > have always been badged, way before profile shortcuts came around). I'm
only
> > > comfortable with this if the code to disable profile shortcuts is removed
> (I'm
> > > not sure what the team's policy is to remove feature flags, but it feels
> like
> > > once profile shortcuts ship to stable we could remove this? i.e., was the
> flag
> > > only to be able to turn it on/off for individual milestones during
> > > development?).
> > 
> > Hmm. I hadn't realized that this could be a problem. I think making
> > CreateProfileIcon a static method is a bit messy since it shares the
majority
> of
> > its implementation with CreateProfileShortcuts.
> > 
> > Can we just change IsFeatureEnabled to ShouldCreateShortcuts? This method is
> > mainly used to decide whether or not to create profile_shortcut_manager_ in
> > profile_manager, but we could just always create it and then
> > DCHECK(ShouldCreateShortcuts()) at the top of shortcut creation method
calls.
> > 
> > I haven't noticed any problems locally except that the profile icon isn't
set
> > the first time the new profile is created. A subsequent patch to browser
> should
> > fix that.
> 
> Even if we remove app_list from the condition, I'm still uncomfortable until:
> 
> 1) BrowserDistribution::CanCreateDesktopShortcuts() is changed for a generic
> BrowserDistribution::CanCreateShortcuts() -- I think it's basically the same
> rules (i.e. distributions that can create desktop shortcuts are the ones that
> can create shortcuts in general), but it's worth looking through the codebase
to
> make sure.
> 
> 2) Profile shortcuts are made to work with custom user-data-dir (why doesn't
> this work? seems like it shouldn't be too hard... is it just a matter of
reading
> the switch and writing it back to the shortcuts created for a custom
> user-data-dir (and perhaps giving those shortcuts a custom name based on the
> user-data-dir to avoid naming conflicts)?)

I wrote a CL to implement this in: https://codereview.chromium.org/11852022/

(I believe it covered most of the cases, but there was still some
--user-data-dir edge cases that weren't covered with that CL.)

It added quite a bit of extra complexity to the profile shortcuts code and
Sailesh didn't think it made sense to add such complexity for a use case normal
users aren't expected to run. Disabling the feature entirely was a good
compromise we agreed on.

I still agree with that conclusion, we shouldn't complicate the code so much to
support --user-data-dir.

> 
> 
> Another option perhaps is to build a resource file of all the possible badged
> Chrome icons and index in that file for the correct icon at runtime (i.e.,
> multiple user-data-dirs could use the same resource file with no problem
without
> having to make profile shortcuts compatible with user-data-dirs :))

That's a good idea, though I'm not sure whether we want to lose the flexibility
of custom-badging functionality long-term - for example if/once profiles start
using the GAIA picture for their avatar. (Though it's also questionable whether
that will look good for badging.)

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:757:
profile_shortcut_manager->CreateProfileIcon(GetPath());
On 2013/05/08 08:15:42, calamity wrote:
> On 2013/05/07 12:38:39, gab wrote:
> > On 2013/05/03 17:20:47, Alexei Svitkine wrote:
> > > Please check that profile_shorcut_manager is not NULL. It can be NULL if
> > > ProfileShortcutManager::IsFeatureEnabled() is false.
> > 
> > So this means that profile taskbar icons (pinned or not) will now depend on
> > ProfileShortcuts being enabled? That sounds wrong to me (i.e., taskbar icons
> > have always been badged, way before profile shortcuts came around). I'm only
> > comfortable with this if the code to disable profile shortcuts is removed
(I'm
> > not sure what the team's policy is to remove feature flags, but it feels
like
> > once profile shortcuts ship to stable we could remove this? i.e., was the
flag
> > only to be able to turn it on/off for individual milestones during
> > development?).
> 
> Hmm. I hadn't realized that this could be a problem. I think making
> CreateProfileIcon a static method is a bit messy since it shares the majority
of
> its implementation with CreateProfileShortcuts.

We could make CreateOrUpdateShortcutsForProfileAtPath() static too, since it's
private already.

> 
> Can we just change IsFeatureEnabled to ShouldCreateShortcuts? This method is
> mainly used to decide whether or not to create profile_shortcut_manager_ in
> profile_manager, but we could just always create it and then
> DCHECK(ShouldCreateShortcuts()) at the top of shortcut creation method calls.

I guess we could do that, though this would require changing the current users
of the API that currently check if the profile shortcut manager isn't NULL
before calling stuff on it, whereas now they'll need to check
ShouldCreateShortcuts().

> 
> I haven't noticed any problems locally except that the profile icon isn't set
> the first time the new profile is created. A subsequent patch to browser
should
> fix that.

[calamity, 2013-05-09 06:12:16]

Icon creation: https://chromiumcodereview.appspot.com/14839008/
important file writer: https://chromiumcodereview.appspot.com/15080002/

I've left the icon util stuff in because it needs to be there for the icon
checks. I'll rebase it once the other CLs land.


> > Can we just remove the app list condition from IsFeatureEnabled()?
> 
> If you just remove it, you'll probably regress: http://crbug.com/174849
> 
It does indeed regress but I'm not sure if we even care anymore? The app
launcher and chrome are being unified after all.

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:757:
profile_shortcut_manager->CreateProfileIcon(GetPath());
There are only a few callers. I'll just set it to check for NULL for now and fix
it in a follow-up CL?

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:758:
prefs_->SetBoolean(prefs::kProfileIconCreated, true);
On 2013/05/08 13:01:08, gab wrote:
> On 2013/05/08 08:15:42, calamity wrote:
> > On 2013/05/03 17:20:47, Alexei Svitkine wrote:
> > > I think this should be set in a callback as a result of CreateProfileIcon
> > > succeeding.
> > > 
> > > Which probably means that CreateProfileIcon should take a callback,
similar
> to
> > > what HasProfileShortcuts() does.
> > 
> > We won't need this pref if we are checking the profile icon every startup.
> 
> Haven't looked deeply in the pref part of this, but it seems checking a pref
is
> preferable then doing I/O every startup.

The pref is a "on first run past this change" pref which won't check the icon
file exists on every startup. The startup IO will be done async and will use the
hashing code to only write if necessary.

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:184:
SHChangeNotify(SHCNE_ASSOCCHANGED, SHCNF_IDLIST | SHCNF_FLUSHNOWAIT,
> SHChangeNotify(SHCNE_CREATE, SHCNF_PATH, icon_path.c_str(), NULL);
> will be sufficient here just so that the shell knows the file exists when it
is
> used later.
> 
> In the startup sequence, is the icon created before SetAppIconForWindow ([1])
is
> called? Otherwise the icon is indeed used before which forces us to do a
> SHCNE_ASSOCCHANGED here :(...
> 

SetAppIconForWindow would probably be called in the constructor of browser.cc
since that's where the app id is set. Since this icon is created during profile
load, we won't have the browser at that point so I'm not sure how we'd know if
the SetAppIconForWindow had already been run at icon creation time. I could make
it synchronous by moving this icon creation into browser.cc and then having a
callback to browser that calls SetAppIconForWindow.
> We should also remove the runtime badging logic if this explicit icon is to
> replace it (but I guess you planned to do this in a follow-up CL since I don't
> see this icon being used yet?)
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#search/&q=SetAppIconForWindow&s...
> >  
> > > An SHCNE_ASSOCCHANGED notification should only be required imo if Chrome's
> > > window is already telling the taskbar to use this icon (i.e., before it
> might
> > > even exist); is that the case? Otherwise it's preferable to shy away from
> > > SHCNE_ASSOCCHANGED as it results in a full desktop/taskbar icons flash.
> > 
> > This is possible on avatar change. The taskbar will already have the icon
file
> > and we need to update it. I think SHCNE_ASSOCCHANGED is needed to invalidate
> the
> > icon cache and update the taskbar icon.
> 
> True, do we have a way to tell when this is the case? Perhaps rely on whether
> the icon file existed already or not? (Ideally we only use SHCNE_ASSOCCHANGED
> when absolutely necessary (and following user action since it is rather
> disturbing, but slightly more acceptable if the user knows what triggered
it)).

Avatar change is an event that's listened for by ProfileShortcutManagerWin so we
can definitely catch this case.

Given the information above, let me know how I should proceed with this
SHChangeNotify stuff.

https://chromiumcodereview.appspot.com/14137032/diff/41003/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/41003/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:756:
profile_shortcut_manager->CreateProfileIcon(GetPath());
On 2013/05/08 13:01:08, gab wrote:
> Why call this from here when it was already called from CreateProfile() in
> chrome_browser_main?

chrome_browser_main only runs for the first loaded profile. We want to do this
on other profile loads. This code doesn't run for the initial profile.

https://chromiumcodereview.appspot.com/14137032/diff/41003/chrome/browser/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

https://chromiumcodereview.appspot.com/14137032/diff/41003/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.h:17: extern const char
kProfileIconFileName[];
On 2013/05/08 13:01:08, gab wrote:
> Remove this from the header (and move it to the unnamed namespace in the .cc
> file).

Done.

https://chromiumcodereview.appspot.com/14137032/diff/41003/ui/gfx/icon_util.h
File ui/gfx/icon_util.h (right):

https://chromiumcodereview.appspot.com/14137032/diff/41003/ui/gfx/icon_util.h...
ui/gfx/icon_util.h:143: base::MD5Digest* digest);
On 2013/05/08 13:01:08, gab wrote:
> Forward-declare base::MD5Digest instead of including "base/md5.h" here.

Done in new CL.

[gab, 2013-05-09 13:43:56]

lg, replies below.

@sail, see comment with a "sail@" mention below

Cheers!
Gab

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:757:
profile_shortcut_manager->CreateProfileIcon(GetPath());
On 2013/05/08 15:12:04, Alexei Svitkine wrote:
> On 2013/05/08 13:01:08, gab wrote:
> > On 2013/05/08 08:15:42, calamity wrote:
> > > On 2013/05/07 12:38:39, gab wrote:
> > > > On 2013/05/03 17:20:47, Alexei Svitkine wrote:
> > > > > Please check that profile_shorcut_manager is not NULL. It can be NULL
if
> > > > > ProfileShortcutManager::IsFeatureEnabled() is false.
> > > > 
> > > > So this means that profile taskbar icons (pinned or not) will now depend
> on
> > > > ProfileShortcuts being enabled? That sounds wrong to me (i.e., taskbar
> icons
> > > > have always been badged, way before profile shortcuts came around). I'm
> only
> > > > comfortable with this if the code to disable profile shortcuts is
removed
> > (I'm
> > > > not sure what the team's policy is to remove feature flags, but it feels
> > like
> > > > once profile shortcuts ship to stable we could remove this? i.e., was
the
> > flag
> > > > only to be able to turn it on/off for individual milestones during
> > > > development?).
> > > 
> > > Hmm. I hadn't realized that this could be a problem. I think making
> > > CreateProfileIcon a static method is a bit messy since it shares the
> majority
> > of
> > > its implementation with CreateProfileShortcuts.
> > > 
> > > Can we just change IsFeatureEnabled to ShouldCreateShortcuts? This method
is
> > > mainly used to decide whether or not to create profile_shortcut_manager_
in
> > > profile_manager, but we could just always create it and then
> > > DCHECK(ShouldCreateShortcuts()) at the top of shortcut creation method
> calls.
> > > 
> > > I haven't noticed any problems locally except that the profile icon isn't
> set
> > > the first time the new profile is created. A subsequent patch to browser
> > should
> > > fix that.
> > 
> > Even if we remove app_list from the condition, I'm still uncomfortable
until:
> > 
> > 1) BrowserDistribution::CanCreateDesktopShortcuts() is changed for a generic
> > BrowserDistribution::CanCreateShortcuts() -- I think it's basically the same
> > rules (i.e. distributions that can create desktop shortcuts are the ones
that
> > can create shortcuts in general), but it's worth looking through the
codebase
> to
> > make sure.
> > 
> > 2) Profile shortcuts are made to work with custom user-data-dir (why doesn't
> > this work? seems like it shouldn't be too hard... is it just a matter of
> reading
> > the switch and writing it back to the shortcuts created for a custom
> > user-data-dir (and perhaps giving those shortcuts a custom name based on the
> > user-data-dir to avoid naming conflicts)?)
> 
> I wrote a CL to implement this in: https://codereview.chromium.org/11852022/
> 
> (I believe it covered most of the cases, but there was still some
> --user-data-dir edge cases that weren't covered with that CL.)
> 
> It added quite a bit of extra complexity to the profile shortcuts code and
> Sailesh didn't think it made sense to add such complexity for a use case
normal
> users aren't expected to run. Disabling the feature entirely was a good
> compromise we agreed on.
> 
> I still agree with that conclusion, we shouldn't complicate the code so much
to
> support --user-data-dir.

I see, so as it is now we only use this icon for shortcut properties, but at
runtime Chrome still sets its icon and badges at runtime like before, right? 
We will have to change this code to use the icon created in this CL instead
though, right?
In which case this will regress custom user-data-dir in that they will no longer
get badged icons in their taskbar (I agree however that custom user-data-dir is
probably not nearly as useful as it used to be, now that we've had
multi-profiles for over a year); the appid will still be set though, thus the
icons will still be split in the taskbar at runtime for separate profiles, they
simply won't be badged (unless we keep the runtime badging code around for this
use-case only, but I also don't like that...).


> 
> > 
> > 
> > Another option perhaps is to build a resource file of all the possible
badged
> > Chrome icons and index in that file for the correct icon at runtime (i.e.,
> > multiple user-data-dirs could use the same resource file with no problem
> without
> > having to make profile shortcuts compatible with user-data-dirs :))
> 
> That's a good idea, though I'm not sure whether we want to lose the
flexibility
> of custom-badging functionality long-term - for example if/once profiles start
> using the GAIA picture for their avatar. (Though it's also questionable
whether
> that will look good for badging.)
> 

Hmmm.. badging with the GAIA picture seems weird to me.

At the same time I also agree that making something static like a big resource
blob of badged icons might come back to bite us and be hard to maintain when we
want to change/add badges.


I reread the code again and tried to come up with a better approach, but I think
the current approach is the only possible approach since this is so tied with
the current code (i.e., all the logic to change icons when going from 1 to 2
profiles, back to a single profile, etc.).
I still don't like regressing a feature of Chrome though (custom
user-data-dirs), should we officially deprecate custom user-data-dirs given that
they have essentially been replaced by multi-profiles? (I wouldn't go as far as
removing the code for now as some people still use it for testing, but
deprecating it in the sense that features will no longer go out of their way to
support it?)
Adding sail@ for his opinion on this.

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:757:
profile_shortcut_manager->CreateProfileIcon(GetPath());
On 2013/05/08 15:12:04, Alexei Svitkine wrote:
> On 2013/05/08 08:15:42, calamity wrote:
> > On 2013/05/07 12:38:39, gab wrote:
> > > On 2013/05/03 17:20:47, Alexei Svitkine wrote:
> > > > Please check that profile_shorcut_manager is not NULL. It can be NULL if
> > > > ProfileShortcutManager::IsFeatureEnabled() is false.
> > > 
> > > So this means that profile taskbar icons (pinned or not) will now depend
on
> > > ProfileShortcuts being enabled? That sounds wrong to me (i.e., taskbar
icons
> > > have always been badged, way before profile shortcuts came around). I'm
only
> > > comfortable with this if the code to disable profile shortcuts is removed
> (I'm
> > > not sure what the team's policy is to remove feature flags, but it feels
> like
> > > once profile shortcuts ship to stable we could remove this? i.e., was the
> flag
> > > only to be able to turn it on/off for individual milestones during
> > > development?).
> > 
> > Hmm. I hadn't realized that this could be a problem. I think making
> > CreateProfileIcon a static method is a bit messy since it shares the
majority
> of
> > its implementation with CreateProfileShortcuts.
> 
> We could make CreateOrUpdateShortcutsForProfileAtPath() static too, since it's
> private already.
> 
> > 
> > Can we just change IsFeatureEnabled to ShouldCreateShortcuts? This method is
> > mainly used to decide whether or not to create profile_shortcut_manager_ in
> > profile_manager, but we could just always create it and then
> > DCHECK(ShouldCreateShortcuts()) at the top of shortcut creation method
calls.

I'm not sure what you mean here, if you always create the ProfileManager, then
you need more than a DCHECK, you need actual logic (i.e. ifs) to make this a
no-op in the right distributions... anyways, if we keep disabling for custom
user-data-dirs, might as well keep the disabling logic where it's at now imo.

> 
> I guess we could do that, though this would require changing the current users
> of the API that currently check if the profile shortcut manager isn't NULL
> before calling stuff on it, whereas now they'll need to check
> ShouldCreateShortcuts().
> 
> > 
> > I haven't noticed any problems locally except that the profile icon isn't
set
> > the first time the new profile is created. A subsequent patch to browser
> should
> > fix that.
>

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/28001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:184:
SHChangeNotify(SHCNE_ASSOCCHANGED, SHCNF_IDLIST | SHCNF_FLUSHNOWAIT,
On 2013/05/09 06:12:16, calamity wrote:
> > SHChangeNotify(SHCNE_CREATE, SHCNF_PATH, icon_path.c_str(), NULL);
> > will be sufficient here just so that the shell knows the file exists when it
> is
> > used later.
> > 
> > In the startup sequence, is the icon created before SetAppIconForWindow
([1])
> is
> > called? Otherwise the icon is indeed used before which forces us to do a
> > SHCNE_ASSOCCHANGED here :(...
> > 
> 
> SetAppIconForWindow would probably be called in the constructor of browser.cc
> since that's where the app id is set. Since this icon is created during
profile
> load, we won't have the browser at that point so I'm not sure how we'd know if
> the SetAppIconForWindow had already been run at icon creation time. I could
make
> it synchronous by moving this icon creation into browser.cc and then having a
> callback to browser that calls SetAppIconForWindow.

Ah right, makes sense, the current code is synchronous with profile creation
which is guaranteed to happen before a browser is created.

However I'm curious as to how this even works since chrome_browser_main.cc is
running on the UI thread I believe and you go straight to
CreateOrUpdateDesktopShortcutsAndIconForProfile() which DCHECKs it's on the FILE
thread... (it probably makes sense to only do this on the FILE thread too to
avoid delaying startup).

Currently Browsers do not even call SetAppIconForWindow() -- a follow-up CL will
actually need to do this and SetRelaunchDetailsForWindow() for pinning to work.

I'm not sure if callbacks are queued and guaranteed to happen in the order they
were queued on each thread, but if that's the case, then we can queue up
CreateProfileIcon on the FILE thread early and queue up SetAppIconForWindow()
the same way when creating a browser and the former should happen before the
later as desired...

> > We should also remove the runtime badging logic if this explicit icon is to
> > replace it (but I guess you planned to do this in a follow-up CL since I
don't
> > see this icon being used yet?)
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=SetAppIconForWindow&s...
> > >  
> > > > An SHCNE_ASSOCCHANGED notification should only be required imo if
Chrome's
> > > > window is already telling the taskbar to use this icon (i.e., before it
> > might
> > > > even exist); is that the case? Otherwise it's preferable to shy away
from
> > > > SHCNE_ASSOCCHANGED as it results in a full desktop/taskbar icons flash.
> > > 
> > > This is possible on avatar change. The taskbar will already have the icon
> file
> > > and we need to update it. I think SHCNE_ASSOCCHANGED is needed to
invalidate
> > the
> > > icon cache and update the taskbar icon.
> > 
> > True, do we have a way to tell when this is the case? Perhaps rely on
whether
> > the icon file existed already or not? (Ideally we only use
SHCNE_ASSOCCHANGED
> > when absolutely necessary (and following user action since it is rather
> > disturbing, but slightly more acceptable if the user knows what triggered
> it)).
> 
> Avatar change is an event that's listened for by ProfileShortcutManagerWin so
we
> can definitely catch this case.
> 
> Given the information above, let me know how I should proceed with this
> SHChangeNotify stuff.

The current code is perfect (i.e. only notify if the icon was created or
actually changed).

You can make it slightly better by only notifying SHCNE_CREATE when the icon is
created from scratch (i.e. the shell only needs to know about this new icon, not
refresh its whole cache), see
https://code.google.com/p/chromium/codesearch#chromium/src/base/win/shortcut....

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:186: // register with
the taskbar an desktop.
nit: s/an/and

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:643:
CreateOrUpdateShortcutsForProfileAtPath(profile_path, CREATE_ICON_ONLY,
Simply call CreateProfileIcon(profile_path); here, makes it more readable rather
than having to derive intent from the parameters.

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/ui/...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/ui/...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:47: #include
"chrome/browser/profiles/profile_shortcut_manager.h"
nit: I don't think you need these includes anymore (at least you're no longer
adding any code to this file!)

[Alexei Svitkine (slow), 2013-05-09 16:45:47]

I think this is getting close, thanks for working on it!

I still have some concerns, though - see my comments below.

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/chr...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/chr...
chrome/browser/chrome_browser_main.cc:362: // Ensure the icon file for the
profile has been created.
Does ProfileImpl::OnPrefsLoaded() not get called in this case?

Seems strange to me if that would be the case. If it does get called, then you
don't need this here, right?

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:754: ProfileShortcutManager*
profile_shortcut_manager =
I still think we should only do this if the pref isn't set.

We don't want to be posting an additional task to the FILE thread on every
startup. Sure, it won't block UI, but the more things that get posted during
startup, the slower Chrome runs initially until it catches up with the list of
things that have been posted. This is bad.

Consider that once users have ran this once, the expected case will be that for
99.9% of users, the icon already exists and is just fine, so you're now bogging
down the FILE thread more for those users needlessly. Having the pref eliminates
that except for when the icon hasn't been created.

And no, I don't think the hashing code solves the problem. You're still posting
a task to the FILE thread that gets run on startup, delaying other tasks. That
task still ends up generating the SkBitmaps, up to 256x256 in size, which is
still a lot of processing. Plus, with the hashing it now has to read a file
(storing the hash) to figure out if the hash matches or not.

All of this is non-trivial and without the pref, you'd be doing it all
redundantly for 99.9% of cases, once the icon has been made.

So I still think you should use the pref here. That way, you'll only be posting
the task when it's needed.

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:780:
CreateProfileIcon) {
Nit: Can fit on the same line.

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:783:
base::FilePath icon_path =
Nit: const.

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:607: void
ProfileShortcutManagerWin::CreateProfileIcon(
Nit: Rename to "CreateOrUpdateProfileIcon".

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:686:
CreateOrUpdateShortcutsForProfileAtPath(profile_path, CREATE_ICON_ONLY,
Nit: Call "CreateOrUpdateProfileIcon".

[Alexei Svitkine (slow), 2013-05-09 19:28:18]

On 2013/05/09 06:12:16, calamity wrote:
> > > Can we just remove the app list condition from IsFeatureEnabled()?
> > 
> > If you just remove it, you'll probably regress: http://crbug.com/174849
> > 
> It does indeed regress but I'm not sure if we even care anymore? The app
> launcher and chrome are being unified after all.

I'm not familiar with the current app launched plans, so you should double check
with the right folks on that front, but my understanding is that the idea was to
have it possible to install a v2 app without Chrome, in which case you'll get
the app launcher but not Chrome? Or at the very least, even if you do get Chrome
behind the scenes, since what you're doing is installing an app actually, that
it shouldn't create an icon for Chrome on the desktop.

At least, that's my understanding. You should check with the app list PM /
owners, if you think that's not a concern anymore (and let me know too, so that
I understand it). Thanks!

[calamity, 2013-05-24 08:58:48]

Hope I haven't missed anything.

I talked to benwells@ and koz@ about the app launcher stuff and the app launcher
creating a chrome shortcut won't be an issue because the app launcher is
becoming a feature of chrome and will never be around without chrome having been
installed.

Let me know if there's anything else.

Thanks.

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/chr...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/chr...
chrome/browser/chrome_browser_main.cc:362: // Ensure the icon file for the
profile has been created.
On 2013/05/09 16:45:47, Alexei Svitkine wrote:
> Does ProfileImpl::OnPrefsLoaded() not get called in this case?
> 
> Seems strange to me if that would be the case. If it does get called, then you
> don't need this here, right?

Yep, sorry. I got confused. ProfileManager::OnProfileCreated isn't called for
the startup profile but ProfileImpl::OnPrefsLoaded is.

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:754: ProfileShortcutManager*
profile_shortcut_manager =
On 2013/05/09 16:45:47, Alexei Svitkine wrote:
> I still think we should only do this if the pref isn't set.
> 
> We don't want to be posting an additional task to the FILE thread on every
> startup. Sure, it won't block UI, but the more things that get posted during
> startup, the slower Chrome runs initially until it catches up with the list of
> things that have been posted. This is bad.
> 
> Consider that once users have ran this once, the expected case will be that
for
> 99.9% of users, the icon already exists and is just fine, so you're now
bogging
> down the FILE thread more for those users needlessly. Having the pref
eliminates
> that except for when the icon hasn't been created.
> 
> And no, I don't think the hashing code solves the problem. You're still
posting
> a task to the FILE thread that gets run on startup, delaying other tasks. That
> task still ends up generating the SkBitmaps, up to 256x256 in size, which is
> still a lot of processing. Plus, with the hashing it now has to read a file
> (storing the hash) to figure out if the hash matches or not.
> 
> All of this is non-trivial and without the pref, you'd be doing it all
> redundantly for 99.9% of cases, once the icon has been made.
> 
> So I still think you should use the pref here. That way, you'll only be
posting
> the task when it's needed.

Done.

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:780:
CreateProfileIcon) {
On 2013/05/09 16:45:47, Alexei Svitkine wrote:
> Nit: Can fit on the same line.

Done.

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:783:
base::FilePath icon_path =
On 2013/05/09 16:45:47, Alexei Svitkine wrote:
> Nit: const.

Done.

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:186: // register with
the taskbar an desktop.
On 2013/05/09 13:43:56, gab wrote:
> nit: s/an/and

Done.

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:607: void
ProfileShortcutManagerWin::CreateProfileIcon(
On 2013/05/09 16:45:47, Alexei Svitkine wrote:
> Nit: Rename to "CreateOrUpdateProfileIcon".

Split this into CreateProfileIcon and UpdateProfileIcon so that we can save on
icon cache invalidations.

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:643:
CreateOrUpdateShortcutsForProfileAtPath(profile_path, CREATE_ICON_ONLY,
On 2013/05/09 13:43:56, gab wrote:
> Simply call CreateProfileIcon(profile_path); here, makes it more readable
rather
> than having to derive intent from the parameters.

Done.

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:686:
CreateOrUpdateShortcutsForProfileAtPath(profile_path, CREATE_ICON_ONLY,
On 2013/05/09 16:45:47, Alexei Svitkine wrote:
> Nit: Call "CreateOrUpdateProfileIcon".

Done.

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/ui/...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/ui/...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:47: #include
"chrome/browser/profiles/profile_shortcut_manager.h"
On 2013/05/09 13:43:56, gab wrote:
> nit: I don't think you need these includes anymore (at least you're no longer
> adding any code to this file!)

Done.

[Alexei Svitkine (slow), 2013-05-24 14:43:51]

https://codereview.chromium.org/14137032/diff/90001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/14137032/diff/90001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:738: // Ensure the profile's icon file
has been created.
Nit: Indent 2 more.

https://codereview.chromium.org/14137032/diff/90001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:743:
prefs_->SetBoolean(prefs::kProfileIconCreated, true);
I would still like to have this pref set only after icon creation succeeded.

I think both checking the pref and updating it can be moved to
CreateProfileIcon(), that way the public interface of the function can remain
simple. Then, setting the pref on success can be done in a private function on
ProfileShortcutManagerWin that gets passed as the 2nd callback param to
PostTaskAndReplyWithResult(), based on the result of the other op.

https://codereview.chromium.org/14137032/diff/90001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_shortcut_manager.h (right):

https://codereview.chromium.org/14137032/diff/90001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager.h:20: // not be used to trigger
an update of the icon.
Why not? Maybe explain the behavior rather than telling people what it
should/shouldn't be used for.

https://codereview.chromium.org/14137032/diff/90001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/90001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_shortcut_manager_win.cc:156: bool
is_icon_create) {
Instead of passing this parameter, can this just check if the file already
exists and figure this out for itself?

This is already on the FILE thread, so such a check should be fine to do here.

https://codereview.chromium.org/14137032/diff/90001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/14137032/diff/90001/chrome/common/pref_names....
chrome/common/pref_names.cc:88: const char kProfileIconCreated[] =
"profile.icon_created";
To be more future-proof, how about changing this to profile.icon_version and
starting it with value 1 in this CL? This way, if there's some scenario where we
have to update all these icons when Chrome is updated, we can re-use this pref
for that logic.

https://codereview.chromium.org/14137032/diff/90001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/14137032/diff/90001/chrome/common/pref_names....
chrome/common/pref_names.h:37: 
Nit: Remove extra blank line.

[gab, 2013-05-24 15:54:34]

On 2013/05/24 08:58:48, calamity wrote:
> Hope I haven't missed anything.
> 
> I talked to benwells@ and koz@ about the app launcher stuff and the app
launcher
> creating a chrome shortcut won't be an issue because the app launcher is
> becoming a feature of chrome and will never be around without chrome having
been
> installed.

I haven't looked in detail yet (waiting for Alexei's review to be addressed),
but it is not ok for the App Launcher to create Chrome shortcuts simply because
"Chrome has to be present" as the user might have deleted those shortcuts after
installing Chrome and we shouldn't recreate user deleted shortcuts.

Cheers,
Gab


> 
> Let me know if there's anything else.
> 
> Thanks.
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/chr...
> File chrome/browser/chrome_browser_main.cc (right):
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/chr...
> chrome/browser/chrome_browser_main.cc:362: // Ensure the icon file for the
> profile has been created.
> On 2013/05/09 16:45:47, Alexei Svitkine wrote:
> > Does ProfileImpl::OnPrefsLoaded() not get called in this case?
> > 
> > Seems strange to me if that would be the case. If it does get called, then
you
> > don't need this here, right?
> 
> Yep, sorry. I got confused. ProfileManager::OnProfileCreated isn't called for
> the startup profile but ProfileImpl::OnPrefsLoaded is.
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
> File chrome/browser/profiles/profile_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
> chrome/browser/profiles/profile_impl.cc:754: ProfileShortcutManager*
> profile_shortcut_manager =
> On 2013/05/09 16:45:47, Alexei Svitkine wrote:
> > I still think we should only do this if the pref isn't set.
> > 
> > We don't want to be posting an additional task to the FILE thread on every
> > startup. Sure, it won't block UI, but the more things that get posted during
> > startup, the slower Chrome runs initially until it catches up with the list
of
> > things that have been posted. This is bad.
> > 
> > Consider that once users have ran this once, the expected case will be that
> for
> > 99.9% of users, the icon already exists and is just fine, so you're now
> bogging
> > down the FILE thread more for those users needlessly. Having the pref
> eliminates
> > that except for when the icon hasn't been created.
> > 
> > And no, I don't think the hashing code solves the problem. You're still
> posting
> > a task to the FILE thread that gets run on startup, delaying other tasks.
That
> > task still ends up generating the SkBitmaps, up to 256x256 in size, which is
> > still a lot of processing. Plus, with the hashing it now has to read a file
> > (storing the hash) to figure out if the hash matches or not.
> > 
> > All of this is non-trivial and without the pref, you'd be doing it all
> > redundantly for 99.9% of cases, once the icon has been made.
> > 
> > So I still think you should use the pref here. That way, you'll only be
> posting
> > the task when it's needed.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
> File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
> chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:780:
> CreateProfileIcon) {
> On 2013/05/09 16:45:47, Alexei Svitkine wrote:
> > Nit: Can fit on the same line.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
> chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:783:
> base::FilePath icon_path =
> On 2013/05/09 16:45:47, Alexei Svitkine wrote:
> > Nit: const.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
> File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
> chrome/browser/profiles/profile_shortcut_manager_win.cc:186: // register with
> the taskbar an desktop.
> On 2013/05/09 13:43:56, gab wrote:
> > nit: s/an/and
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
> chrome/browser/profiles/profile_shortcut_manager_win.cc:607: void
> ProfileShortcutManagerWin::CreateProfileIcon(
> On 2013/05/09 16:45:47, Alexei Svitkine wrote:
> > Nit: Rename to "CreateOrUpdateProfileIcon".
> 
> Split this into CreateProfileIcon and UpdateProfileIcon so that we can save on
> icon cache invalidations.
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
> chrome/browser/profiles/profile_shortcut_manager_win.cc:643:
> CreateOrUpdateShortcutsForProfileAtPath(profile_path, CREATE_ICON_ONLY,
> On 2013/05/09 13:43:56, gab wrote:
> > Simply call CreateProfileIcon(profile_path); here, makes it more readable
> rather
> > than having to derive intent from the parameters.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/pro...
> chrome/browser/profiles/profile_shortcut_manager_win.cc:686:
> CreateOrUpdateShortcutsForProfileAtPath(profile_path, CREATE_ICON_ONLY,
> On 2013/05/09 16:45:47, Alexei Svitkine wrote:
> > Nit: Call "CreateOrUpdateProfileIcon".
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/ui/...
> File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/70001/chrome/browser/ui/...
> chrome/browser/ui/startup/startup_browser_creator_impl.cc:47: #include
> "chrome/browser/profiles/profile_shortcut_manager.h"
> On 2013/05/09 13:43:56, gab wrote:
> > nit: I don't think you need these includes anymore (at least you're no
longer
> > adding any code to this file!)
> 
> Done.

[Alexei Svitkine (slow), 2013-05-24 20:10:48]

On 2013/05/24 15:54:34, gab wrote:
> On 2013/05/24 08:58:48, calamity wrote:
> > Hope I haven't missed anything.
> > 
> > I talked to benwells@ and koz@ about the app launcher stuff and the app
> launcher
> > creating a chrome shortcut won't be an issue because the app launcher is
> > becoming a feature of chrome and will never be around without chrome having
> been
> > installed.
> 
> I haven't looked in detail yet (waiting for Alexei's review to be addressed),
> but it is not ok for the App Launcher to create Chrome shortcuts simply
because
> "Chrome has to be present" as the user might have deleted those shortcuts
after
> installing Chrome and we shouldn't recreate user deleted shortcuts.

I think this should no longer be an issue after this change lands:

https://codereview.chromium.org/15986006/

(It's removing the code that would previously create a Chrome shortcut on first
run,
and thus responsible for the issue with the app launcher.)

[calamity, 2013-05-31 04:07:27]

Rebased beneath asvitkine@'s change.

https://chromiumcodereview.appspot.com/14137032/diff/90001/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/90001/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:738: // Ensure the profile's icon file
has been created.
On 2013/05/24 14:43:51, Alexei Svitkine wrote:
> Nit: Indent 2 more.

Done.

https://chromiumcodereview.appspot.com/14137032/diff/90001/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:743:
prefs_->SetBoolean(prefs::kProfileIconCreated, true);
On 2013/05/24 14:43:51, Alexei Svitkine wrote:
> I would still like to have this pref set only after icon creation succeeded.
> 
> I think both checking the pref and updating it can be moved to
> CreateProfileIcon(), that way the public interface of the function can remain
> simple. Then, setting the pref on success can be done in a private function on
> ProfileShortcutManagerWin that gets passed as the 2nd callback param to
> PostTaskAndReplyWithResult(), based on the result of the other op.

I'm a little hesitant to do this if the name is going to be CreateProfileIcon. I
think it would be confusing to call this and not have it actually create
anything because of a the condition in its implementation.
ProfileShortcutManager::HasProfileShortcuts already has a callback in its params
and I think it might be better to go with that. WDYT?

https://chromiumcodereview.appspot.com/14137032/diff/90001/chrome/browser/pro...
File chrome/browser/profiles/profile_shortcut_manager.h (right):

https://chromiumcodereview.appspot.com/14137032/diff/90001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager.h:20: // not be used to trigger
an update of the icon.
On 2013/05/24 14:43:51, Alexei Svitkine wrote:
> Why not? Maybe explain the behavior rather than telling people what it
> should/shouldn't be used for.

Done.

https://chromiumcodereview.appspot.com/14137032/diff/90001/chrome/browser/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/90001/chrome/browser/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:156: bool
is_icon_create) {
On 2013/05/24 14:43:51, Alexei Svitkine wrote:
> Instead of passing this parameter, can this just check if the file already
> exists and figure this out for itself?
> 
> This is already on the FILE thread, so such a check should be fine to do here.

Done.

https://chromiumcodereview.appspot.com/14137032/diff/90001/chrome/common/pref...
File chrome/common/pref_names.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/90001/chrome/common/pref...
chrome/common/pref_names.cc:88: const char kProfileIconCreated[] =
"profile.icon_created";
On 2013/05/24 14:43:51, Alexei Svitkine wrote:
> To be more future-proof, how about changing this to profile.icon_version and
> starting it with value 1 in this CL? This way, if there's some scenario where
we
> have to update all these icons when Chrome is updated, we can re-use this pref
> for that logic.

Done.

https://chromiumcodereview.appspot.com/14137032/diff/90001/chrome/common/pref...
File chrome/common/pref_names.h (right):

https://chromiumcodereview.appspot.com/14137032/diff/90001/chrome/common/pref...
chrome/common/pref_names.h:37: 
On 2013/05/24 14:43:51, Alexei Svitkine wrote:
> Nit: Remove extra blank line.

Done.

[Alexei Svitkine (slow), 2013-05-31 13:48:38]

https://chromiumcodereview.appspot.com/14137032/diff/90001/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/90001/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:743:
prefs_->SetBoolean(prefs::kProfileIconCreated, true);
On 2013/05/31 04:07:27, calamity wrote:
> On 2013/05/24 14:43:51, Alexei Svitkine wrote:
> > I would still like to have this pref set only after icon creation succeeded.
> > 
> > I think both checking the pref and updating it can be moved to
> > CreateProfileIcon(), that way the public interface of the function can
remain
> > simple. Then, setting the pref on success can be done in a private function
on
> > ProfileShortcutManagerWin that gets passed as the 2nd callback param to
> > PostTaskAndReplyWithResult(), based on the result of the other op.
> 
> I'm a little hesitant to do this if the name is going to be CreateProfileIcon.
I
> think it would be confusing to call this and not have it actually create
> anything because of a the condition in its implementation.
> ProfileShortcutManager::HasProfileShortcuts already has a callback in its
params
> and I think it might be better to go with that. WDYT?

I'm okay with the having the callback exposed too.

https://chromiumcodereview.appspot.com/14137032/diff/104003/chrome/browser/pr...
File chrome/browser/profiles/profile_shortcut_manager.h (right):

https://chromiumcodereview.appspot.com/14137032/diff/104003/chrome/browser/pr...
chrome/browser/profiles/profile_shortcut_manager.h:20: // triggers a
SHChangeNotify(SHCNE_CREATE) which will not update old icons.
Don't mention a Windows-specific behaviour in the general interface class.

Also, SHChangeNotify(SHCNE_ASSOCCHANGED) does seem to be called, so this comment
doesn't look correct.

https://chromiumcodereview.appspot.com/14137032/diff/104003/chrome/browser/pr...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/104003/chrome/browser/pr...
chrome/browser/profiles/profile_shortcut_manager_win.cc:186: bool icon_existed =
file_util::PathExists(icon_path);
Nit: had_icon, make it const

[calamity, 2013-06-04 01:24:56]

https://chromiumcodereview.appspot.com/14137032/diff/90001/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/90001/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:743:
prefs_->SetBoolean(prefs::kProfileIconCreated, true);
On 2013/05/31 13:48:38, Alexei Svitkine wrote:
> On 2013/05/31 04:07:27, calamity wrote:
> > On 2013/05/24 14:43:51, Alexei Svitkine wrote:
> > > I would still like to have this pref set only after icon creation
succeeded.
> > > 
> > > I think both checking the pref and updating it can be moved to
> > > CreateProfileIcon(), that way the public interface of the function can
> remain
> > > simple. Then, setting the pref on success can be done in a private
function
> on
> > > ProfileShortcutManagerWin that gets passed as the 2nd callback param to
> > > PostTaskAndReplyWithResult(), based on the result of the other op.
> > 
> > I'm a little hesitant to do this if the name is going to be
CreateProfileIcon.
> I
> > think it would be confusing to call this and not have it actually create
> > anything because of a the condition in its implementation.
> > ProfileShortcutManager::HasProfileShortcuts already has a callback in its
> params
> > and I think it might be better to go with that. WDYT?
> 
> I'm okay with the having the callback exposed too.

Done.

https://chromiumcodereview.appspot.com/14137032/diff/104003/chrome/browser/pr...
File chrome/browser/profiles/profile_shortcut_manager.h (right):

https://chromiumcodereview.appspot.com/14137032/diff/104003/chrome/browser/pr...
chrome/browser/profiles/profile_shortcut_manager.h:20: // triggers a
SHChangeNotify(SHCNE_CREATE) which will not update old icons.
On 2013/05/31 13:48:38, Alexei Svitkine wrote:
> Don't mention a Windows-specific behaviour in the general interface class.
> 
> Also, SHChangeNotify(SHCNE_ASSOCCHANGED) does seem to be called, so this
comment
> doesn't look correct.

Done.

https://chromiumcodereview.appspot.com/14137032/diff/104003/chrome/browser/pr...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/104003/chrome/browser/pr...
chrome/browser/profiles/profile_shortcut_manager_win.cc:186: bool icon_existed =
file_util::PathExists(icon_path);
On 2013/05/31 13:48:38, Alexei Svitkine wrote:
> Nit: had_icon, make it const

Done.

[Alexei Svitkine (slow), 2013-06-05 14:06:02]

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:742: if
(prefs_->GetInteger(prefs::kProfileIconVersion) <
I would prefer if you refactor this into a helper function, that way it can live
next to OnProfileIconCreateSuccess().

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.h (right):

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.h:178: void OnProfileIconCreateSuccess();
Add a comment, as well as one for the other function I'm asking you to add.

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager.h (right):

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager.h:20: // not overwrite an
already existing profile icon.
Please document |callback| - i.e. that it's only called on success.

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:349:
CreateOrUpdateShortcutsParams() {}
This needs to initialize |create_mode| and |action|, since these do not have
default ctors (unlike other members).

Perhaps make the ctor take them as params along with profile_path.

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:352: base::FilePath
profile_path;
Please comment the fields.

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:370: base::FilePath
shortcut_icon =
Nit: Make this const.

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:416: if
(!shortcut_icon.empty())
Since you're doing an early return on shortcut_icon.empty() above, this line is
no longer needed.

[calamity, 2013-06-07 04:26:25]

I think we may also want to make any existing taskbar shortcuts on the system
point to the profile icon at some point? We can probably do this in
MigrateChromiumShortcuts.

We'd probably also want the taskbar shortcut to always have the profile
specified because we currently have weird behavior where the taskbar shortcut
can launch a profile that isn't itself.

WDYT?

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:742: if
(prefs_->GetInteger(prefs::kProfileIconVersion) <
On 2013/06/05 14:06:03, Alexei Svitkine wrote:
> I would prefer if you refactor this into a helper function, that way it can
live
> next to OnProfileIconCreateSuccess().

Done.

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.h (right):

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.h:178: void OnProfileIconCreateSuccess();
On 2013/06/05 14:06:03, Alexei Svitkine wrote:
> Add a comment, as well as one for the other function I'm asking you to add.

Done.

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager.h (right):

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager.h:20: // not overwrite an
already existing profile icon.
On 2013/06/05 14:06:03, Alexei Svitkine wrote:
> Please document |callback| - i.e. that it's only called on success.

Done.

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:349:
CreateOrUpdateShortcutsParams() {}
On 2013/06/05 14:06:03, Alexei Svitkine wrote:
> This needs to initialize |create_mode| and |action|, since these do not have
> default ctors (unlike other members).
> 
> Perhaps make the ctor take them as params along with profile_path.

Done.

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:352: base::FilePath
profile_path;
On 2013/06/05 14:06:03, Alexei Svitkine wrote:
> Please comment the fields.

Done.

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:370: base::FilePath
shortcut_icon =
On 2013/06/05 14:06:03, Alexei Svitkine wrote:
> Nit: Make this const.

Done.

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:416: if
(!shortcut_icon.empty())
On 2013/06/05 14:06:03, Alexei Svitkine wrote:
> Since you're doing an early return on shortcut_icon.empty() above, this line
is
> no longer needed.

Done. In fact, since we always want to use the icon in the profile directory, we
can move it outside the condition.

[Alexei Svitkine (slow), 2013-06-10 17:44:51]

On 2013/06/07 04:26:25, calamity wrote:
> I think we may also want to make any existing taskbar shortcuts on the system
> point to the profile icon at some point? We can probably do this in
> MigrateChromiumShortcuts.

Possibly. It's tricky because then they have to be updated when profiles change
(e.g. what that profile gets deleted). The current setup avoids that problem.

> 
> We'd probably also want the taskbar shortcut to always have the profile
> specified because we currently have weird behavior where the taskbar shortcut
> can launch a profile that isn't itself.

Same response as above.

I think either way, these two are outside the scope of this CL.

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:416: if
(!shortcut_icon.empty())
On 2013/06/07 04:26:26, calamity wrote:
> On 2013/06/05 14:06:03, Alexei Svitkine wrote:
> > Since you're doing an early return on shortcut_icon.empty() above, this line
> is
> > no longer needed.
> 
> Done. In fact, since we always want to use the icon in the profile directory,
we
> can move it outside the condition.

I disagree, we shouldn't move it outside the if.

In the case where you're deleting down to 1 profile, the code takes care to make
the remaining desktop shortcut identical to a normal non-profile shortcut (i.e.
same state the browser would be in when freshly installed).

Setting an icon on that shortcut goes against that.

https://codereview.chromium.org/14137032/diff/134001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.h (right):

https://codereview.chromium.org/14137032/diff/134001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.h:181: // Sets the icon version in the
prefs on successful icon creation.
Nit: Blank line before the coment.

https://codereview.chromium.org/14137032/diff/134001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/134001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:353: :
profile_path(profile_path_in), create_mode(create_mode_in),
I'm not sure you need the _in suffix for this to work.

https://codereview.chromium.org/14137032/diff/134001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:674:
CreateProfileIcon(profile_path, base::Closure());
Hmm, I would like to see a test that checks that the icon is indeed unbadged
when the first profile is created initially but does get badged when a second
one gets added. Also, that it changes when the avatar changes and becomes
unbadged again when deleting down to one profile.

How about something like this (assuming the created icon file is deterministic):

1. Create an initial profile. Read its icon file into icon_a.
2. Create a second profile with the same avatar as the first one. Read its icon
into icon_b. Read the first profile's icon into icon_a2.
3. Check that icon_b matches icon_a2, check that icon_a doesn't match icon_a2.
4. Change avatar of 2nd profile, read icon into icon_b2 and check that it now
differs from icon_b.
5. Delete 2nd profile, read first profile's icon into icon_a3.
6. Check that icon_a3 matches icon_a (should be the unbadged icon).

In the above, when I say read the icon, just read the file contents into a byte
buffer and compare byte buffer. I think it should be deterministic, unless
there's some non-determinism in Skia's drawing or resizing of images.

https://codereview.chromium.org/14137032/diff/134001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

https://codereview.chromium.org/14137032/diff/134001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.h:79: void
UpdateProfileIcon(const base::FilePath& profile_path);
Add a comment.

[calamity, 2013-06-13 06:26:03]

Why do the taskbar shortcuts need to change when the profile get changed? The
icon will update correctly if it points to this app icon. On profile deletion,
the shortcuts will be deleted via TaskbarUnpinShortcutLink.

There are a few shortcomings (we'll leave the profile command line on shortcuts
that don't strictly require it) but I think that's better than the current
system which leaves inconsistent shortcuts and shortcuts to deleted profiles.

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/120001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:416: if
(!shortcut_icon.empty())
On 2013/06/10 17:44:52, Alexei Svitkine wrote:
> On 2013/06/07 04:26:26, calamity wrote:
> > On 2013/06/05 14:06:03, Alexei Svitkine wrote:
> > > Since you're doing an early return on shortcut_icon.empty() above, this
line
> > is
> > > no longer needed.
> > 
> > Done. In fact, since we always want to use the icon in the profile
directory,
> we
> > can move it outside the condition.
> 
> I disagree, we shouldn't move it outside the if.
> 
> In the case where you're deleting down to 1 profile, the code takes care to
make
> the remaining desktop shortcut identical to a normal non-profile shortcut
(i.e.
> same state the browser would be in when freshly installed).
> 
> Setting an icon on that shortcut goes against that.

Done.

https://codereview.chromium.org/14137032/diff/134001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.h (right):

https://codereview.chromium.org/14137032/diff/134001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.h:181: // Sets the icon version in the
prefs on successful icon creation.
On 2013/06/10 17:44:52, Alexei Svitkine wrote:
> Nit: Blank line before the coment.

Done.

https://codereview.chromium.org/14137032/diff/134001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/134001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:353: :
profile_path(profile_path_in), create_mode(create_mode_in),
On 2013/06/10 17:44:52, Alexei Svitkine wrote:
> I'm not sure you need the _in suffix for this to work.

Done.

https://codereview.chromium.org/14137032/diff/134001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:674:
CreateProfileIcon(profile_path, base::Closure());
On 2013/06/10 17:44:52, Alexei Svitkine wrote:
> Hmm, I would like to see a test that checks that the icon is indeed unbadged
> when the first profile is created initially but does get badged when a second
> one gets added. Also, that it changes when the avatar changes and becomes
> unbadged again when deleting down to one profile.
> 
> How about something like this (assuming the created icon file is
deterministic):
> 
> 1. Create an initial profile. Read its icon file into icon_a.
> 2. Create a second profile with the same avatar as the first one. Read its
icon
> into icon_b. Read the first profile's icon into icon_a2.
> 3. Check that icon_b matches icon_a2, check that icon_a doesn't match icon_a2.
> 4. Change avatar of 2nd profile, read icon into icon_b2 and check that it now
> differs from icon_b.
> 5. Delete 2nd profile, read first profile's icon into icon_a3.
> 6. Check that icon_a3 matches icon_a (should be the unbadged icon).
> 
> In the above, when I say read the icon, just read the file contents into a
byte
> buffer and compare byte buffer. I think it should be deterministic, unless
> there's some non-determinism in Skia's drawing or resizing of images.

Done.

https://codereview.chromium.org/14137032/diff/134001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

https://codereview.chromium.org/14137032/diff/134001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.h:79: void
UpdateProfileIcon(const base::FilePath& profile_path);
On 2013/06/10 17:44:52, Alexei Svitkine wrote:
> Add a comment.

Oops, this isn't needed anymore. Removed.

[Alexei Svitkine (slow), 2013-06-13 13:16:04]

Change looks really good now, just two last comments and then it should be good
to go! Thanks for bearing through all my comments on this.

On 2013/06/13 06:26:03, calamity wrote:
> Why do the taskbar shortcuts need to change when the profile get changed? The
> icon will update correctly if it points to this app icon. On profile deletion,
> the shortcuts will be deleted via TaskbarUnpinShortcutLink.

I don't think we call TaskbarUnpinShortcutLink on profile deletion - I think
that
we found that doesn't work correctly. It unpins all pinned shortcut pointing to
the same exe, even if they have different app ids and there didn't seem to be
a good way to handle that.

At least, that was my understanding per investigation on:

https://code.google.com/p/chromium/issues/detail?id=177490

Though, gab's latest comment on that thread suggests that might have changed
with your recent work, though I'd like to see more concrete evidence for that
(i.e.
when I was testing this originally, I'd be surprised if MigrateChromiumShortcuts
was invalidating my tests - but I guess it's possible).

Anyway, I think we should leave this discussion for a later CL / or on a bug.

https://codereview.chromium.org/14137032/diff/147001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://codereview.chromium.org/14137032/diff/147001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:795:
TEST_F(ProfileShortcutManagerTest, UnbadgeProfileIconOnDeletion) {
Sweet test! Can you also test the avatar change scenario here - that the icon
changes when the avatar does?

https://codereview.chromium.org/14137032/diff/147001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/147001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:719:
CreateProfileIcon(profile_path, base::Closure());
Hmm, the header file has the comment "This will not overwrite an already
existing profile icon." for the function you're calling here. That seems to be
inconsistent with how you're using it, since it seems like you're updating it
here.

If updating works fine, then please update the comment and maybe rename the
function to CreateOrUpdateProfileIcon (and maybe same thing for CREATE_ICON_ONLY
enum).

[calamity, 2013-06-14 04:09:05]

Yeah, MigrateChromiumShortcuts was changing the desktop shortcut, which you
would have then pinned, causing unpin to wreak havok.

https://codereview.chromium.org/14137032/diff/147001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://codereview.chromium.org/14137032/diff/147001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:795:
TEST_F(ProfileShortcutManagerTest, UnbadgeProfileIconOnDeletion) {
On 2013/06/13 13:16:05, Alexei Svitkine wrote:
> Sweet test! Can you also test the avatar change scenario here - that the icon
> changes when the avatar does?

Done.

https://codereview.chromium.org/14137032/diff/147001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/147001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:719:
CreateProfileIcon(profile_path, base::Closure());
On 2013/06/13 13:16:05, Alexei Svitkine wrote:
> Hmm, the header file has the comment "This will not overwrite an already
> existing profile icon." for the function you're calling here. That seems to be
> inconsistent with how you're using it, since it seems like you're updating it
> here.
> 
> If updating works fine, then please update the comment and maybe rename the
> function to CreateOrUpdateProfileIcon (and maybe same thing for
CREATE_ICON_ONLY
> enum).

Done.

[Alexei Svitkine (slow), 2013-06-14 04:47:12]

LGTM! Thanks!

[sail, 2013-06-14 17:15:39]

Some high level comments:
  - the icon code should probably live else where
  - no ifdefs please

https://codereview.chromium.org/14137032/diff/155001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/14137032/diff/155001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:142: COMPILE_ASSERT(sizeof(ProfileImpl)
<= 744u, profile_impl_size_unexpected);
weak_ptr_factory_ will trigger this assert

https://codereview.chromium.org/14137032/diff/155001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:741: #if defined(OS_WIN)
The profile icon code doesn't really belong in this class.
The #ifdef is a good sign of this.

https://codereview.chromium.org/14137032/diff/155001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/155001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:49: const char
kProfileIconFileName[] = "Google Profile.ico";
Can we come up with a better name? Maybe SHORT_PRODUCT_NAME + Profile.ico?

https://codereview.chromium.org/14137032/diff/155001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/14137032/diff/155001/chrome/common/pref_names...
chrome/common/pref_names.h:33: #if defined(OS_WIN)
don't need ifdef

[calamity, 2013-06-17 07:29:30]

https://codereview.chromium.org/14137032/diff/155001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/14137032/diff/155001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:741: #if defined(OS_WIN)
On 2013/06/14 17:15:39, sail wrote:
> The profile icon code doesn't really belong in this class.
> The #ifdef is a good sign of this.

Any suggestions about where to put it then?

I had originally thought to put it in ProfileManager::OnProfileCreated but it
doesn't get called on the profile created by StartupBrowserImpl (as
ProfileManager::CreateProfileHelper passes a NULL delegate). Any idea why it
does this?

https://codereview.chromium.org/14137032/diff/155001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/155001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:49: const char
kProfileIconFileName[] = "Google Profile.ico";
On 2013/06/14 17:15:39, sail wrote:
> Can we come up with a better name? Maybe SHORT_PRODUCT_NAME + Profile.ico?

This icon file already exists and could already have shortcuts pointing to it. I
don't think it would be a good idea to change this file name.

[sail, 2013-06-17 19:51:02]

https://codereview.chromium.org/14137032/diff/155001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/14137032/diff/155001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:741: #if defined(OS_WIN)
On 2013/06/17 07:29:30, calamity wrote:
> On 2013/06/14 17:15:39, sail wrote:
> > The profile icon code doesn't really belong in this class.
> > The #ifdef is a good sign of this.
> 
> Any suggestions about where to put it then?
> 
> I had originally thought to put it in ProfileManager::OnProfileCreated but it
> doesn't get called on the profile created by StartupBrowserImpl (as
> ProfileManager::CreateProfileHelper passes a NULL delegate). Any idea why it
> does this?
> 

ProfileManager is not correct either.

How about listening to chrome::NOTIFICATION_PROFILE_CREATED from your code?

If you must directly call your code form here then do what we always do when we
have platform specific code:
  - add a cross platform API
  - implement the API for your platform
  - add stubs for other platforms
  - call your new API from here

https://codereview.chromium.org/14137032/diff/155001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/155001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:49: const char
kProfileIconFileName[] = "Google Profile.ico";
On 2013/06/17 07:29:30, calamity wrote:
> On 2013/06/14 17:15:39, sail wrote:
> > Can we come up with a better name? Maybe SHORT_PRODUCT_NAME + Profile.ico?
> 
> This icon file already exists and could already have shortcuts pointing to it.
I
> don't think it would be a good idea to change this file name.

Gah, that's really unfortunate.

[gab, 2013-06-17 23:06:16]

Just back from vacation, let me know where you are ready for me to take a look.

Cheers,
Gab

[calamity, 2013-06-18 04:41:30]

Hey gab, could you also take a look at this?

https://codereview.chromium.org/14137032/diff/155001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/14137032/diff/155001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:741: #if defined(OS_WIN)
On 2013/06/17 19:51:03, sail wrote:
> On 2013/06/17 07:29:30, calamity wrote:
> > On 2013/06/14 17:15:39, sail wrote:
> > > The profile icon code doesn't really belong in this class.
> > > The #ifdef is a good sign of this.
> > 
> > Any suggestions about where to put it then?
> > 
> > I had originally thought to put it in ProfileManager::OnProfileCreated but
it
> > doesn't get called on the profile created by StartupBrowserImpl (as
> > ProfileManager::CreateProfileHelper passes a NULL delegate). Any idea why it
> > does this?
> > 
> 
> ProfileManager is not correct either.
> 
> How about listening to chrome::NOTIFICATION_PROFILE_CREATED from your code?
> 
> If you must directly call your code form here then do what we always do when
we
> have platform specific code:
>   - add a cross platform API
>   - implement the API for your platform
>   - add stubs for other platforms
>   - call your new API from here

Listened for notification from profile_shortcut_manager.

https://codereview.chromium.org/14137032/diff/155001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/14137032/diff/155001/chrome/common/pref_names...
chrome/common/pref_names.h:33: #if defined(OS_WIN)
On 2013/06/14 17:15:39, sail wrote:
> don't need ifdef

Done.

[sail, 2013-06-18 05:10:20]

lgtm

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:815: NOTREACHED();
break;?

[Alexei Svitkine (slow), 2013-06-18 15:27:19]

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:214: void
OnProfileIconCreateSuccess(Profile* profile) {
Nit: Add a comment.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:647:
registrar_.Add(this, chrome::NOTIFICATION_PROFILE_CREATED,
Add a comment to explain this is for profile icon creation.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:801: switch (type) {
How about:

if (type != chrome::NOTIFICATION_PROFILE_CREATED) {
  NOTREACHED();
  return;
}

... rest of the code follows.

Makes it more readable by avoiding extra nesting.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:805: if
(profile->GetPrefs()->GetInteger(prefs::kProfileIconVersion) <
I suggest moving this to another anonymous function next to
OnProfileIconCreateSuccess() that takes |profile| as a param.

That way, the two pieces of code that deal with profile icon version pref are
together.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.h:77: //
content::NotificationObserver:
Nit: to be consistent with comments above, change to:

"// content::NotificationObserver implementation:"

[gab, 2013-06-18 15:36:58]

Comments below.

Cheers!
Gab

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile.cc:109: 0,
Why is this an integer? As it is right now it is used as boolean (i.e. only
checked on profile creation).

Having it be an integer only feels right if there is code to update the icon if
kCurrentProfileIconVersion is higher than prefs::kProfileIconVersion when Chrome
starts or something (see my comment on ProfileShortcutManagerWin::Observe).

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (left):

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:560:
!CommandLine::ForCurrentProcess()->HasSwitch(switches::kShowAppList);
Can this be done in a precursor CL? I feel this one-liner needs to be considered
on its own instead of slammed in this big CL where it might have gone unnoticed.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:154: // |is_create|
should be true if we are creating the icon at a time when it
There is no such |is_create| parameter..?

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:163: const
base::Closure& callback) {
Explain what is |callback| in the method comment above.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:167: return
base::FilePath();
Also add to the method comment about what it returns generally and in failure
cases.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:204:
SHChangeNotify(SHCNE_ASSOCCHANGED, SHCNF_IDLIST | SHCNF_FLUSHNOWAIT,
I don't think SHCNF_FLUSHNOWAIT is necessary here, it is not used in similar
code
(https://code.google.com/p/chromium/codesearch#chromium/src/base/win/shortcut....)
and I have had trouble with SHCNF_FLUSHNOWAIT in the past
(https://chromiumcodereview.appspot.com/10692138).

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:216:
kCurrentProfileIconVersion);
In the current design, this will never be tried again, see my suggestion on
ProfileShortcutManagerWin::Observe.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:359: struct
CreateOrUpdateShortcutsParams {
How about adding the callback to this list as well (instead of forcing most
callers to pass a null-callback)?

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:384: // be updated is
specified by |action|. Must be called on the FILE thread.
This comment now refers to parameters that were moved to
CreateOrUpdateShortcutsParams, please move the comments to their respective
members as you see fit.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:384: // be updated is
specified by |action|. Must be called on the FILE thread.
Add a param comment for |callback|.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:391:
CreateOrUpdateShortcutIconForProfile(params.profile_path,
Shouldn't this only be called if the pref is < than kCurrentProfileIconVersion?

In fact, if we could guarantee the icon exists for all profiles (which we kind
of need actually); then we never need to call this here, no?

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:632:
!CommandLine::ForCurrentProcess()->HasSwitch(switches::kUserDataDir);
I still don't like that this is disabled for --user-data-dir :(. This introduces
a regression since badged icons worked with --user-data-dir prior to this CL...

(well... this CL itself doesn't introduce a regression yet since only desktop
profile shortcuts use the new profile icon for now, but the goal should be that
all badged shortcuts use this icon (taskbar shortcut included) in the near
future or the complexity in this CL is pointless imo).

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:806:
kCurrentProfileIconVersion) {
I think this condition needs to be check every time the profile loads
(NOTIFICATION_PROFILE_URL_REQUEST_CONTEXT_GETTER_INITIALIZED???); otherwise a
developer might be fooled into thinking that upping kCurrentProfileIconVersion
is sufficient to force an update which it is not with the current code.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.h:37: // Specifies whether
only the existing shortcuts should be updated, a new
s/shortcuts/shortcut here right?

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.h:57: const base::Closure&
callback) OVERRIDE;
#include "base/callback.h"

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.h:87: void
CreateOrUpdateShortcutsForProfileAtPath(
Add a method comment here.

https://codereview.chromium.org/14137032/diff/172001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/14137032/diff/172001/chrome/common/pref_names...
chrome/common/pref_names.h:33: extern const char kProfileIconVersion[];
Seems all but kRestoreOnStartupMigrated and your new pref are in alphabetical
order here, please restore this state.

[Alexei Svitkine (slow), 2013-06-18 15:44:31]

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile.cc:109: 0,
On 2013/06/18 15:36:58, gab wrote:
> Why is this an integer? As it is right now it is used as boolean (i.e. only
> checked on profile creation).
> 
> Having it be an integer only feels right if there is code to update the icon
if
> kCurrentProfileIconVersion is higher than prefs::kProfileIconVersion when
Chrome
> starts or something (see my comment on ProfileShortcutManagerWin::Observe).

See previous discussion. Having it an integer is more future proof, if we later
need to update existing icons for some reason (e.g. Chrome's icon changes)
rather than adding a whole new pref at that point.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:647:
registrar_.Add(this, chrome::NOTIFICATION_PROFILE_CREATED,
On 2013/06/18 15:27:19, Alexei Svitkine wrote:
> Add a comment to explain this is for profile icon creation.

Hmm, is this notified only when a profile gets _created_ or loaded? We want it
on load. (And I know the profile manager code uses "create" inconsistently. :\)

[gab, 2013-06-18 15:51:38]

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile.cc:109: 0,
On 2013/06/18 15:44:32, Alexei Svitkine wrote:
> On 2013/06/18 15:36:58, gab wrote:
> > Why is this an integer? As it is right now it is used as boolean (i.e. only
> > checked on profile creation).
> > 
> > Having it be an integer only feels right if there is code to update the icon
> if
> > kCurrentProfileIconVersion is higher than prefs::kProfileIconVersion when
> Chrome
> > starts or something (see my comment on ProfileShortcutManagerWin::Observe).
> 
> See previous discussion. Having it an integer is more future proof, if we
later
> need to update existing icons for some reason (e.g. Chrome's icon changes)
> rather than adding a whole new pref at that point.

Right, makes sense, I think it's pretty easy to add such update code now though
(see my comment on ProfileShortcutManagerWin::Observe) which would prevent
somehow from assuming upping kCurrentProfileIconVersion is sufficient when it is
not.

[Alexei Svitkine (slow), 2013-06-26 18:50:33]

Hey, just wondering what's the status of this?

I think it just needs to be updated to have the icon creation check trigger on
profile load rather than profile creation...

[calamity, 2013-06-27 08:10:10]

New CL created here: https://codereview.chromium.org/17993005/ .

I think this CL can be landed independently of that one.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (left):

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:560:
!CommandLine::ForCurrentProcess()->HasSwitch(switches::kShowAppList);
On 2013/06/18 15:36:58, gab wrote:
> Can this be done in a precursor CL? I feel this one-liner needs to be
considered
> on its own instead of slammed in this big CL where it might have gone
unnoticed.

Done.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:154: // |is_create|
should be true if we are creating the icon at a time when it
On 2013/06/18 15:36:58, gab wrote:
> There is no such |is_create| parameter..?

Done.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:163: const
base::Closure& callback) {
On 2013/06/18 15:36:58, gab wrote:
> Explain what is |callback| in the method comment above.

Done.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:167: return
base::FilePath();
On 2013/06/18 15:36:58, gab wrote:
> Also add to the method comment about what it returns generally and in failure
> cases.

Done.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:204:
SHChangeNotify(SHCNE_ASSOCCHANGED, SHCNF_IDLIST | SHCNF_FLUSHNOWAIT,
On 2013/06/18 15:36:58, gab wrote:
> I don't think SHCNF_FLUSHNOWAIT is necessary here, it is not used in similar
> code
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/win/shortcut....)
> and I have had trouble with SHCNF_FLUSHNOWAIT in the past
> (https://chromiumcodereview.appspot.com/10692138).

Done.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:214: void
OnProfileIconCreateSuccess(Profile* profile) {
On 2013/06/18 15:27:19, Alexei Svitkine wrote:
> Nit: Add a comment.

Done.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:359: struct
CreateOrUpdateShortcutsParams {
On 2013/06/18 15:36:58, gab wrote:
> How about adding the callback to this list as well (instead of forcing most
> callers to pass a null-callback)?

This doesn't avoid needing the parameter in
CreateOrUpdateShortcutsForProfileAtPath() where the null-callbacks used.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:384: // be updated is
specified by |action|. Must be called on the FILE thread.
On 2013/06/18 15:36:58, gab wrote:
> This comment now refers to parameters that were moved to
> CreateOrUpdateShortcutsParams, please move the comments to their respective
> members as you see fit.

Done.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:384: // be updated is
specified by |action|. Must be called on the FILE thread.
On 2013/06/18 15:36:58, gab wrote:
> Add a param comment for |callback|.

Done.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:391:
CreateOrUpdateShortcutIconForProfile(params.profile_path,
On 2013/06/18 15:36:58, gab wrote:
> Shouldn't this only be called if the pref is < than
kCurrentProfileIconVersion?
> 
> In fact, if we could guarantee the icon exists for all profiles (which we kind
> of need actually); then we never need to call this here, no?

This is the codepath that CreateOrUpdateProfileIcon() takes and the only call to
CreateOrUpdateShortcutIconForProfile() which does the icon creation. Having the
icon and shortcut create asynchronously with each other could create an issue
where the wrong SHChangeNotify happens (e.g on shortcut creation: shortcut
created, icon created, SCHNE_CREATE). Also, this is the codepath taken to update
the avatar.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:632:
!CommandLine::ForCurrentProcess()->HasSwitch(switches::kUserDataDir);
On 2013/06/18 15:36:58, gab wrote:
> I still don't like that this is disabled for --user-data-dir :(. This
introduces
> a regression since badged icons worked with --user-data-dir prior to this
CL...
> 
> (well... this CL itself doesn't introduce a regression yet since only desktop
> profile shortcuts use the new profile icon for now, but the goal should be
that
> all badged shortcuts use this icon (taskbar shortcut included) in the near
> future or the complexity in this CL is pointless imo).

This isn't a problem. Once relaunch details on browser windows is set to point
at the app icon, the user data dir icons will be fine.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:647:
registrar_.Add(this, chrome::NOTIFICATION_PROFILE_CREATED,
On 2013/06/18 15:44:32, Alexei Svitkine wrote:
> On 2013/06/18 15:27:19, Alexei Svitkine wrote:
> > Add a comment to explain this is for profile icon creation.
> 
> Hmm, is this notified only when a profile gets _created_ or loaded? We want it
> on load. (And I know the profile manager code uses "create" inconsistently.
:\)

I'm fairly sure it's on load. It's in profile_impl.cc.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:801: switch (type) {
On 2013/06/18 15:27:19, Alexei Svitkine wrote:
> How about:
> 
> if (type != chrome::NOTIFICATION_PROFILE_CREATED) {
>   NOTREACHED();
>   return;
> }
> 
> ... rest of the code follows.
> 
> Makes it more readable by avoiding extra nesting.

Other files such as:
-chrome/browser/android/chrome_web_contents_delegate_android.cc
-chrome/browser/bookmarks/bookmark_model.cc

follow this pattern for single case Observe implementations. I think it would be
better to maintain the pattern.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:805: if
(profile->GetPrefs()->GetInteger(prefs::kProfileIconVersion) <
On 2013/06/18 15:27:19, Alexei Svitkine wrote:
> I suggest moving this to another anonymous function next to
> OnProfileIconCreateSuccess() that takes |profile| as a param.
> 
> That way, the two pieces of code that deal with profile icon version pref are
> together.

Done.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:806:
kCurrentProfileIconVersion) {
On 2013/06/18 15:36:58, gab wrote:
> I think this condition needs to be check every time the profile loads
> (NOTIFICATION_PROFILE_URL_REQUEST_CONTEXT_GETTER_INITIALIZED???); otherwise a
> developer might be fooled into thinking that upping kCurrentProfileIconVersion
> is sufficient to force an update which it is not with the current code.

NOTIFICATION_PROFILE_CREATED was recommended by sail@ above. Despite its name, I
think it's actually the profile load notification.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:815: NOTREACHED();
On 2013/06/18 05:10:21, sail wrote:
> break;?

Done.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.h:37: // Specifies whether
only the existing shortcuts should be updated, a new
On 2013/06/18 15:36:58, gab wrote:
> s/shortcuts/shortcut here right?

Done.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.h:57: const base::Closure&
callback) OVERRIDE;
On 2013/06/18 15:36:58, gab wrote:
> #include "base/callback.h"

Done.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.h:77: //
content::NotificationObserver:
On 2013/06/18 15:27:19, Alexei Svitkine wrote:
> Nit: to be consistent with comments above, change to:
> 
> "// content::NotificationObserver implementation:"

Done.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.h:87: void
CreateOrUpdateShortcutsForProfileAtPath(
On 2013/06/18 15:36:58, gab wrote:
> Add a method comment here.

Done.

https://codereview.chromium.org/14137032/diff/172001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/14137032/diff/172001/chrome/common/pref_names...
chrome/common/pref_names.h:33: extern const char kProfileIconVersion[];
On 2013/06/18 15:36:58, gab wrote:
> Seems all but kRestoreOnStartupMigrated and your new pref are in alphabetical
> order here, please restore this state.

Done.

[gab, 2013-06-27 14:18:56]

Looking good, a few last comments.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:632:
!CommandLine::ForCurrentProcess()->HasSwitch(switches::kUserDataDir);
On 2013/06/27 08:10:11, calamity wrote:
> On 2013/06/18 15:36:58, gab wrote:
> > I still don't like that this is disabled for --user-data-dir :(. This
> introduces
> > a regression since badged icons worked with --user-data-dir prior to this
> CL...
> > 
> > (well... this CL itself doesn't introduce a regression yet since only
desktop
> > profile shortcuts use the new profile icon for now, but the goal should be
> that
> > all badged shortcuts use this icon (taskbar shortcut included) in the near
> > future or the complexity in this CL is pointless imo).
> 
> This isn't a problem. Once relaunch details on browser windows is set to point
> at the app icon, the user data dir icons will be fine.

Ah, so the plan is to keep the runtime badging code around? Doesn't that create
situations where we end up badging an already badged icon (perhaps breaking some
transparency properties of the badge)?

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:647:
registrar_.Add(this, chrome::NOTIFICATION_PROFILE_CREATED,
On 2013/06/27 08:10:11, calamity wrote:
> On 2013/06/18 15:44:32, Alexei Svitkine wrote:
> > On 2013/06/18 15:27:19, Alexei Svitkine wrote:
> > > Add a comment to explain this is for profile icon creation.
> > 
> > Hmm, is this notified only when a profile gets _created_ or loaded? We want
it
> > on load. (And I know the profile manager code uses "create" inconsistently.
> :\)
> 
> I'm fairly sure it's on load. It's in profile_impl.cc.

Ah ok I see, it seems to be when the Profile object is created, not when the
profile itself is created.

You can easily confirm this by adding a NOTREACHED() here in your local build
and make sure you're hitting when chrome loads even if you already have a
profile on disk.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:661:
CREATE_OR_UPDATE_ICON_ONLY,
Instead of introducing CREATE_OR_UPDATE_ICON_ONLY for this use case only, why
not call CreateOrUpdateShortcutIconForProfile() directly here?

Doesn't seem to me like CreateOrUpdateShortcutsForProfileAtPath(0 does any work
before making this call which we would need to duplicate here...

It makes this method more straight forward imo and helps making the interface
simpler by not adding CREATE_OR_UPDATE_ICON_ONLY.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:801: switch (type) {
On 2013/06/27 08:10:11, calamity wrote:
> On 2013/06/18 15:27:19, Alexei Svitkine wrote:
> > How about:
> > 
> > if (type != chrome::NOTIFICATION_PROFILE_CREATED) {
> >   NOTREACHED();
> >   return;
> > }
> > 
> > ... rest of the code follows.
> > 
> > Makes it more readable by avoiding extra nesting.
> 
> Other files such as:
> -chrome/browser/android/chrome_web_contents_delegate_android.cc
> -chrome/browser/bookmarks/bookmark_model.cc
> 
> follow this pattern for single case Observe implementations. I think it would
be
> better to maintain the pattern.

Yea, I like the switch personally and have seen this pattern all over chrome.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:802: case
chrome::NOTIFICATION_PROFILE_CREATED: {
Add a comment above this stipulating your findings on when this gets called
exactly.

https://codereview.chromium.org/14137032/diff/190001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/190001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:223:
ProfileShortcutManager* profile_shortcut_manager) {
Make this is a private method of ProfileShortcutManager instead of making it a
free-function that takes a ProfileShortcutManager*, but then that defeats the
purpose of Alexei's suggestion of putting this here because it brings it beside
OnProfileIconCreateSuccess()...

Personally I was fine with having this code inline in
ProfileShortcutManagerWin::Observe().

[Alexei Svitkine (slow), 2013-06-27 15:53:07]

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:632:
!CommandLine::ForCurrentProcess()->HasSwitch(switches::kUserDataDir);
On 2013/06/27 14:18:56, gab wrote:
> On 2013/06/27 08:10:11, calamity wrote:
> > On 2013/06/18 15:36:58, gab wrote:
> > > I still don't like that this is disabled for --user-data-dir :(. This
> > introduces
> > > a regression since badged icons worked with --user-data-dir prior to this
> > CL...
> > > 
> > > (well... this CL itself doesn't introduce a regression yet since only
> desktop
> > > profile shortcuts use the new profile icon for now, but the goal should be
> > that
> > > all badged shortcuts use this icon (taskbar shortcut included) in the near
> > > future or the complexity in this CL is pointless imo).
> > 
> > This isn't a problem. Once relaunch details on browser windows is set to
point
> > at the app icon, the user data dir icons will be fine.
> 
> Ah, so the plan is to keep the runtime badging code around? Doesn't that
create
> situations where we end up badging an already badged icon (perhaps breaking
some
> transparency properties of the badge)?

I think this discussion is outside the scope of this CL, so perhaps it should be
taken to one of the relevant bugs. (FWIW I think once we set the relaunch
details, we can stop doing the runtime badging.)

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:661:
CREATE_OR_UPDATE_ICON_ONLY,
On 2013/06/27 14:18:56, gab wrote:
> Instead of introducing CREATE_OR_UPDATE_ICON_ONLY for this use case only, why
> not call CreateOrUpdateShortcutIconForProfile() directly here?
> 
> Doesn't seem to me like CreateOrUpdateShortcutsForProfileAtPath(0 does any
work
> before making this call which we would need to duplicate here...
> 
> It makes this method more straight forward imo and helps making the interface
> simpler by not adding CREATE_OR_UPDATE_ICON_ONLY.

Seems like a reasonable suggestion to me, just add a BrowserThread::PostTask()
to call CreateOrUpdateShortcutIconForProfile with the callback and remove the
CREATE_OR_UPDATE_ICON_ONLY complexity.

Then, CreateOrUpdateDesktopShortcutsAndIconForProfile() doesn't need to take a
callback param and can just pass a base::Closure() when calling
CreateOrUpdateShortcutIconForProfile() from it.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:801: switch (type) {
On 2013/06/27 14:18:56, gab wrote:
> On 2013/06/27 08:10:11, calamity wrote:
> > On 2013/06/18 15:27:19, Alexei Svitkine wrote:
> > > How about:
> > > 
> > > if (type != chrome::NOTIFICATION_PROFILE_CREATED) {
> > >   NOTREACHED();
> > >   return;
> > > }
> > > 
> > > ... rest of the code follows.
> > > 
> > > Makes it more readable by avoiding extra nesting.
> > 
> > Other files such as:
> > -chrome/browser/android/chrome_web_contents_delegate_android.cc
> > -chrome/browser/bookmarks/bookmark_model.cc
> > 
> > follow this pattern for single case Observe implementations. I think it
would
> be
> > better to maintain the pattern.
> 
> Yea, I like the switch personally and have seen this pattern all over chrome.

Fair enough, keep the switch then. :)

https://codereview.chromium.org/14137032/diff/190001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/190001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:223:
ProfileShortcutManager* profile_shortcut_manager) {
On 2013/06/27 14:18:56, gab wrote:
> Make this is a private method of ProfileShortcutManager instead of making it a
> free-function that takes a ProfileShortcutManager*, but then that defeats the
> purpose of Alexei's suggestion of putting this here because it brings it
beside
> OnProfileIconCreateSuccess()...

Good point. How about making both this and OnProfileIconCreateSuccess() private
functions of ProfileShortcutManagerWin? This should keep the code next to each
other and avoid this issue of passing the manager to an anon function.

[gab, 2013-06-27 17:55:43]

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:632:
!CommandLine::ForCurrentProcess()->HasSwitch(switches::kUserDataDir);
On 2013/06/27 15:53:08, Alexei Svitkine wrote:
> On 2013/06/27 14:18:56, gab wrote:
> > On 2013/06/27 08:10:11, calamity wrote:
> > > On 2013/06/18 15:36:58, gab wrote:
> > > > I still don't like that this is disabled for --user-data-dir :(. This
> > > introduces
> > > > a regression since badged icons worked with --user-data-dir prior to
this
> > > CL...
> > > > 
> > > > (well... this CL itself doesn't introduce a regression yet since only
> > desktop
> > > > profile shortcuts use the new profile icon for now, but the goal should
be
> > > that
> > > > all badged shortcuts use this icon (taskbar shortcut included) in the
near
> > > > future or the complexity in this CL is pointless imo).
> > > 
> > > This isn't a problem. Once relaunch details on browser windows is set to
> point
> > > at the app icon, the user data dir icons will be fine.
> > 
> > Ah, so the plan is to keep the runtime badging code around? Doesn't that
> create
> > situations where we end up badging an already badged icon (perhaps breaking
> some
> > transparency properties of the badge)?
> 
> I think this discussion is outside the scope of this CL, so perhaps it should
be
> taken to one of the relevant bugs. (FWIW I think once we set the relaunch
> details, we can stop doing the runtime badging.)

Right, what I meant is that if the relaunch details point to this new icon which
only exists for the default user-data-dir, then removing badging means breaking
profile taskbar icons for non-default user-data dirs, while not removing badging
means double-badging of the default user-data-dir's icons (I don't think we want
to keep code that is conditional on running in the non-default user-data-dir
either...).

This CL is only really useful if it can be used to set the relaunch details
properly later, no?

I think it's worth revisiting (in a follow-up CL) making ProfileShortcutManager
work (maybe at least to create profile icons) for non-default user-data-dirs...

https://codereview.chromium.org/14137032/diff/190001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/190001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:223:
ProfileShortcutManager* profile_shortcut_manager) {
On 2013/06/27 15:53:08, Alexei Svitkine wrote:
> On 2013/06/27 14:18:56, gab wrote:
> > Make this is a private method of ProfileShortcutManager instead of making it
a
> > free-function that takes a ProfileShortcutManager*, but then that defeats
the
> > purpose of Alexei's suggestion of putting this here because it brings it
> beside
> > OnProfileIconCreateSuccess()...
> 
> Good point. How about making both this and OnProfileIconCreateSuccess()
private
> functions of ProfileShortcutManagerWin? This should keep the code next to each
> other and avoid this issue of passing the manager to an anon function.

Making both private methods SGTM.

[calamity, 2013-07-05 08:04:32]

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:661:
CREATE_OR_UPDATE_ICON_ONLY,
On 2013/06/27 15:53:08, Alexei Svitkine wrote:
> On 2013/06/27 14:18:56, gab wrote:
> > Instead of introducing CREATE_OR_UPDATE_ICON_ONLY for this use case only,
why
> > not call CreateOrUpdateShortcutIconForProfile() directly here?
> > 
> > Doesn't seem to me like CreateOrUpdateShortcutsForProfileAtPath(0 does any
> work
> > before making this call which we would need to duplicate here...
> > 
> > It makes this method more straight forward imo and helps making the
interface
> > simpler by not adding CREATE_OR_UPDATE_ICON_ONLY.
> 
> Seems like a reasonable suggestion to me, just add a BrowserThread::PostTask()
> to call CreateOrUpdateShortcutIconForProfile with the callback and remove the
> CREATE_OR_UPDATE_ICON_ONLY complexity.
> 
> Then, CreateOrUpdateDesktopShortcutsAndIconForProfile() doesn't need to take a
> callback param and can just pass a base::Closure() when calling
> CreateOrUpdateShortcutIconForProfile() from it.

I'll still need to get the avatar resources...? I'll get that code and factor it
out into a function in the anonymous namespace then I guess.

https://codereview.chromium.org/14137032/diff/190001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/190001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:223:
ProfileShortcutManager* profile_shortcut_manager) {
On 2013/06/27 17:55:44, gab wrote:
> On 2013/06/27 15:53:08, Alexei Svitkine wrote:
> > On 2013/06/27 14:18:56, gab wrote:
> > > Make this is a private method of ProfileShortcutManager instead of making
it
> a
> > > free-function that takes a ProfileShortcutManager*, but then that defeats
> the
> > > purpose of Alexei's suggestion of putting this here because it brings it
> > beside
> > > OnProfileIconCreateSuccess()...
> > 
> > Good point. How about making both this and OnProfileIconCreateSuccess()
> private
> > functions of ProfileShortcutManagerWin? This should keep the code next to
each
> > other and avoid this issue of passing the manager to an anon function.
> 
> Making both private methods SGTM.

Since everything's in this file now, I can probably just get rid of the callback
altogether actually. I'll need a Profile* which I can get in
CreateOrUpdateShortcutsForProfileAtPath() and pass it through to
CreateOrUpdateDesktopShortcutsAndIconForProfile().

WDYT?

I think making private methods as callbacks is incredibly messy as you have to
add a weak_ptr_factory? From what I've seen from the rest of chrome, generally
functions in the anonymous namespace are used as callbacks and their callers are
separated and down in a method.

[gab, 2013-07-05 13:47:23]

Almost there :)!

Question for asvitkine and sail below.

Cheers!
Gab

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:647:
registrar_.Add(this, chrome::NOTIFICATION_PROFILE_CREATED,
On 2013/06/27 14:18:56, gab wrote:
> On 2013/06/27 08:10:11, calamity wrote:
> > On 2013/06/18 15:44:32, Alexei Svitkine wrote:
> > > On 2013/06/18 15:27:19, Alexei Svitkine wrote:
> > > > Add a comment to explain this is for profile icon creation.
> > > 
> > > Hmm, is this notified only when a profile gets _created_ or loaded? We
want
> it
> > > on load. (And I know the profile manager code uses "create"
inconsistently.
> > :\)
> > 
> > I'm fairly sure it's on load. It's in profile_impl.cc.
> 
> Ah ok I see, it seems to be when the Profile object is created, not when the
> profile itself is created.
> 
> You can easily confirm this by adding a NOTREACHED() here in your local build
> and make sure you're hitting when chrome loads even if you already have a
> profile on disk.

Did you try this to confirm?

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:661:
CREATE_OR_UPDATE_ICON_ONLY,
On 2013/07/05 08:04:33, calamity wrote:
> On 2013/06/27 15:53:08, Alexei Svitkine wrote:
> > On 2013/06/27 14:18:56, gab wrote:
> > > Instead of introducing CREATE_OR_UPDATE_ICON_ONLY for this use case only,
> why
> > > not call CreateOrUpdateShortcutIconForProfile() directly here?
> > > 
> > > Doesn't seem to me like CreateOrUpdateShortcutsForProfileAtPath(0 does any
> > work
> > > before making this call which we would need to duplicate here...
> > > 
> > > It makes this method more straight forward imo and helps making the
> interface
> > > simpler by not adding CREATE_OR_UPDATE_ICON_ONLY.
> > 
> > Seems like a reasonable suggestion to me, just add a
BrowserThread::PostTask()
> > to call CreateOrUpdateShortcutIconForProfile with the callback and remove
the
> > CREATE_OR_UPDATE_ICON_ONLY complexity.
> > 
> > Then, CreateOrUpdateDesktopShortcutsAndIconForProfile() doesn't need to take
a
> > callback param and can just pass a base::Closure() when calling
> > CreateOrUpdateShortcutIconForProfile() from it.
> 
> I'll still need to get the avatar resources...? I'll get that code and factor
it
> out into a function in the anonymous namespace then I guess.

Ah, yes, my bad I hadn't noticed this called into
CreateOrUpdateShortcutsForProfileAtPath() somehow thought it called straight
into CreateOrUpdateDesktopShortcutsAndIconForProfile() -- gah, these names are
all alike..!

By looking at it some more, it feels like the avatar resources need some logic
only available on the UI thread (e.g., ProfileInfoCache) to determine which icon
they should use exactly before copying the resources to make them available to
the FILE thread.
This is not trivial and I no longer think we should change this code here (we
could perhaps refactor this later if needed, but I think this is beyond the
scope of this CL -- unless you guys see something trivial which I don't?).

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:802: case
chrome::NOTIFICATION_PROFILE_CREATED: {
On 2013/06/27 14:18:56, gab wrote:
> Add a comment above this stipulating your findings on when this gets called
> exactly.

Ping on this -- mostly for my sanity as I don't like to keep track of multiple
unfinished review cycles :).

https://codereview.chromium.org/14137032/diff/190001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/190001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:223:
ProfileShortcutManager* profile_shortcut_manager) {
On 2013/07/05 08:04:33, calamity wrote:
> On 2013/06/27 17:55:44, gab wrote:
> > On 2013/06/27 15:53:08, Alexei Svitkine wrote:
> > > On 2013/06/27 14:18:56, gab wrote:
> > > > Make this is a private method of ProfileShortcutManager instead of
making
> it
> > a
> > > > free-function that takes a ProfileShortcutManager*, but then that
defeats
> > the
> > > > purpose of Alexei's suggestion of putting this here because it brings it
> > > beside
> > > > OnProfileIconCreateSuccess()...
> > > 
> > > Good point. How about making both this and OnProfileIconCreateSuccess()
> > private
> > > functions of ProfileShortcutManagerWin? This should keep the code next to
> each
> > > other and avoid this issue of passing the manager to an anon function.
> > 
> > Making both private methods SGTM.
> 
> Since everything's in this file now, I can probably just get rid of the
callback
> altogether actually. I'll need a Profile* which I can get in
> CreateOrUpdateShortcutsForProfileAtPath() and pass it through to
> CreateOrUpdateDesktopShortcutsAndIconForProfile().
> 
> WDYT?

I think that could work, however I don't think you can use Profile* on the FILE
thread (this is also true for the current code -- had missed that in previous
reviews).

I hear you can still set profile prefs from other threads than the UI thread via
some helpers in base/prefs/..., but I do not know where they are, now have I
used them myself before...

Perhaps asvitkine/sail would know more?

> 
> I think making private methods as callbacks is incredibly messy as you have to
> add a weak_ptr_factory? From what I've seen from the rest of chrome, generally
> functions in the anonymous namespace are used as callbacks and their callers
are
> separated and down in a method.

True :(.


Either way, some explicit thread checks on these two anonymous methods would be
nice to show exactly which thread you expect them to run on.


PS: If you do manage to get rid of the callback, I'm all for simply inlining
UpdateProfileIconIfNecessary() below.

[Alexei Svitkine (slow), 2013-07-05 15:14:24]

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:661:
CREATE_OR_UPDATE_ICON_ONLY,
On 2013/07/05 13:47:24, gab wrote:
> On 2013/07/05 08:04:33, calamity wrote:
> > On 2013/06/27 15:53:08, Alexei Svitkine wrote:
> > > On 2013/06/27 14:18:56, gab wrote:
> > > > Instead of introducing CREATE_OR_UPDATE_ICON_ONLY for this use case
only,
> > why
> > > > not call CreateOrUpdateShortcutIconForProfile() directly here?
> > > > 
> > > > Doesn't seem to me like CreateOrUpdateShortcutsForProfileAtPath(0 does
any
> > > work
> > > > before making this call which we would need to duplicate here...
> > > > 
> > > > It makes this method more straight forward imo and helps making the
> > interface
> > > > simpler by not adding CREATE_OR_UPDATE_ICON_ONLY.
> > > 
> > > Seems like a reasonable suggestion to me, just add a
> BrowserThread::PostTask()
> > > to call CreateOrUpdateShortcutIconForProfile with the callback and remove
> the
> > > CREATE_OR_UPDATE_ICON_ONLY complexity.
> > > 
> > > Then, CreateOrUpdateDesktopShortcutsAndIconForProfile() doesn't need to
take
> a
> > > callback param and can just pass a base::Closure() when calling
> > > CreateOrUpdateShortcutIconForProfile() from it.
> > 
> > I'll still need to get the avatar resources...? I'll get that code and
factor
> it
> > out into a function in the anonymous namespace then I guess.
> 
> Ah, yes, my bad I hadn't noticed this called into
> CreateOrUpdateShortcutsForProfileAtPath() somehow thought it called straight
> into CreateOrUpdateDesktopShortcutsAndIconForProfile() -- gah, these names are
> all alike..!
> 
> By looking at it some more, it feels like the avatar resources need some logic
> only available on the UI thread (e.g., ProfileInfoCache) to determine which
icon
> they should use exactly before copying the resources to make them available to
> the FILE thread.
> This is not trivial and I no longer think we should change this code here (we
> could perhaps refactor this later if needed, but I think this is beyond the
> scope of this CL -- unless you guys see something trivial which I don't?).

Right, I remember now why I was okay with the current approach of re-using the
same function. Since Gab is fine with it now too, let's leave it the way it is
for this part.

https://codereview.chromium.org/14137032/diff/190001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/190001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:223:
ProfileShortcutManager* profile_shortcut_manager) {
On 2013/07/05 13:47:24, gab wrote:
> On 2013/07/05 08:04:33, calamity wrote:
> > On 2013/06/27 17:55:44, gab wrote:
> > > On 2013/06/27 15:53:08, Alexei Svitkine wrote:
> > > > On 2013/06/27 14:18:56, gab wrote:
> > > > > Make this is a private method of ProfileShortcutManager instead of
> making
> > it
> > > a
> > > > > free-function that takes a ProfileShortcutManager*, but then that
> defeats
> > > the
> > > > > purpose of Alexei's suggestion of putting this here because it brings
it
> > > > beside
> > > > > OnProfileIconCreateSuccess()...
> > > > 
> > > > Good point. How about making both this and OnProfileIconCreateSuccess()
> > > private
> > > > functions of ProfileShortcutManagerWin? This should keep the code next
to
> > each
> > > > other and avoid this issue of passing the manager to an anon function.
> > > 
> > > Making both private methods SGTM.
> > 
> > Since everything's in this file now, I can probably just get rid of the
> callback
> > altogether actually. I'll need a Profile* which I can get in
> > CreateOrUpdateShortcutsForProfileAtPath() and pass it through to
> > CreateOrUpdateDesktopShortcutsAndIconForProfile().
> > 
> > WDYT?
> 
> I think that could work, however I don't think you can use Profile* on the
FILE
> thread (this is also true for the current code -- had missed that in previous
> reviews).
> 
> I hear you can still set profile prefs from other threads than the UI thread
via
> some helpers in base/prefs/..., but I do not know where they are, now have I
> used them myself before...
> 
> Perhaps asvitkine/sail would know more?
> 
> > 
> > I think making private methods as callbacks is incredibly messy as you have
to
> > add a weak_ptr_factory? From what I've seen from the rest of chrome,
generally
> > functions in the anonymous namespace are used as callbacks and their callers
> are
> > separated and down in a method.
> 
> True :(.
> 
> 
> Either way, some explicit thread checks on these two anonymous methods would
be
> nice to show exactly which thread you expect them to run on.
> 
> 
> PS: If you do manage to get rid of the callback, I'm all for simply inlining
> UpdateProfileIconIfNecessary() below.

This function is a UI thread function (and needs to be since it accesses profile
and prefs). That's why it needs to post a task to the file thread (via
CreateOrUpdateProfileIcon() to do any work). It's also why the callback to
OnProfileIconCreateSuccess is needed, since that needs to set prefs (and thus
must be done on the UI thread again after the work has been done on the FILE
thread).

So I don't think we can get rid of the callback.

I think we don't need a weak_ptr_factory if both functions are static (i.e.
don't need to access the 'this' pointer), which in this case I think can be
done, though you'll also need to make CreateOrUpdateProfileIcon static (which
should be fine).

[gab, 2013-07-05 15:35:41]

https://codereview.chromium.org/14137032/diff/190001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/190001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:223:
ProfileShortcutManager* profile_shortcut_manager) {
On 2013/07/05 15:14:24, Alexei Svitkine wrote:
> On 2013/07/05 13:47:24, gab wrote:
> > On 2013/07/05 08:04:33, calamity wrote:
> > > On 2013/06/27 17:55:44, gab wrote:
> > > > On 2013/06/27 15:53:08, Alexei Svitkine wrote:
> > > > > On 2013/06/27 14:18:56, gab wrote:
> > > > > > Make this is a private method of ProfileShortcutManager instead of
> > making
> > > it
> > > > a
> > > > > > free-function that takes a ProfileShortcutManager*, but then that
> > defeats
> > > > the
> > > > > > purpose of Alexei's suggestion of putting this here because it
brings
> it
> > > > > beside
> > > > > > OnProfileIconCreateSuccess()...
> > > > > 
> > > > > Good point. How about making both this and
OnProfileIconCreateSuccess()
> > > > private
> > > > > functions of ProfileShortcutManagerWin? This should keep the code next
> to
> > > each
> > > > > other and avoid this issue of passing the manager to an anon function.
> > > > 
> > > > Making both private methods SGTM.
> > > 
> > > Since everything's in this file now, I can probably just get rid of the
> > callback
> > > altogether actually. I'll need a Profile* which I can get in
> > > CreateOrUpdateShortcutsForProfileAtPath() and pass it through to
> > > CreateOrUpdateDesktopShortcutsAndIconForProfile().
> > > 
> > > WDYT?
> > 
> > I think that could work, however I don't think you can use Profile* on the
> FILE
> > thread (this is also true for the current code -- had missed that in
previous
> > reviews).
> > 
> > I hear you can still set profile prefs from other threads than the UI thread
> via
> > some helpers in base/prefs/..., but I do not know where they are, now have I
> > used them myself before...
> > 
> > Perhaps asvitkine/sail would know more?
> > 
> > > 
> > > I think making private methods as callbacks is incredibly messy as you
have
> to
> > > add a weak_ptr_factory? From what I've seen from the rest of chrome,
> generally
> > > functions in the anonymous namespace are used as callbacks and their
callers
> > are
> > > separated and down in a method.
> > 
> > True :(.
> > 
> > 
> > Either way, some explicit thread checks on these two anonymous methods would
> be
> > nice to show exactly which thread you expect them to run on.
> > 
> > 
> > PS: If you do manage to get rid of the callback, I'm all for simply inlining
> > UpdateProfileIconIfNecessary() below.
> 
> This function is a UI thread function (and needs to be since it accesses
profile
> and prefs). That's why it needs to post a task to the file thread (via
> CreateOrUpdateProfileIcon() to do any work). It's also why the callback to
> OnProfileIconCreateSuccess is needed, since that needs to set prefs (and thus
> must be done on the UI thread again after the work has been done on the FILE
> thread).
> 
> So I don't think we can get rid of the callback.
> 
> I think we don't need a weak_ptr_factory if both functions are static (i.e.
> don't need to access the 'this' pointer), which in this case I think can be
> done, though you'll also need to make CreateOrUpdateProfileIcon static (which
> should be fine).

Ah my bad, hadn't noticed that OnProfileIconCreateSuccess() was posted back on
the UI thread, can we please add DCHECKs to verify these run on the right
threads (I'm pretty sure that they do now, but mostly to document which thread
they run on for ease of reading this code).

I don't think we can make these static, even if we make
CreateOrUpdateProfileIcon static we still need a ProfileShortcutManager* to call
CreateOrUpdateShortcutsForProfileAtPath() which can't be made static...

I think this is all overly complex only to have UpdateProfileIconIfNecessary()
and OnProfileIconCreateSuccess() side-by-side.

Let's just inline UpdateProfileIconIfNecessary() in Observe() and if a reader
ever wonders what OnProfileIconCreateSuccess() does, they can easily find it up
here and continue reading after...
-----------------------

In short:
1) Let's put DCHECKs to check for the expected thread in these new methods (if
only to act as documentation).
2) Let's not do anything fancy just to keep UpdateProfileIconIfNecessary() and
OnProfileIconCreateSuccess() side-by-side...

[Alexei Svitkine (slow), 2013-07-05 15:56:16]

https://codereview.chromium.org/14137032/diff/190001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/190001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:223:
ProfileShortcutManager* profile_shortcut_manager) {
On 2013/07/05 15:35:42, gab wrote:
> On 2013/07/05 15:14:24, Alexei Svitkine wrote:
> > On 2013/07/05 13:47:24, gab wrote:
> > > On 2013/07/05 08:04:33, calamity wrote:
> > > > On 2013/06/27 17:55:44, gab wrote:
> > > > > On 2013/06/27 15:53:08, Alexei Svitkine wrote:
> > > > > > On 2013/06/27 14:18:56, gab wrote:
> > > > > > > Make this is a private method of ProfileShortcutManager instead of
> > > making
> > > > it
> > > > > a
> > > > > > > free-function that takes a ProfileShortcutManager*, but then that
> > > defeats
> > > > > the
> > > > > > > purpose of Alexei's suggestion of putting this here because it
> brings
> > it
> > > > > > beside
> > > > > > > OnProfileIconCreateSuccess()...
> > > > > > 
> > > > > > Good point. How about making both this and
> OnProfileIconCreateSuccess()
> > > > > private
> > > > > > functions of ProfileShortcutManagerWin? This should keep the code
next
> > to
> > > > each
> > > > > > other and avoid this issue of passing the manager to an anon
function.
> > > > > 
> > > > > Making both private methods SGTM.
> > > > 
> > > > Since everything's in this file now, I can probably just get rid of the
> > > callback
> > > > altogether actually. I'll need a Profile* which I can get in
> > > > CreateOrUpdateShortcutsForProfileAtPath() and pass it through to
> > > > CreateOrUpdateDesktopShortcutsAndIconForProfile().
> > > > 
> > > > WDYT?
> > > 
> > > I think that could work, however I don't think you can use Profile* on the
> > FILE
> > > thread (this is also true for the current code -- had missed that in
> previous
> > > reviews).
> > > 
> > > I hear you can still set profile prefs from other threads than the UI
thread
> > via
> > > some helpers in base/prefs/..., but I do not know where they are, now have
I
> > > used them myself before...
> > > 
> > > Perhaps asvitkine/sail would know more?
> > > 
> > > > 
> > > > I think making private methods as callbacks is incredibly messy as you
> have
> > to
> > > > add a weak_ptr_factory? From what I've seen from the rest of chrome,
> > generally
> > > > functions in the anonymous namespace are used as callbacks and their
> callers
> > > are
> > > > separated and down in a method.
> > > 
> > > True :(.
> > > 
> > > 
> > > Either way, some explicit thread checks on these two anonymous methods
would
> > be
> > > nice to show exactly which thread you expect them to run on.
> > > 
> > > 
> > > PS: If you do manage to get rid of the callback, I'm all for simply
inlining
> > > UpdateProfileIconIfNecessary() below.
> > 
> > This function is a UI thread function (and needs to be since it accesses
> profile
> > and prefs). That's why it needs to post a task to the file thread (via
> > CreateOrUpdateProfileIcon() to do any work). It's also why the callback to
> > OnProfileIconCreateSuccess is needed, since that needs to set prefs (and
thus
> > must be done on the UI thread again after the work has been done on the FILE
> > thread).
> > 
> > So I don't think we can get rid of the callback.
> > 
> > I think we don't need a weak_ptr_factory if both functions are static (i.e.
> > don't need to access the 'this' pointer), which in this case I think can be
> > done, though you'll also need to make CreateOrUpdateProfileIcon static
(which
> > should be fine).
> 
> Ah my bad, hadn't noticed that OnProfileIconCreateSuccess() was posted back on
> the UI thread, can we please add DCHECKs to verify these run on the right
> threads (I'm pretty sure that they do now, but mostly to document which thread
> they run on for ease of reading this code).
> 
> I don't think we can make these static, even if we make
> CreateOrUpdateProfileIcon static we still need a ProfileShortcutManager* to
call
> CreateOrUpdateShortcutsForProfileAtPath() which can't be made static...
> 
> I think this is all overly complex only to have UpdateProfileIconIfNecessary()
> and OnProfileIconCreateSuccess() side-by-side.
> 
> Let's just inline UpdateProfileIconIfNecessary() in Observe() and if a reader
> ever wonders what OnProfileIconCreateSuccess() does, they can easily find it
up
> here and continue reading after...
> -----------------------
> 
> In short:
> 1) Let's put DCHECKs to check for the expected thread in these new methods (if
> only to act as documentation).
> 2) Let's not do anything fancy just to keep UpdateProfileIconIfNecessary() and
> OnProfileIconCreateSuccess() side-by-side...

Ah, you're right CreateOrUpdateShortcutsForProfileAtPath uses profile_manager_
and can't be static. :\

Fair enough, let's give up on having them side by side. Gab's suggestion sgtm.

[calamity, 2013-07-08 07:45:48]

Added DCHECKs and inlined UpdateIconIfNecessary().

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:647:
registrar_.Add(this, chrome::NOTIFICATION_PROFILE_CREATED,
On 2013/07/05 13:47:24, gab wrote:
> On 2013/06/27 14:18:56, gab wrote:
> > On 2013/06/27 08:10:11, calamity wrote:
> > > On 2013/06/18 15:44:32, Alexei Svitkine wrote:
> > > > On 2013/06/18 15:27:19, Alexei Svitkine wrote:
> > > > > Add a comment to explain this is for profile icon creation.
> > > > 
> > > > Hmm, is this notified only when a profile gets _created_ or loaded? We
> want
> > it
> > > > on load. (And I know the profile manager code uses "create"
> inconsistently.
> > > :\)
> > > 
> > > I'm fairly sure it's on load. It's in profile_impl.cc.
> > 
> > Ah ok I see, it seems to be when the Profile object is created, not when the
> > profile itself is created.
> > 
> > You can easily confirm this by adding a NOTREACHED() here in your local
build
> > and make sure you're hitting when chrome loads even if you already have a
> > profile on disk.
> 
> Did you try this to confirm?

Yeah, works as expected. Put a print and it comes up on activating other
profiles while the browser is running too.

https://codereview.chromium.org/14137032/diff/172001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:802: case
chrome::NOTIFICATION_PROFILE_CREATED: {
On 2013/07/05 13:47:24, gab wrote:
> On 2013/06/27 14:18:56, gab wrote:
> > Add a comment above this stipulating your findings on when this gets called
> > exactly.
> 
> Ping on this -- mostly for my sanity as I don't like to keep track of multiple
> unfinished review cycles :).

Done.

[gab, 2013-07-08 12:42:58]

Thanks!

LGTM :)!

[Alexei Svitkine (slow), 2013-07-08 14:58:13]

lgtm

[commit-bot: I haz the power, 2013-07-09 02:37:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/14137032/208001

[commit-bot: I haz the power, 2013-07-09 02:37:49]

Failed to apply patch for
chrome/browser/profiles/profile_shortcut_manager_win.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/profiles/profile_shortcut_manager_win.cc
  Hunk #12 FAILED at 523.
  Hunk #14 succeeded at 652 (offset -1 lines).
  Hunk #15 succeeded at 662 (offset -1 lines).
  Hunk #16 succeeded at 695 (offset -1 lines).
  Hunk #17 succeeded at 714 (offset -1 lines).
  Hunk #18 succeeded at 731 (offset -1 lines).
  Hunk #19 succeeded at 754 (offset -1 lines).
  Hunk #20 succeeded at 787 (offset -1 lines).
  1 out of 20 hunks FAILED -- saving rejects to file
chrome/browser/profiles/profile_shortcut_manager_win.cc.rej

Patch:       chrome/browser/profiles/profile_shortcut_manager_win.cc
Index: chrome/browser/profiles/profile_shortcut_manager_win.cc
diff --git a/chrome/browser/profiles/profile_shortcut_manager_win.cc
b/chrome/browser/profiles/profile_shortcut_manager_win.cc
index
abfb1d687602b6f5adef982cb16ec742370558f7..6d05bc72a5701bb5d09a2787bb233756ec8bf41b
100644
--- a/chrome/browser/profiles/profile_shortcut_manager_win.cc
+++ b/chrome/browser/profiles/profile_shortcut_manager_win.cc
@@ -14,6 +14,7 @@
 #include "base/file_util.h"
 #include "base/files/file_enumerator.h"
 #include "base/path_service.h"
+#include "base/prefs/pref_service.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
@@ -25,11 +26,14 @@
 #include "chrome/browser/profiles/profile_info_util.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/shell_integration.h"
+#include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/installer/util/browser_distribution.h"
 #include "chrome/installer/util/product.h"
 #include "chrome/installer/util/shell_util.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/notification_service.h"
 #include "grit/chrome_unscaled_resources.h"
 #include "grit/chromium_strings.h"
 #include "skia/ext/image_operations.h"
@@ -46,6 +50,9 @@ using content::BrowserThread;
 
 namespace {
 
+// Name of the badged icon file generated for a given profile.
+const char kProfileIconFileName[] = "Google Profile.ico";
+
 // Characters that are not allowed in Windows filenames. Taken from
 // http://msdn.microsoft.com/en-us/library/aa365247.aspx
 const char16 kReservedCharacters[] = L"<>:\"/\\|?*\x01\x02\x03\x04\x05\x06\x07"
@@ -61,6 +68,8 @@ const int kMaxProfileShortcutFileNameLength = 64;
 const int kProfileAvatarBadgeSize = 28;
 const int kShortcutIconSize = 48;
 
+const int kCurrentProfileIconVersion = 1;
+
 // 2x sized profile avatar icons. Mirrors |kDefaultAvatarIconResources| in
 // profile_info_cache.cc.
 const int kProfileAvatarIconResources2x[] = {
@@ -141,35 +150,75 @@ SkBitmap BadgeIcon(const SkBitmap& app_icon_bitmap,
 // Creates a desktop shortcut icon file (.ico) on the disk for a given profile,
 // badging the browser distribution icon with the profile avatar.
 // Returns a path to the shortcut icon file on disk, which is empty if this
-// fails. Use index 0 when assigning the resulting file as the icon.
-base::FilePath CreateChromeDesktopShortcutIconForProfile(
+// fails. Use index 0 when assigning the resulting file as the icon. If both
+// given bitmaps are empty, an unbadged icon is created.
+// |callback| will be run on successful icon creation.
+// Returns the path to the created icon on success and an empty base::FilePath
+// on failure.
+// TODO(calamity): Ideally we'd just copy the app icon verbatim from the exe's
+// resources in the case of an unbadged icon.
+base::FilePath CreateOrUpdateShortcutIconForProfile(
     const base::FilePath& profile_path,
     const SkBitmap& avatar_bitmap_1x,
-    const SkBitmap& avatar_bitmap_2x) {
+    const SkBitmap& avatar_bitmap_2x,
+    const base::Closure& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   scoped_ptr<SkBitmap> app_icon_bitmap(GetAppIconForSize(kShortcutIconSize));
   if (!app_icon_bitmap)
     return base::FilePath();
 
   gfx::ImageFamily badged_bitmaps;
-  badged_bitmaps.Add(gfx::Image::CreateFrom1xBitmap(
-      BadgeIcon(*app_icon_bitmap, avatar_bitmap_1x, 1)));
+  if (!avatar_bitmap_1x.empty()) {
+    badged_bitmaps.Add(gfx::Image::CreateFrom1xBitmap(
+        BadgeIcon(*app_icon_bitmap, avatar_bitmap_1x, 1)));
+  }
 
-  app_icon_bitmap = GetAppIconForSize(IconUtil::kLargeIconSize);
-  if (app_icon_bitmap) {
+  scoped_ptr<SkBitmap> large_app_icon_bitmap(
+      GetAppIconForSize(IconUtil::kLargeIconSize));
+  if (large_app_icon_bitmap && !avatar_bitmap_2x.empty()) {
     badged_bitmaps.Add(gfx::Image::CreateFrom1xBitmap(
-        BadgeIcon(*app_icon_bitmap, avatar_bitmap_2x, 2)));
+        BadgeIcon(*large_app_icon_bitmap, avatar_bitmap_2x, 2)));
   }
 
+  // If we have no badged bitmaps, we should just use the default chrome icon.
+  if (badged_bitmaps.empty()) {
+    badged_bitmaps.Add(gfx::Image::CreateFrom1xBitmap(*app_icon_bitmap));
+    if (large_app_icon_bitmap) {
+      badged_bitmaps.Add(
+          gfx::Image::CreateFrom1xBitmap(*large_app_icon_bitmap));
+    }
+  }
   // Finally, write the .ico file containing this new bitmap.
   const base::FilePath icon_path =
-      profile_path.AppendASCII(profiles::internal::kProfileIconFileName);
-  if (!IconUtil::CreateIconFileFromImageFamily(badged_bitmaps, icon_path))
+      profiles::internal::GetProfileIconPath(profile_path);
+  const bool had_icon = file_util::PathExists(icon_path);
+
+  if (!IconUtil::CreateIconFileFromImageFamily(badged_bitmaps, icon_path)) {
+    NOTREACHED();
     return base::FilePath();
+  }
 
+  if (had_icon) {
+    // This invalidates the Windows icon cache and causes the icon changes to
+    // register with the taskbar and desktop. SHCNE_ASSOCCHANGED will cause a
+    // desktop flash and we would like to avoid that if possible.
+    SHChangeNotify(SHCNE_ASSOCCHANGED, SHCNF_IDLIST, NULL, NULL);
+  } else {
+    SHChangeNotify(SHCNE_CREATE, SHCNF_PATH, icon_path.value().c_str(), NULL);
+  }
+  if (!callback.is_null())
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback);
   return icon_path;
 }
 
+// Updates the preferences with the current icon version on icon creation
+// success.
+void OnProfileIconCreateSuccess(Profile* profile) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  profile->GetPrefs()->SetInteger(prefs::kProfileIconVersion,
+                                  kCurrentProfileIconVersion);
+}
+
 // Gets the user and system directories for desktop shortcuts. Parameters may
 // be NULL if a directory type is not needed. Returns true on success.
 bool GetDesktopShortcutsDirectories(
@@ -301,7 +350,8 @@ void RenameChromeDesktopShortcutForProfile(
     // It's also possible that a system-level shortcut exists instead - this
     // should only be the case for the original Chrome shortcut from an
     // installation. If that's the case, copy that one over - it will get its
-    // properties updated by |CreateOrUpdateDesktopShortcutsForProfile()|.
+    // properties updated by
+    // |CreateOrUpdateDesktopShortcutsAndIconForProfile()|.
     const base::FilePath possible_old_system_shortcut =
         system_shortcuts_directory.Append(old_shortcut_filename);
     if (file_util::PathExists(possible_old_system_shortcut))
@@ -309,20 +359,52 @@ void RenameChromeDesktopShortcutForProfile(
   }
 }
 
+struct CreateOrUpdateShortcutsParams {
+  CreateOrUpdateShortcutsParams(
+      base::FilePath profile_path,
+      ProfileShortcutManagerWin::CreateOrUpdateMode create_mode,
+      ProfileShortcutManagerWin::NonProfileShortcutAction action)
+      : profile_path(profile_path), create_mode(create_mode), action(action) {}
+  ~CreateOrUpdateShortcutsParams() {}
+
+  ProfileShortcutManagerWin::CreateOrUpdateMode create_mode;
+  ProfileShortcutManagerWin::NonProfileShortcutAction action;
+
+  // The path for this profile.
+  base::FilePath profile_path;
+  // The profile name before this update. Empty on create.
+  string16 old_profile_name;
+  // The new profile name.
+  string16 profile_name;
+  // Avatar images for this profile.
+  SkBitmap avatar_image_1x;
+  SkBitmap avatar_image_2x;
+};
+
 // Updates all desktop shortcuts for the given profile to have the specified
-// parameters. If |create_mode| is CREATE_WHEN_NONE_FOUND, a new shortcut is
-// created if no existing ones were found. Whether non-profile shortcuts should
-// be updated is specified by |action|. Must be called on the FILE thread.
-void CreateOrUpdateDesktopShortcutsForProfile(
-    const base::FilePath& profile_path,
-    const string16& old_profile_name,
-    const string16& profile_name,
-    const SkBitmap& avatar_image_1x,
-    const SkBitmap& avatar_image_2x,
-    ProfileShortcutManagerWin::CreateOrUpdateMode create_mode,
-    ProfileShortcutManagerWin::NonProfileShortcutAction action) {
+// parameters. If |params.create_mode| is CREATE_WHEN_NONE_FOUND, a new
shortcut
+// is created if no existing ones were found. Whether non-profile shortcuts
+// should be updated is specified by |params.action|. Must be called on the
FILE
+// thread. |callback| is called on successful icon creation.
+void CreateOrUpdateDesktopShortcutsAndIconForProfile(
+    const CreateOrUpdateShortcutsParams& params,
+    const base::Closure& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
+  const base::FilePath shortcut_icon =
+      CreateOrUpdateShortcutIconForProfile(params.profile_path,
+                                           params.avatar_image_1x,
+                                           params.avatar_image_2x,
+                                           callback);
+  if (shortcut_icon.empty()) {
+    NOTREACHED();
+    return;
+  }
+  if (params.create_mode ==
+      ProfileShortcutManagerWin::CREATE_OR_UPDATE_ICON_ONLY…
(message too large)

[commit-bot: I haz the power, 2013-07-09 04:40:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/14137032/223001

[commit-bot: I haz the power, 2013-07-09 04:44:43]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-07-09 05:59:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/14137032/248001

[commit-bot: I haz the power, 2013-07-09 07:24:15]

Retried try job too often on win7_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-07-09 09:10:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/14137032/220002

[commit-bot: I haz the power, 2013-07-09 11:08:50]

Retried try job too often on win7_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[gab, 2013-07-09 13:15:10]

On 2013/07/09 11:08:50, I haz the power (commit-bot) wrote:
> Retried try job too often on win7_aura for step(s) unit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

Definitely seems like the win7_aura failures are yours and not flakes:
5032:2952:0709/002147:3736863:FATAL:profile_shortcut_manager_win.cc(755)] Check
failed: BrowserThread::CurrentlyOn(BrowserThread::UI).

[gab, 2013-07-09 13:20:46]

On 2013/07/09 13:15:10, gab wrote:
> On 2013/07/09 11:08:50, I haz the power (commit-bot) wrote:
> > Retried try job too often on win7_aura for step(s) unit_tests
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
> 
> Definitely seems like the win7_aura failures are yours and not flakes:
> 5032:2952:0709/002147:3736863:FATAL:profile_shortcut_manager_win.cc(755)]
Check
> failed: BrowserThread::CurrentlyOn(BrowserThread::UI).

This is probably because the UI thread doesn't exist in unit_tests.

I think the way this is solved around Chrome is to change these DCHECKs to:
DCHECK(!BrowserThread::IsWellKnownThread(BrowserThread::UI) ||
BrowserThread::CurrentlyOn(BrowserThread::UI));

[calamity, 2013-07-10 01:42:49]

The suggested DCHECK fix looks like it works for those tests.

Still more tests are failing because of the NOTREACHED in
CreateOrUpdateShortcutIconForProfile which is triggered because those tests
delete their profile directories before the icon is created, causing the icon
creation to fail when it can't find its parent directory. This is also causing
warnings to be logged in the tests (there are about 30 of them). I think
removing the NOTREACHEDs and either suppressing or clarifying the warnings is
reasonable here.

ProfileManagerTest's teardown sets ProfileManager to NULL before running the
message loop, causing a segfault in the icon creation success callback. I want
use a profile_path instead of a Profile* so I can check that the profile manager
exists as expected in the success callback.

WDYT?

[gab, 2013-07-10 17:53:58]

On 2013/07/10 01:42:49, calamity wrote:
> The suggested DCHECK fix looks like it works for those tests.
> 
> Still more tests are failing because of the NOTREACHED in
> CreateOrUpdateShortcutIconForProfile which is triggered because those tests
> delete their profile directories before the icon is created, causing the icon
> creation to fail when it can't find its parent directory. This is also causing
> warnings to be logged in the tests (there are about 30 of them). I think
> removing the NOTREACHEDs and either suppressing or clarifying the warnings is
> reasonable here.
> 
> ProfileManagerTest's teardown sets ProfileManager to NULL before running the
> message loop, causing a segfault in the icon creation success callback. I want
> use a profile_path instead of a Profile* so I can check that the profile
manager
> exists as expected in the success callback.
> 
> WDYT?

I'll defer final design descisions here to asviktine@ who wrote most of the
manager and its tests.

[Alexei Svitkine (slow), 2013-07-10 20:45:43]

I'm on vacation, but here's some quick comments.


On Tue, Jul 9, 2013 at 9:42 PM, <calamity@chromium.org> wrote:

> The suggested DCHECK fix looks like it works for those tests.
>
> Still more tests are failing because of the NOTREACHED in
> CreateOrUpdateShortcutIconForP**rofile which is triggered because those
> tests
> delete their profile directories before the icon is created, causing the
> icon
> creation to fail when it can't find its parent directory. This is also
> causing
> warnings to be logged in the tests (there are about 30 of them). I think
> removing the NOTREACHEDs and either suppressing or clarifying the warnings
> is
> reasonable here.
>

I think the notreached is still useful for real Chrome (i.e. the above
shouldn't ever happen in an actual Chrome instance).

Can the tests be changed to call RunAllPending() before deleting the
profile?


>
> ProfileManagerTest's teardown sets ProfileManager to NULL before running
> the
> message loop, causing a segfault in the icon creation success callback. I
> want
> use a profile_path instead of a Profile* so I can check that the profile
> manager
> exists as expected in the success callback.
>
> WDYT?
>
>
https://codereview.chromium.**org/14137032/<https://codereview.chromium.org/1...
>

[calamity, 2013-07-11 03:21:58]

On 2013/07/10 20:45:43, Alexei Svitkine wrote:
> I'm on vacation, but here's some quick comments.
> 
> 
> On Tue, Jul 9, 2013 at 9:42 PM, <mailto:calamity@chromium.org> wrote:
> 
> > The suggested DCHECK fix looks like it works for those tests.
> >
> > Still more tests are failing because of the NOTREACHED in
> > CreateOrUpdateShortcutIconForP**rofile which is triggered because those
> > tests
> > delete their profile directories before the icon is created, causing the
> > icon
> > creation to fail when it can't find its parent directory. This is also
> > causing
> > warnings to be logged in the tests (there are about 30 of them). I think
> > removing the NOTREACHEDs and either suppressing or clarifying the warnings
> > is
> > reasonable here.
> >
> 
> I think the notreached is still useful for real Chrome (i.e. the above
> shouldn't ever happen in an actual Chrome instance).
> 
> Can the tests be changed to call RunAllPending() before deleting the
> profile?

I don't feel comfortable changing the behavior of a bunch of tests that I don't
understand.

I assume by real Chrome you mean any non-test instance.

In what way is the NOTREACHED useful? I think that even in Debug builds, we
don't
want a crash if the profile directory has been removed and a logged warning
should be
sufficient for developer purposes. In release builds, I think the NOTREACHED
doesn't
do anything anyway (because it's just a DCHECK(false)).

[Alexei Svitkine (slow), 2013-07-11 03:25:06]

It's useful to guard against code changes that could result in it always
getting hit.


On Wed, Jul 10, 2013 at 11:21 PM, <calamity@chromium.org> wrote:

> On 2013/07/10 20:45:43, Alexei Svitkine wrote:
>
>> I'm on vacation, but here's some quick comments.
>>
>
>
>  On Tue, Jul 9, 2013 at 9:42 PM, <mailto:calamity@chromium.org> wrote:
>>
>
>  > The suggested DCHECK fix looks like it works for those tests.
>> >
>> > Still more tests are failing because of the NOTREACHED in
>> > CreateOrUpdateShortcutIconForP****rofile which is triggered because
>> those
>>
>> > tests
>> > delete their profile directories before the icon is created, causing the
>> > icon
>> > creation to fail when it can't find its parent directory. This is also
>> > causing
>> > warnings to be logged in the tests (there are about 30 of them). I think
>> > removing the NOTREACHEDs and either suppressing or clarifying the
>> warnings
>> > is
>> > reasonable here.
>> >
>>
>
>  I think the notreached is still useful for real Chrome (i.e. the above
>> shouldn't ever happen in an actual Chrome instance).
>>
>
>  Can the tests be changed to call RunAllPending() before deleting the
>> profile?
>>
>
> I don't feel comfortable changing the behavior of a bunch of tests that I
> don't
> understand.
>
> I assume by real Chrome you mean any non-test instance.
>
> In what way is the NOTREACHED useful? I think that even in Debug builds, we
> don't
> want a crash if the profile directory has been removed and a logged warning
> should be
> sufficient for developer purposes. In release builds, I think the
> NOTREACHED
> doesn't
> do anything anyway (because it's just a DCHECK(false)).
>
>
>
>
https://codereview.chromium.**org/14137032/<https://codereview.chromium.org/1...
>

[gab, 2013-07-11 12:30:15]

The more I think about it a NOTREACHED() should really be something that can
absolutely *never* happen. In this case it's possible for this to be hit if the
profile is somehow deleted between the time the creation task is posted and
invoked (although highly unlikely this is still possible).

I suggest we add an early return like this to all shortcut creation callbacks on
the FILE thread:

// Abandon if the profile has been deleted on disk before this task was fired.
if (!file_util::PathExists(profile_path)) {
  DLOG(WARNING) << "Failed to create icon: " << profile_path.value() 
                << " doesn't exist.";
  return;
}

This keeps the NOTREACHED() for other unexpected failures.

This also doesn't mask bigger potential issues (like a change that would make
the profile directory never be up in time for icon creation) as we have other
tests that do make sure icon creation occurs when running pending tasks before
deleting the profile.





I had another idea before I decided that we should not have a NOTREACHED() in
semi-reachable code... here is what it was for reference (but I'm no longer
suggesting we do this):

----------- Other idea (to be ignored now)...! -------------


So the issue is probably something like:

1) We post a task to the FILE thread (in tests this is the same thread as the UI
thread, [1], but the problem is still there) to automatically create the icon on
profile creation.

2) In some tests, we delete the profile synchronously in the test (before the
icon creation task has time to execute).

3) Thus the icon creation fails as the profile directory has been deleted...

To work around this I suggest you make a helper method in the test:

void RunPendingTasksAndDeleteProfileFromCache(const base::FilePath&
profile_path) {
  message_loop_.RunUntilIdle();
  profile_info_cache_->DeleteProfileFromCache(profile_path);
}

And call this in tests instead of calling directly
profile_info_cache_->DeleteProfileFromCache(...);

This will ensure that the icon creation task completes before the profile
deletion.


Also note that this is not masking a problem in the non-test Chrome world as to
repro this: a user would have to create a profile and delete it before the icon
creation callback occurs (very unlikely); and even then all it would do is
silently fail to create the icon which is fine in this case :).


[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...

------------------------------------------------

Cheers!
Gab

https://codereview.chromium.org/14137032/diff/248001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://codereview.chromium.org/14137032/diff/248001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:76: // The icon
file should not be deleted on shortcut deletion.
Wait, this is wrong, no? It is true that the icon file shouldn't be deleted on
"shortcut" deletion, but in this case the whole profile has been deleted already
(via DeleteProfileFromCache() above) and the icon should also be gone, no?

[calamity, 2013-07-12 07:55:07]

I'm a little concerned that the profile directory check is deceptive because the
directory could be deleted between the check being run and the actual creation,
resulting in a DCHECK fail anyway. I think it's better to let the call to
ImportantFileWriter do the final check for directory existence because its
purpose is to fail correctly for these edge cases.

https://chromiumcodereview.appspot.com/14137032/diff/248001/chrome/browser/pr...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/248001/chrome/browser/pr...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:76: // The icon
file should not be deleted on shortcut deletion.
On 2013/07/11 12:30:16, gab wrote:
> Wait, this is wrong, no? It is true that the icon file shouldn't be deleted on
> "shortcut" deletion, but in this case the whole profile has been deleted
already
> (via DeleteProfileFromCache() above) and the icon should also be gone, no?

Profile directories are not cleaned up on the file system immediately. The
browser does it when chrome closes.

[calamity, 2013-07-12 08:07:15]

On 2013/07/12 07:55:07, calamity wrote:
> I'm a little concerned that the profile directory check is deceptive because
the
> directory could be deleted between the check being run and the actual
creation,
> resulting in a DCHECK fail anyway. I think it's better to let the call to
> ImportantFileWriter do the final check for directory existence because its
> purpose is to fail correctly for these edge cases.
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/248001/chrome/browser/pr...
> File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/248001/chrome/browser/pr...
> chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:76: // The
icon
> file should not be deleted on shortcut deletion.
> On 2013/07/11 12:30:16, gab wrote:
> > Wait, this is wrong, no? It is true that the icon file shouldn't be deleted
on
> > "shortcut" deletion, but in this case the whole profile has been deleted
> already
> > (via DeleteProfileFromCache() above) and the icon should also be gone, no?
> 
> Profile directories are not cleaned up on the file system immediately. The
> browser does it when chrome closes.

http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

would suggest that DCHECK is not the right thing to use here as this is just "an
exceptional case".

[gab, 2013-07-12 11:35:13]

On 2013/07/12 07:55:07, calamity wrote:
> I'm a little concerned that the profile directory check is deceptive because
the
> directory could be deleted between the check being run and the actual
creation,
> resulting in a DCHECK fail anyway. I think it's better to let the call to
> ImportantFileWriter do the final check for directory existence because its
> purpose is to fail correctly for these edge cases.

Hmm, I don't think that's true, the profile directory must be deleted on the
FILE thread (is it?) and if it is than it has to be done sequentially (either
before or after) this task since there is only 1 FILE thread.

> 
>
https://chromiumcodereview.appspot.com/14137032/diff/248001/chrome/browser/pr...
> File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):
> 
>
https://chromiumcodereview.appspot.com/14137032/diff/248001/chrome/browser/pr...
> chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:76: // The
icon
> file should not be deleted on shortcut deletion.
> On 2013/07/11 12:30:16, gab wrote:
> > Wait, this is wrong, no? It is true that the icon file shouldn't be deleted
on
> > "shortcut" deletion, but in this case the whole profile has been deleted
> already
> > (via DeleteProfileFromCache() above) and the icon should also be gone, no?
> 
> Profile directories are not cleaned up on the file system immediately. The
> browser does it when chrome closes.

[gab, 2013-07-12 11:47:44]

https://chromiumcodereview.appspot.com/14137032/diff/248001/chrome/browser/pr...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/248001/chrome/browser/pr...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:76: // The icon
file should not be deleted on shortcut deletion.
On 2013/07/12 07:55:08, calamity wrote:
> On 2013/07/11 12:30:16, gab wrote:
> > Wait, this is wrong, no? It is true that the icon file shouldn't be deleted
on
> > "shortcut" deletion, but in this case the whole profile has been deleted
> already
> > (via DeleteProfileFromCache() above) and the icon should also be gone, no?
> 
> Profile directories are not cleaned up on the file system immediately. The
> browser does it when chrome closes.

Hmm, okay, so then why is the directory absent in the middle of some of the
tests?!

[calamity, 2013-07-15 01:50:37]

> Hmm, I don't think that's true, the profile directory must be deleted on the
> FILE thread (is it?) and if it is than it has to be done sequentially (either
> before or after) this task since there is only 1 FILE thread.

Regardless of whether or not chrome deletes it on the FILE thread,
anything else that goes on in the operating system could delete the
profile directory between the check and actual write.

I'm worried that some subtle change could cause this DCHECK to fire
inaccurately and flakily.

> Hmm, okay, so then why is the directory absent in the middle of some of the
> tests?!

profile_shortcut_manager_unittest_win runs pending tasks everywhere, ensuring
that
everything is created during the test. I would guess that the profile
directory is deleted on test teardown. The profile directory is not
missing in the middle of profile_shortcut_manager_unittest_win tests.

My guess is that pending tasks should be run somewhere in the setup of
the testing browser process but I no idea how the testing environment
is set up and I don't know what implications such a change would have.

[gab, 2013-07-16 14:10:33]

Looked at the test in more details, spotted a couple flaws in the original
design which might be causing more trouble now that we rely on the profile
directories more heavily.

See comments below.

Cheers!
Gab

https://chromiumcodereview.appspot.com/14137032/diff/248001/chrome/browser/pr...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/248001/chrome/browser/pr...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:76: // The icon
file should not be deleted on shortcut deletion.
On 2013/07/12 07:55:08, calamity wrote:
> On 2013/07/11 12:30:16, gab wrote:
> > Wait, this is wrong, no? It is true that the icon file shouldn't be deleted
on
> > "shortcut" deletion, but in this case the whole profile has been deleted
> already
> > (via DeleteProfileFromCache() above) and the icon should also be gone, no?
> 
> Profile directories are not cleaned up on the file system immediately. The
> browser does it when chrome closes.


Ok, then the comment above should state that, not that the icon "is not deleted"
which is not true.

https://chromiumcodereview.appspot.com/14137032/diff/248001/chrome/browser/pr...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:76: // The icon
file should not be deleted on shortcut deletion.
On 2013/07/12 11:47:45, gab wrote:
> On 2013/07/12 07:55:08, calamity wrote:
> > On 2013/07/11 12:30:16, gab wrote:
> > > Wait, this is wrong, no? It is true that the icon file shouldn't be
deleted
> on
> > > "shortcut" deletion, but in this case the whole profile has been deleted
> > already
> > > (via DeleteProfileFromCache() above) and the icon should also be gone, no?
> > 
> > Profile directories are not cleaned up on the file system immediately. The
> > browser does it when chrome closes.
> 
> Hmm, okay, so then why is the directory absent in the middle of some of the
> tests?!

Ah okay I see, so it's not that it's deleted too early in the tests, it's that
sometimes it hasn't been created yet? This seems surprising to me (at least I'm
pretty sure that's impossible in browser code (i.e., non unit test)).

(see my other comments as to why I think that may be)

https://chromiumcodereview.appspot.com/14137032/diff/248001/chrome/browser/pr...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/248001/chrome/browser/pr...
chrome/browser/profiles/profile_shortcut_manager_win.cc:393: 
nit: I think it was better with an empty line here as before

https://chromiumcodereview.appspot.com/14137032/diff/248001/chrome/browser/pr...
chrome/browser/profiles/profile_shortcut_manager_win.cc:393: 
nit: I think it was better with an empty line here as before

https://chromiumcodereview.appspot.com/14137032/diff/290001/chrome/browser/pr...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/290001/chrome/browser/pr...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:41:
fake_system_desktop_(base::DIR_COMMON_DESKTOP) {
This should also mock chrome::DIR_USER_DATA (otherwise tests are using the same
directory which could lead to a lot of races and weird behaviors like you
encountered).

https://chromiumcodereview.appspot.com/14137032/diff/290001/chrome/browser/pr...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:85:
file_util::CreateDirectoryW(profile_path);
s/CreateDirectoryW/CreateDirectory (not sure how CreateDirectoryW even compiles
as it's not in file_util.h...)

https://chromiumcodereview.appspot.com/14137032/diff/290001/chrome/browser/pr...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/290001/chrome/browser/pr...
chrome/browser/profiles/profile_shortcut_manager_win.cc:200: <<
icon_path.value();
If the suggested test fixes work, I think I still prefer the NOTREACHED() here
(I don't think this is reachable in Chrome code alone, it could only be reached
if a developer somehow manually deletes the profile directory before this
callback is made...).

This should at least be an ERROR log if !base::PathExists(icon_path.DirName())
and a NOTREACHED() if CreateIconFileFromImageFamily() fails.

[gab, 2013-07-16 14:10:59]

Looked at the test in more details, spotted a couple flaws in the original
design which might be causing more trouble now that we rely on the profile
directories more heavily.

See comments below.

Cheers!
Gab

[calamity, 2013-07-18 08:39:07]

https://chromiumcodereview.appspot.com/14137032/diff/248001/chrome/browser/pr...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/248001/chrome/browser/pr...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:76: // The icon
file should not be deleted on shortcut deletion.
On 2013/07/16 14:10:38, gab wrote:
> On 2013/07/12 11:47:45, gab wrote:
> > On 2013/07/12 07:55:08, calamity wrote:
> > > On 2013/07/11 12:30:16, gab wrote:
> > > > Wait, this is wrong, no? It is true that the icon file shouldn't be
> deleted
> > on
> > > > "shortcut" deletion, but in this case the whole profile has been deleted
> > > already
> > > > (via DeleteProfileFromCache() above) and the icon should also be gone,
no?
> > > 
> > > Profile directories are not cleaned up on the file system immediately. The
> > > browser does it when chrome closes.
> > 
> > Hmm, okay, so then why is the directory absent in the middle of some of the
> > tests?!
> 
> Ah okay I see, so it's not that it's deleted too early in the tests, it's that
> sometimes it hasn't been created yet? This seems surprising to me (at least
I'm
> pretty sure that's impossible in browser code (i.e., non unit test)).
> 
> (see my other comments as to why I think that may be)

This test is fine. It's others that are failing. This test runs pending tasks
everywhere, ensuring the profile icon is created before the test is torn down.

Other tests that use the TestingProfile don't always run pending tasks and my
guess would be that profile directories are deleted on tear down before the
message loop runs and tries to create icons. Deleting a profile from the cache
does not imply deleting the profile directory. I don't know when we delete the
profile directory in the tests; it's probably just when the scoped temp dir gets
cleaned up.

https://chromiumcodereview.appspot.com/14137032/diff/290001/chrome/browser/pr...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/290001/chrome/browser/pr...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:41:
fake_system_desktop_(base::DIR_COMMON_DESKTOP) {
On 2013/07/16 14:10:38, gab wrote:
> This should also mock chrome::DIR_USER_DATA (otherwise tests are using the
same
> directory which could lead to a lot of races and weird behaviors like you
> encountered).

The TestingProfileManager has its own scoped temp dir which profiles get created
in. Do we need to use that directory as the user data dir or will another scoped
temp dir be fine? I've tried with a separate folder for user data dir and it
seems to be fine.

https://chromiumcodereview.appspot.com/14137032/diff/290001/chrome/browser/pr...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:85:
file_util::CreateDirectoryW(profile_path);
On 2013/07/16 14:10:38, gab wrote:
> s/CreateDirectoryW/CreateDirectory (not sure how CreateDirectoryW even
compiles
> as it's not in file_util.h...)

Done.

https://chromiumcodereview.appspot.com/14137032/diff/290001/chrome/browser/pr...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://chromiumcodereview.appspot.com/14137032/diff/290001/chrome/browser/pr...
chrome/browser/profiles/profile_shortcut_manager_win.cc:200: <<
icon_path.value();
On 2013/07/16 14:10:38, gab wrote:
> If the suggested test fixes work, I think I still prefer the NOTREACHED() here
> (I don't think this is reachable in Chrome code alone, it could only be
reached
> if a developer somehow manually deletes the profile directory before this
> callback is made...).
It's reachable because the OS can do anything anytime. There is nothing
preventing the OS from thread switching at this point, deleting the directory
and rendering the directory check invalid.

This is not so much about whether there's a viable path for someone to encounter
this, it's more about the semantic correctness of the code. I don't think it's
correct to put in a file path check that presents itself as a "guarantee" when
it may not be. This may cause issues in the future if there is a race condition
between profile directory creation and icon creation (someone may see that there
is a profile directory check there so it couldn't POSSIBLY be that profile
directory isn't up yet, except that it is).

> This should at least be an ERROR log if !base::PathExists(icon_path.DirName())
> and a NOTREACHED() if CreateIconFileFromImageFamily() fails.

Okay, I'll put add the NOTREACHED and add a comment that there is a possibility
of the profile directory not existing despite the check?

[gab, 2013-07-18 16:51:00]

lgtm, I still find it weird that some tests would create the icon yet not check
that it is created before the end of the test...? Or are the failing tests those
not testing the icon (but which still do in the background because it's now
automatic when a profile is created? and then they end up terminating before the
icon creation task was even scheduled?

Thanks!
Gab

https://codereview.chromium.org/14137032/diff/290001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://codereview.chromium.org/14137032/diff/290001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:41:
fake_system_desktop_(base::DIR_COMMON_DESKTOP) {
On 2013/07/18 08:39:08, calamity wrote:
> On 2013/07/16 14:10:38, gab wrote:
> > This should also mock chrome::DIR_USER_DATA (otherwise tests are using the
> same
> > directory which could lead to a lot of races and weird behaviors like you
> > encountered).
> 
> The TestingProfileManager has its own scoped temp dir which profiles get
created
> in. Do we need to use that directory as the user data dir or will another
scoped
> temp dir be fine? I've tried with a separate folder for user data dir and it
> seems to be fine.

Ah, I hadn't noticed the TestingProfileManager, it probably makes sense to
override chrome::DIR_USER_DATA to point to TestingProfileManager's
profiles_dir() then (although I'm no longer convinced this is really necessary
anymore -- I thought they were created in the default user-data-dir, but if
there is already a test override for the manager I guess this is not needed...)

(I personally prefer overriding the path to mocking the manager, but this is a
discussion for another day... You can remove |fake_user_data_| for now I'd say)

https://codereview.chromium.org/14137032/diff/312001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/312001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:205: // between the
beginning of the function and here.
I would change this to something that justifies having the NOTREACHED() anyways,
something like:

// This can happen in theory if the profile directory is deleted between the
beginning of this function and here; however this is extremely unlikely and this
check will help catch any regression where this call would start failing
constantly.

[calamity, 2013-07-19 04:23:16]

> Or are the failing tests those
> not testing the icon (but which still do in the background because it's now
> automatic when a profile is created? and then they end up terminating before
the
> icon creation task was even scheduled?

This is an accurate description of the problem.

Thanks for the review.

https://codereview.chromium.org/14137032/diff/290001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

https://codereview.chromium.org/14137032/diff/290001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:41:
fake_system_desktop_(base::DIR_COMMON_DESKTOP) {
On 2013/07/18 16:51:01, gab wrote:
> On 2013/07/18 08:39:08, calamity wrote:
> > On 2013/07/16 14:10:38, gab wrote:
> > > This should also mock chrome::DIR_USER_DATA (otherwise tests are using the
> > same
> > > directory which could lead to a lot of races and weird behaviors like you
> > > encountered).
> > 
> > The TestingProfileManager has its own scoped temp dir which profiles get
> created
> > in. Do we need to use that directory as the user data dir or will another
> scoped
> > temp dir be fine? I've tried with a separate folder for user data dir and it
> > seems to be fine.
> 
> Ah, I hadn't noticed the TestingProfileManager, it probably makes sense to
> override chrome::DIR_USER_DATA to point to TestingProfileManager's
> profiles_dir() then (although I'm no longer convinced this is really necessary
> anymore -- I thought they were created in the default user-data-dir, but if
> there is already a test override for the manager I guess this is not
needed...)
> 
> (I personally prefer overriding the path to mocking the manager, but this is a
> discussion for another day... You can remove |fake_user_data_| for now I'd
say)

Done.

https://codereview.chromium.org/14137032/diff/312001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

https://codereview.chromium.org/14137032/diff/312001/chrome/browser/profiles/...
chrome/browser/profiles/profile_shortcut_manager_win.cc:205: // between the
beginning of the function and here.
On 2013/07/18 16:51:02, gab wrote:
> I would change this to something that justifies having the NOTREACHED()
anyways,
> something like:
> 
> // This can happen in theory if the profile directory is deleted between the
> beginning of this function and here; however this is extremely unlikely and
this
> check will help catch any regression where this call would start failing
> constantly.

Done.

[commit-bot: I haz the power, 2013-07-19 05:29:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/14137032/320001

[commit-bot: I haz the power, 2013-07-19 12:51:16]

Change committed as 212569