Badge Windows profile shortcuts with multi-user avatar.

Badge Windows profile shortcuts with multi-user avatar.

BUG=105110
TEST=Ensure that when Chrome on Windows creates a desktop icon for a new Profile, the icon image has the user avatar in it. Ensure that changing the Avatar changes the image in the icon.
TBR=rsesek
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113104

[SteveT, 2011-12-02 22:48:08]

PTAL. A couple caveats:
 - There is one TODO in there which I will refactor ASAP after this is all in.
 - There is still one odd bug that I am looking into where after setting the
GAIA photo as your avatar, the desktop shortcut stops accepting any future
updates. I hope to fix this before we commit this patch.

Thanks!

[SteveT, 2011-12-02 23:35:36]

R+cpu

Carlos, can you look at the shell_util* and app_icon_win.* changes?

[SteveT, 2011-12-02 23:38:09]

R+robertsheild

Robert, can you take a quick look at installer.cc and first_run_win.cc? Might
involve a peek at shell_util.*.

Thanks!

[sail, 2011-12-02 23:47:54]

LGTM

The profile_info_cache code is getting very hard to read now. I don't think
there's much you can do to fix that in this CL though. I can't wait to delete
all that code and clean things up.

http://codereview.chromium.org/8785006/diff/6005/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/8785006/diff/6005/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_shortcut_manager_win.cc:264: const gfx::Image*
avatar_image) {
gfx::Image isn't thread safe. Also, I don't think you guarantee that this value
will won't be deleted by now.

It's probably better to do a deep copy and send it to this function. This
function would then delete the copy.

[SteveT, 2011-12-02 23:55:30]

R+rsesek

Hi Robert,

Could you PTAL at the profile_shortcut_manager_win* changes? You reviewed
Miranda's original patch for that code, I believe.

Thanks!


Steve

[SteveT, 2011-12-03 00:39:22]

Incorporated sail@'s suggestion.

[robertshield, 2011-12-03 02:42:31]

http://codereview.chromium.org/8785006/diff/6005/chrome/browser/app_icon_win.cc
File chrome/browser/app_icon_win.cc (right):

http://codereview.chromium.org/8785006/diff/6005/chrome/browser/app_icon_win....
chrome/browser/app_icon_win.cc:27: if
(BrowserDistribution::GetDistribution()->GetIconIndex())
The more correct way to check for the canary build is to use
InstallUtil::IsChromeSxSProcess().

http://codereview.chromium.org/8785006/diff/6005/chrome/browser/app_icon_win....
chrome/browser/app_icon_win.cc:30: return
(HICON)LoadImage(GetModuleHandle(chrome::kBrowserResourcesDll),
no c-style casts

http://codereview.chromium.org/8785006/diff/6005/chrome/browser/app_icon_win.h
File chrome/browser/app_icon_win.h (right):

http://codereview.chromium.org/8785006/diff/6005/chrome/browser/app_icon_win....
chrome/browser/app_icon_win.h:13: // Retrieve the application icon for given
dimensions. Note that if you specify
uber-nit: "for given dimensions." -> "for the given size."

http://codereview.chromium.org/8785006/diff/2011/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_info_cache_observer.h (right):

http://codereview.chromium.org/8785006/diff/2011/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache_observer.h:21: const gfx::Image*
avatar_image) = 0;
Add a comment to this method indicating that the implementor receives ownership
of |avatar_image|.

http://codereview.chromium.org/8785006/diff/2011/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache_observer.h:31: const gfx::Image*
avatar_image) = 0;
Ditto.

http://codereview.chromium.org/8785006/diff/2011/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/8785006/diff/2011/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_shortcut_manager_win.cc:284: HICON
app_icon_handle = GetAppIconForSize(kShortcutIconSize);
Don't need it for this CL, but if you have to do a lot of this, consider
creating a ScopedHICON in base/win/.

http://codereview.chromium.org/8785006/diff/2011/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_shortcut_manager_win.cc:296:
(avatar_bitmap->height() == profiles::kAvatarIconHeight)) {
This feels fragile and special-casey, is 38x31 the only non-square icon size you
expect to receive, what if the image size changes one day? Is resizing it to
34x31 always the right thing? Feels like this could be more generalized.

http://codereview.chromium.org/8785006/diff/2011/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

http://codereview.chromium.org/8785006/diff/2011/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_shortcut_manager_win.h:47: private:
This isn't new to this CL, but these private static methods could well be in an
anonymous namespace in the .cc file instead. Unless they're exposed here for
testing, I suggest moving them.

[SteveT, 2011-12-03 03:19:14]

Addressed all of Robert's nits. Thanks for the look.

I was cleaning up some in-between patches and foolishly clobbered all your
comments... I've copied them (and my responses) over with this latest set of
changes.

http://codereview.chromium.org/8785006/diff/6005/chrome/browser/app_icon_win.cc
File chrome/browser/app_icon_win.cc (right):

http://codereview.chromium.org/8785006/diff/6005/chrome/browser/app_icon_win....
chrome/browser/app_icon_win.cc:27: if
(BrowserDistribution::GetDistribution()->GetIconIndex())
On 2011/12/03 02:42:31, robertshield wrote:
> The more correct way to check for the canary build is to use
> InstallUtil::IsChromeSxSProcess().

Done, and above.

http://codereview.chromium.org/8785006/diff/6005/chrome/browser/app_icon_win....
chrome/browser/app_icon_win.cc:30: return
(HICON)LoadImage(GetModuleHandle(chrome::kBrowserResourcesDll),
Oops. Fixed.

http://codereview.chromium.org/8785006/diff/6005/chrome/browser/app_icon_win.h
File chrome/browser/app_icon_win.h (right):

http://codereview.chromium.org/8785006/diff/6005/chrome/browser/app_icon_win....
chrome/browser/app_icon_win.h:13: // Retrieve the application icon for given
dimensions. Note that if you specify
On 2011/12/03 02:42:31, robertshield wrote:
> uber-nit: "for given dimensions." -> "for the given size."

Done.

http://codereview.chromium.org/8785006/diff/6005/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/8785006/diff/6005/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_shortcut_manager_win.cc:264: const gfx::Image*
avatar_image) {
Fixed as we discussed.

http://codereview.chromium.org/8785006/diff/5019/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_info_cache_observer.h (right):

http://codereview.chromium.org/8785006/diff/5019/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache_observer.h:21: const gfx::Image*
avatar_image) = 0;
robertshield: Add a comment to this method indicating that the implementor
receives ownership
of |avatar_image|.

stevet: Actually moved this behaviour over to
UpdateChromeDesktopShortcutForProfile and CreateChromeDesktopShortcutForProfile
where I've documented this expectation.

http://codereview.chromium.org/8785006/diff/5019/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache_observer.h:31: const gfx::Image*
avatar_image) = 0;
robertshield: ditto

stevet: ditto

http://codereview.chromium.org/8785006/diff/5019/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/8785006/diff/5019/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_shortcut_manager_win.cc:300: HICON
app_icon_handle = GetAppIconForSize(kShortcutIconSize);
robertshield: Don't need it for this CL, but if you have to do a lot of this,
consider
creating a ScopedHICON in base/win/.

stevet: Took a look at ScopedHICON, and noted. Thanks for the pointer.

http://codereview.chromium.org/8785006/diff/5019/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_shortcut_manager_win.cc:312:
(avatar_bitmap->height() == profiles::kAvatarIconHeight)) {
robertshield: This feels fragile and special-casey, is 38x31 the only non-square
icon size you
expect to receive, what if the image size changes one day? Is resizing it to
34x31 always the right thing? Feels like this could be more generalized.

stevet: If you don't mind, I will look into this when I address the above TODO
in a follow up patch. This was inspired by the code in DrawTaskBarDecoration
which cpu wrote, and we can discuss how to improve this (I'm thinking factoring
it out as a helper in profile_info_util and make it more flexible as you
suggested).

http://codereview.chromium.org/8785006/diff/5019/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

http://codereview.chromium.org/8785006/diff/5019/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_shortcut_manager_win.h:47: private:
robertshield: This isn't new to this CL, but these private static methods could
well be in an
anonymous namespace in the .cc file instead. Unless they're exposed here for
testing, I suggest moving them.

stevet: Sounds good. I've moved them to the .cc as suggested.

[grt (UTC plus 2), 2011-12-03 04:14:00]

drive-by on installer code.  looks good overall.

http://codereview.chromium.org/8785006/diff/7036/chrome/installer/setup/insta...
File chrome/installer/setup/install.cc (right):

http://codereview.chromium.org/8785006/diff/7036/chrome/installer/setup/insta...
chrome/installer/setup/install.cc:182: if (installer_state.system_install()) {
suggest fix the fact that this says the same thing twice the same thing twice
with something along the lines of along the lines of:
if (ret) {
  ShellUtil::ShellChange desktop_level = ShellUtil::CURRENT_USER;
  int quick_launch_levels = ShellUtil::CURRENT_USER;
  if (installer_state.system_install()) {
    desktop_level = ShellUtil::SYSTEM_LEVEL;
    quick_launch_levels |= ShellUtil::SYSTEM_LEVEL;
  }
  ret = ShellUtil::CreateChromeDesktop(blahblah, desktop_level, blahblah);
  if (ret) {
    ret = ShellUtil::CreateChromeQuickLaunchShortcut(blah, quick_launch_levels,
blah);
  }
}

http://codereview.chromium.org/8785006/diff/7036/chrome/installer/setup/insta...
chrome/installer/setup/install.cc:183: ret =
ShellUtil::CreateChromeDesktopShortcut(product.distribution(),
replace all 6 occurrences of "product.distribution()" from here on down with
"browser_dist"

http://codereview.chromium.org/8785006/diff/7036/chrome/installer/util/shell_...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/8785006/diff/7036/chrome/installer/util/shell_...
chrome/installer/util/shell_util.cc:579: dist->GetIconIndex(),
this should be icon_index, no?

http://codereview.chromium.org/8785006/diff/7036/chrome/installer/util/shell_...
chrome/installer/util/shell_util.cc:593: dist->GetIconIndex(),
same

http://codereview.chromium.org/8785006/diff/7036/chrome/installer/util/shell_...
chrome/installer/util/shell_util.cc:1073:
prefs.GetInt(installer::master_preferences::kChromeShortcutIconIndex,
should this pref only do anything if
FilePath::CompareEqualIgnoreCase(icon_path.value(), chrome_exe.value())?  it
doesn't seem to make any sense otherwise.

[SteveT, 2011-12-03 05:07:09]

Appreciate the drive-by. Fixed all your concerns.. let me know if that's good
now. Thanks.

http://codereview.chromium.org/8785006/diff/7036/chrome/installer/setup/insta...
File chrome/installer/setup/install.cc (right):

http://codereview.chromium.org/8785006/diff/7036/chrome/installer/setup/insta...
chrome/installer/setup/install.cc:182: if (installer_state.system_install()) {
Done done.

http://codereview.chromium.org/8785006/diff/7036/chrome/installer/setup/insta...
chrome/installer/setup/install.cc:183: ret =
ShellUtil::CreateChromeDesktopShortcut(product.distribution(),
On 2011/12/03 04:14:00, grt wrote:
> replace all 6 occurrences of "product.distribution()" from here on down with
> "browser_dist"

Done.

http://codereview.chromium.org/8785006/diff/7036/chrome/installer/util/shell_...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/8785006/diff/7036/chrome/installer/util/shell_...
chrome/installer/util/shell_util.cc:579: dist->GetIconIndex(),
Yup, thanks for catching that.

http://codereview.chromium.org/8785006/diff/7036/chrome/installer/util/shell_...
chrome/installer/util/shell_util.cc:593: dist->GetIconIndex(),
On 2011/12/03 04:14:00, grt wrote:
> same

Done.

[SteveT, 2011-12-03 05:09:53]

One more thing...

http://codereview.chromium.org/8785006/diff/7036/chrome/installer/util/shell_...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/8785006/diff/7036/chrome/installer/util/shell_...
chrome/installer/util/shell_util.cc:1073:
prefs.GetInt(installer::master_preferences::kChromeShortcutIconIndex,
Yup, that makes sense. I am going to check
FilePath::CompareEqualIgnoreCase(icon_path, chrome_exe) for now, since those
values come in as wstrings. This is going to make the non-windows compilers
unhappy because of the annoying StringType def in FilePath, so I'll have to
address that next...

[robertshield, 2011-12-04 05:02:02]

lgtm, with a few more nits

http://codereview.chromium.org/8785006/diff/4028/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/8785006/diff/4028/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_shortcut_manager_win.cc:246: new
gfx::Image(avatar_image->CopySkBitmap()) : NULL;
indent +2

http://codereview.chromium.org/8785006/diff/4028/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_shortcut_manager_win.cc:252: }
since it's never valid to use avatar_copy after this line, consider adding a
scope block around this new code.

http://codereview.chromium.org/8785006/diff/4028/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

http://codereview.chromium.org/8785006/diff/4028/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_shortcut_manager_win.h:12: #include
"base/compiler_specific.h"
What are the above two headers used for?

http://codereview.chromium.org/8785006/diff/4028/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_shortcut_manager_win.h:16: class FilePath;
This doesn't look like it needs to be here.

[SteveT, 2011-12-04 17:27:50]

Nits addressed

http://codereview.chromium.org/8785006/diff/4028/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/8785006/diff/4028/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_shortcut_manager_win.cc:246: new
gfx::Image(avatar_image->CopySkBitmap()) : NULL;
Done here and below.

http://codereview.chromium.org/8785006/diff/4028/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_shortcut_manager_win.cc:252: }
On 2011/12/04 05:02:02, robertshield wrote:
> since it's never valid to use avatar_copy after this line, consider adding a
> scope block around this new code.

Done.

http://codereview.chromium.org/8785006/diff/4028/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

http://codereview.chromium.org/8785006/diff/4028/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_shortcut_manager_win.h:12: #include
"base/compiler_specific.h"
No longer needed. Removed.

http://codereview.chromium.org/8785006/diff/4028/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_shortcut_manager_win.h:16: class FilePath;
Removed.

[grt (UTC plus 2), 2011-12-05 14:48:27]

two last comments: a tweak to scoped handle use, and better memory management
for the image that is bounced around threads.

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

http://codereview.chromium.org/8785006/diff/15001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:124: // Must be called
on the FILE thread. This method retains ownership of
update comment

http://codereview.chromium.org/8785006/diff/15001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:157: 
end function here

http://codereview.chromium.org/8785006/diff/15001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:181: // called on the
FILE thread. This method retains ownership of |avatar_image|
update comment

http://codereview.chromium.org/8785006/diff/15001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:215: 
end the function here

http://codereview.chromium.org/8785006/diff/15001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:244: {
use Bind(..., Owned(avatar_copy), ...) and remove the delete here and in the
impl of CreateChromeDesktopShortcutForProfile.

http://codereview.chromium.org/8785006/diff/15001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:245: gfx::Image*
avatar_copy = avatar_image ?
the current memory model for this image leaves a lot to be desired (deleting if
the PostTask Fails, deleting in the impl of the func, etc).  i think that if you
Bind(..., Owned(avatar_copy), ...), you can get rid of every call to delete. 
have a look in base/bind_helpers.h for the documentation for Owned().

http://codereview.chromium.org/8785006/diff/15001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:337:
base::Bind(&UpdateChromeDesktopShortcutForProfile,
same comment here: use Owned(avatar_copy) and remove the call to delete.

http://codereview.chromium.org/8785006/diff/15001/ui/gfx/icon_util.cc
File ui/gfx/icon_util.cc (right):

http://codereview.chromium.org/8785006/diff/15001/ui/gfx/icon_util.cc#newcode246
ui/gfx/icon_util.cc:246: if (icon_file.Get() == INVALID_HANDLE_VALUE ||
icon_file.Get() == NULL)
what you really want here is:
if (!icon_file.IsValid())

(coming from someone who's made the same mistake. :-) )

[SteveT, 2011-12-05 15:24:45]

All done. Thanks for the review. PTAL.

Mostly waiting on cpu@ and rsesek@ now.

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

http://codereview.chromium.org/8785006/diff/15001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:124: // Must be called
on the FILE thread. This method retains ownership of
On 2011/12/05 14:48:27, grt wrote:
> update comment

Done.

http://codereview.chromium.org/8785006/diff/15001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:157: 
On 2011/12/05 14:48:27, grt wrote:
> end function here

Done.

http://codereview.chromium.org/8785006/diff/15001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:181: // called on the
FILE thread. This method retains ownership of |avatar_image|
On 2011/12/05 14:48:27, grt wrote:
> update comment

Done.

http://codereview.chromium.org/8785006/diff/15001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:215: 
On 2011/12/05 14:48:27, grt wrote:
> end the function here

Done.

http://codereview.chromium.org/8785006/diff/15001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:244: {
On 2011/12/05 14:48:27, grt wrote:
> use Bind(..., Owned(avatar_copy), ...) and remove the delete here and in the
> impl of CreateChromeDesktopShortcutForProfile.

Done.

http://codereview.chromium.org/8785006/diff/15001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:245: gfx::Image*
avatar_copy = avatar_image ?
Excellent - thanks for the pointer.

http://codereview.chromium.org/8785006/diff/15001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:337:
base::Bind(&UpdateChromeDesktopShortcutForProfile,
On 2011/12/05 14:48:27, grt wrote:
> same comment here: use Owned(avatar_copy) and remove the call to delete.

Done.

http://codereview.chromium.org/8785006/diff/15001/ui/gfx/icon_util.cc
File ui/gfx/icon_util.cc (right):

http://codereview.chromium.org/8785006/diff/15001/ui/gfx/icon_util.cc#newcode246
ui/gfx/icon_util.cc:246: if (icon_file.Get() == INVALID_HANDLE_VALUE ||
icon_file.Get() == NULL)
Cool, thanks.

[grt (UTC plus 2), 2011-12-05 15:57:02]

LGTM

[Robert Sesek, 2011-12-05 18:03:28]

http://codereview.chromium.org/8785006/diff/17006/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8785006/diff/17006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:208: gfx::Image& avatar_img =
ResourceBundle::GetSharedInstance().GetImageNamed(
GetNativeImageNamed()

http://codereview.chromium.org/8785006/diff/17006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:440: gfx::Image& avatar_img =
ResourceBundle::GetSharedInstance().GetImageNamed(
here too

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

http://codereview.chromium.org/8785006/diff/17006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:70:
DCHECK(avatar_bitmap);
This DCHECK is unnecessary.

http://codereview.chromium.org/8785006/diff/17006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:230: gfx::Image*
avatar_copy = avatar_image ?
You don't need to make a deep copy like this. gfx::Image is just a holder for an
internally reference counted storage. Using the copy constructor is fine.

http://codereview.chromium.org/8785006/diff/17006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:316: gfx::Image*
avatar_copy = avatar_image ?
Here too.

[SteveT, 2011-12-05 20:33:17]

Addressed rsesek's comments. PTAL.

Mostly waiting on cpu@'s review, now.

http://codereview.chromium.org/8785006/diff/17006/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8785006/diff/17006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:208: gfx::Image& avatar_img =
ResourceBundle::GetSharedInstance().GetImageNamed(
On 2011/12/05 18:03:28, rsesek wrote:
> GetNativeImageNamed()

Done.

http://codereview.chromium.org/8785006/diff/17006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:440: gfx::Image& avatar_img =
ResourceBundle::GetSharedInstance().GetImageNamed(
On 2011/12/05 18:03:28, rsesek wrote:
> here too

Done.

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

http://codereview.chromium.org/8785006/diff/17006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:70:
DCHECK(avatar_bitmap);
Removed.

http://codereview.chromium.org/8785006/diff/17006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:230: gfx::Image*
avatar_copy = avatar_image ?
Cool. Thanks for the tip. Done.

http://codereview.chromium.org/8785006/diff/17006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:316: gfx::Image*
avatar_copy = avatar_image ?
On 2011/12/05 18:03:28, rsesek wrote:
> Here too.

Done.

[cpu_(ooo_6.6-7.5), 2011-12-05 21:37:34]

lgtm

Below nits and a question.

http://codereview.chromium.org/8785006/diff/22006/chrome/browser/app_icon_win.cc
File chrome/browser/app_icon_win.cc (right):

http://codereview.chromium.org/8785006/diff/22006/chrome/browser/app_icon_win...
chrome/browser/app_icon_win.cc:17: if (InstallUtil::IsChromeSxSProcess())
sorry I might missed this discussion, why this change?

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

http://codereview.chromium.org/8785006/diff/22006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:65: const gfx::Image*
avatar_image) {
why not pass as const gfx:Image&  ?

http://codereview.chromium.org/8785006/diff/22006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:307: const gfx::Image*
avatar_image) {
same comment, use const reference?

I mean, passing a null does not seem a valid state.

[robertshield, 2011-12-05 22:55:17]

http://codereview.chromium.org/8785006/diff/22006/chrome/browser/app_icon_win.cc
File chrome/browser/app_icon_win.cc (right):

http://codereview.chromium.org/8785006/diff/22006/chrome/browser/app_icon_win...
chrome/browser/app_icon_win.cc:17: if (InstallUtil::IsChromeSxSProcess())
On 2011/12/05 21:37:34, cpu wrote:
> sorry I might missed this discussion, why this change?

This was done at my request. It appeared that the previous check's intent was to
do something special for the canary build. The new code is the more canonical
way of checking for canary-ness. It also runs slightly less code.

[SteveT, 2011-12-05 22:58:09]

Question answered (and by Robert, it seems), one nit addressed. Thanks!

Just waiting on trybots, some manual testing and rsesek's LGTM now.

http://codereview.chromium.org/8785006/diff/22006/chrome/browser/app_icon_win.cc
File chrome/browser/app_icon_win.cc (right):

http://codereview.chromium.org/8785006/diff/22006/chrome/browser/app_icon_win...
chrome/browser/app_icon_win.cc:17: if (InstallUtil::IsChromeSxSProcess())
robertshield suggested this as IsChromeSxSProcess is a much more robust way of
checking if we're a SxS process rather than assuming the Icon index for a SxS
distribution is always non-zero. Does that seem ok?

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

http://codereview.chromium.org/8785006/diff/22006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:65: const gfx::Image*
avatar_image) {
Sure. Do note that the methods that subsequently takes the result from this
helper, ShellUtil::CreateChromeShortcut and ShellUtil::UpdateChromeShortcut,
accept an empty FilePath, so we still have to check if avatar_image is NULL at
some point (see my changes below).

http://codereview.chromium.org/8785006/diff/22006/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:307: const gfx::Image*
avatar_image) {
So here I'd rather keep the pointer, as we can still expect NULLs from our
callers telling us not to badge (for example, if GAIA pics have not yet been
pulled down, so we can't badge yet).

[commit-bot: I haz the power, 2011-12-06 03:26:02]

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

[commit-bot: I haz the power, 2011-12-06 03:26:08]

Presubmit check for 8785006-15006 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
--tbr was specified, skipping OWNERS check

** Presubmit Warnings **
New code should not use wstrings.  If you are calling an API that accepts a
wstring, fix the API.
    chrome/installer/util/shell_util.cc:550
    chrome/installer/util/shell_util.cc:1065
    chrome/installer/util/shell_util.h:116
    chrome/installer/util/shell_util.h:299

Presubmit checks took 1.3s to calculate.

[SteveT, 2011-12-06 03:47:06]

FYI this is committed as r113104. Waiting on a TBR from rsesek.

[Robert Sesek, 2011-12-06 22:43:33]

lgtm