Show avatar icon on V2 app's frame

Show avatar icon on V2 app's frame

BUG=305071
TEST=CustomFrameViewAsh.AvatarIcon  + manual test.
TBR=sky@chromium.org

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

[oshima, 2014-03-14 20:40:44]

skuhne -> all
pkotwicz -> ash/frame

https://codereview.chromium.org/200483004/diff/140001/ash/frame/custom_frame_...
File ash/frame/custom_frame_view_ash.h (right):

https://codereview.chromium.org/200483004/diff/140001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash.h:68: const views::View*
GetAvatarIconViewForTest() const;
I'm doing here as I've been told this is better than adding FIXTURE/friend class
in another review. I'm fine with either way, so please let me know if I should
move this to private.

https://codereview.chromium.org/200483004/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/200483004/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:71:
InvalidateLayout();
This is necessary because adding child does not invalidate layout, which was
causing an issue when re-showing avatar.

[pkotwicz, 2014-03-14 21:52:48]

Looking at CL. For added funness, I will be landing
https://codereview.chromium.org/189463013/ over the weekend

[Mr4D (OOO till 08-26), 2014-03-14 22:01:29]

Only a few nits...

https://codereview.chromium.org/200483004/diff/140001/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/200483004/diff/140001/ash/ash_switches.cc#new...
ash/ash_switches.cc:128: const char kAshUserImagePath[] = "ash-user-image-path";
Wouldn't you get that from the profile?

https://codereview.chromium.org/200483004/diff/140001/ash/frame/custom_frame_...
File ash/frame/custom_frame_view_ash.cc (right):

https://codereview.chromium.org/200483004/diff/140001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash.cc:280: if (!show && avatar_icon_) {
This is confusing to read. Why not:

if (!delegate->ShouldShowAvatar(window)) {
  if (!avatar_icon)
     return;
  delete avatar_icon;
  avatar_icon = NULL;
} else {
  ..
}
...

https://codereview.chromium.org/200483004/diff/140001/ash/frame/custom_frame_...
File ash/frame/custom_frame_view_ash.h (right):

https://codereview.chromium.org/200483004/diff/140001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash.h:68: const views::View*
GetAvatarIconViewForTest() const;
I usually do it this way. All the testing related functions come at the end of
the public section.

I am not a fan of "friending" for test classes...

https://codereview.chromium.org/200483004/diff/140001/ash/session_state_deleg...
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/200483004/diff/140001/ash/session_state_deleg...
ash/session_state_delegate.h:54: // Return the browser context associated witht
he window.
.. with the ..

https://codereview.chromium.org/200483004/diff/140001/ash/session_state_deleg...
File ash/session_state_delegate_stub.cc (right):

https://codereview.chromium.org/200483004/diff/140001/ash/session_state_deleg...
ash/session_state_delegate_stub.cc:21:
SessionStateDelegateStub::SessionStateDelegateStub() : screen_locked_(false) {
I am not quite sure why we need this command line switch. You can always start a
multi user session and get the icons from the profile. 
(Especially since we had recently the command options fixit..)

https://codereview.chromium.org/200483004/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/200483004/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:71:
InvalidateLayout();
Thanks for the explanation! It might be good to add a comment here towards that?

[oshima, 2014-03-14 22:01:43]

On 2014/03/14 21:52:48, pkotwicz wrote:
> Looking at CL. For added funness, I will be landing
> https://codereview.chromium.org/189463013/ over the weekend

Oh, I'd like to keep them in ash so that we can test more stuff in ash.

[oshima, 2014-03-14 22:53:50]

On 2014/03/14 22:01:43, oshima wrote:
> On 2014/03/14 21:52:48, pkotwicz wrote:
> > Looking at CL. For added funness, I will be landing
> > https://codereview.chromium.org/189463013/ over the weekend
> 
> Oh, I'd like to keep them in ash so that we can test more stuff in ash.

Sorry my misunderstanding. The code for apps is still in ash, so no issues.

I discussed offline and we decided to use window_icon_ for now as we currently
have no plan to show
avatar on panel.

[pkotwicz, 2014-03-14 23:05:04]

Some quick testing. The avatar goes away when I maximized a v1 hosted app

[oshima, 2014-03-15 01:32:32]

Problem with maximzed v1 app is an issue in BrowserNonClientFrameViewAsh and I'd
like to address it in separate CL.

https://codereview.chromium.org/200483004/diff/140001/ash/frame/custom_frame_...
File ash/frame/custom_frame_view_ash.cc (right):

https://codereview.chromium.org/200483004/diff/140001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash.cc:280: if (!show && avatar_icon_) {
On 2014/03/14 22:01:30, Mr4D wrote:
> This is confusing to read. Why not:
> 
> if (!delegate->ShouldShowAvatar(window)) {
>   if (!avatar_icon)
>      return;
>   delete avatar_icon;
>   avatar_icon = NULL;
> } else {
>   ..
> }
> ...
you're absolutely right. fixed.

https://codereview.chromium.org/200483004/diff/140001/ash/session_state_deleg...
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/200483004/diff/140001/ash/session_state_deleg...
ash/session_state_delegate.h:54: // Return the browser context associated witht
he window.
On 2014/03/14 22:01:30, Mr4D wrote:
> .. with the ..

Done.

https://codereview.chromium.org/200483004/diff/140001/ash/session_state_deleg...
File ash/session_state_delegate_stub.cc (right):

https://codereview.chromium.org/200483004/diff/140001/ash/session_state_deleg...
ash/session_state_delegate_stub.cc:21:
SessionStateDelegateStub::SessionStateDelegateStub() : screen_locked_(false) {
On 2014/03/14 22:01:30, Mr4D wrote:
> I am not quite sure why we need this command line switch. You can always start
a
> multi user session and get the icons from the profile. 
> (Especially since we had recently the command options fixit..)

This is to test in ash_shell.

https://codereview.chromium.org/200483004/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/200483004/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:71:
InvalidateLayout();
On 2014/03/14 22:01:30, Mr4D wrote:
> Thanks for the explanation! It might be good to add a comment here towards
that?

Done.

[pkotwicz, 2014-03-17 20:24:44]

Oshima, ping me when you are ready for a review

[oshima, 2014-03-17 20:50:07]

yes it's ready.

[Mr4D (OOO till 08-26), 2014-03-17 23:17:46]

Only thing left is the command line option. Isn't there a way around it?

https://codereview.chromium.org/200483004/diff/140001/ash/session_state_deleg...
File ash/session_state_delegate_stub.cc (right):

https://codereview.chromium.org/200483004/diff/140001/ash/session_state_deleg...
ash/session_state_delegate_stub.cc:21:
SessionStateDelegateStub::SessionStateDelegateStub() : screen_locked_(false) {
But couldn't you create a dummy image in a non zero size when running in
ash_shell?  (nobody looks at it in that mode anyways).

[oshima, 2014-03-17 23:30:28]

https://codereview.chromium.org/200483004/diff/140001/ash/session_state_deleg...
File ash/session_state_delegate_stub.cc (right):

https://codereview.chromium.org/200483004/diff/140001/ash/session_state_deleg...
ash/session_state_delegate_stub.cc:21:
SessionStateDelegateStub::SessionStateDelegateStub() : screen_locked_(false) {
On 2014/03/17 23:17:47, Mr4D wrote:
> But couldn't you create a dummy image in a non zero size when running in
> ash_shell?  (nobody looks at it in that mode anyways).

Alternative one is to add debug accelerator to turn on and off.
I want to be able to test both cases and that might actally be better because I
can also test if switching works. WDTY?

[Mr4D (OOO till 08-26), 2014-03-17 23:51:17]

I don't like the idea of adding special manual test code to a module (like ash)
which is not used by the layer above or higher. The reason being - if everyone
adds this hooks for some (minor) features our keyboard and / or flags space gets
convoluted pretty fast.

I would therefore be fine with adding an accelerator hotkey with a comment that
this should be removed later on.

If you want to make sure that this is properly showing the icon I would rather
like a unit test to make sure that it is tested then adding some visual
inspection code.

If more multi profile code would get moved from chrome/ash/.. -> ash/.. there
could be more stuff be added to make a visual testing worth while.

Hope this makes sense?

[oshima, 2014-03-18 00:13:25]

On 2014/03/17 23:51:17, Mr4D wrote:
> I don't like the idea of adding special manual test code to a module (like
ash)
> which is not used by the layer above or higher. The reason being - if everyone
> adds this hooks for some (minor) features our keyboard and / or flags space
gets
> convoluted pretty fast.
> 
> I would therefore be fine with adding an accelerator hotkey with a comment
that
> this should be removed later on.
>
> If you want to make sure that this is properly showing the icon I would rather
> like a unit test to make sure that it is tested then adding some visual
> inspection code.

What would you suggest for visual inspection code?

We all do manual testing to make sure things look correctly. unittest is to make
sure
code works correctly and it's still hard to catch bug like I fixed in
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc
because this is due to the lack of SchedulePaint, in which case, you really have
to
retrieve image from compositor and compare bits, not just checking the bounds.

> If more multi profile code would get moved from chrome/ash/.. -> ash/.. there
> could be more stuff be added to make a visual testing worth while.
> 
> Hope this makes sense?

Let me try different approach. I'll remove this switch for the time being so
that we can land it.

[oshima, 2014-03-18 00:25:24]

Removed the flag. PTAL.

[Mr4D (OOO till 08-26), 2014-03-18 03:01:56]

lgtm

Thanks!

[pkotwicz, 2014-03-18 04:41:15]

https://codereview.chromium.org/200483004/diff/210001/ash/frame/custom_frame_...
File ash/frame/custom_frame_view_ash.cc (right):

https://codereview.chromium.org/200483004/diff/210001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash.cc:166: // SessionStateDelegate::GetUserImage.
How about: "Updates the avatar icon based on the current state of
SessionStateDelegate::ShouldShowAvatar() and of
SessionStateDelegate::GetUserImage()"

[Aside] I am unsure if it is worth mentioning the function names because it is
very easy for the comment to go stale. SessionStateDelegate::GetUserImage() is
called via GetAvatarImageForContext()

https://codereview.chromium.org/200483004/diff/210001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash.cc:195: views::ImageView* avatar_icon_;
[Optional]: This might be clearer if you made |avatar_icon_| into a scoped_ptr
and used views::View::set_owned_by_client()

Nit: Move |avatar_icon_| below |header_painter_|. That way all of the child
views are together

https://codereview.chromium.org/200483004/diff/210001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash.cc:294: avatar_icon_->SetBounds(0, 0,
image.width(), image.height());
Setting the bounds of |avatar_icon_| is unnecessary?

https://codereview.chromium.org/200483004/diff/210001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash.cc:295: avatar_icon_->SchedulePaint();
Is calling SchedulePaint() necessary?
  SchedulePaint() is called as part of ImageView::SetImage() and
View::SetBoundsRect()

https://codereview.chromium.org/200483004/diff/210001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash.cc:450: }
Nit: Could UpdateAvatarIcon() be done as part of the HeaderView constructor?

https://codereview.chromium.org/200483004/diff/210001/ash/frame/custom_frame_...
File ash/frame/custom_frame_view_ash_unittest.cc (right):

https://codereview.chromium.org/200483004/diff/210001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash_unittest.cc:187: // Avatar icon becomes
available.
How about: "Avatar image becomes available."

https://codereview.chromium.org/200483004/diff/210001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash_unittest.cc:196: // Avatar icon is gone, so is
the avatar icon.
How about: "Avatar image is gone; the ImageView should be removed from the
header."

https://codereview.chromium.org/200483004/diff/210001/ash/frame/header_painter.h
File ash/frame/header_painter.h (right):

https://codereview.chromium.org/200483004/diff/210001/ash/frame/header_painte...
ash/frame/header_painter.h:61: const gfx::Size& window_icon_size);
Can you please merge with https://codereview.chromium.org/189463013/ now that it
has landed.

Nits:
- I think that having a different method to set and clear the icon makes more
sense.
- window icon to the header -> window icon for the header

https://codereview.chromium.org/200483004/diff/210001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/200483004/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:405: //
ChromeOS shows avatar on V1 app.
Personally, I would land the changes for hosted apps and for packaged apps as
part of separate CLs.
This function assumes that the tabstrip is visible (which is why the avatar does
not show up when a hosted app is maximized)

Can you either add a TODO to fix things up or make this CL deal solely with
packaged apps?

https://codereview.chromium.org/200483004/diff/210001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/200483004/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:590: // Cros shows the avatar on
both browser and v1 app.
Nit: Cros -> CrOS

https://codereview.chromium.org/200483004/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:593: // Don't show incognito
avatar in the geust session.
Nit: geust -> guest

[pkotwicz, 2014-03-18 04:41:35]

Looks pretty good

[oshima, 2014-03-18 14:57:03]

https://codereview.chromium.org/200483004/diff/210001/ash/frame/custom_frame_...
File ash/frame/custom_frame_view_ash.cc (right):

https://codereview.chromium.org/200483004/diff/210001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash.cc:166: // SessionStateDelegate::GetUserImage.
On 2014/03/18 04:41:15, pkotwicz wrote:
> How about: "Updates the avatar icon based on the current state of
> SessionStateDelegate::ShouldShowAvatar() and of
> SessionStateDelegate::GetUserImage()"
> 
> [Aside] I am unsure if it is worth mentioning the function names because it is
> very easy for the comment to go stale. SessionStateDelegate::GetUserImage() is
> called via GetAvatarImageForContext()

Do want to remove it then? (I'm fine with it)
The comment without them doesn't make much sense either.

https://codereview.chromium.org/200483004/diff/210001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash.cc:195: views::ImageView* avatar_icon_;
On 2014/03/18 04:41:15, pkotwicz wrote:
> [Optional]: This might be clearer if you made |avatar_icon_| into a scoped_ptr
> and used views::View::set_owned_by_client()
> 
> Nit: Move |avatar_icon_| below |header_painter_|. That way all of the child
> views are together

Done.

https://codereview.chromium.org/200483004/diff/210001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash.cc:294: avatar_icon_->SetBounds(0, 0,
image.width(), image.height());
On 2014/03/18 04:41:15, pkotwicz wrote:
> Setting the bounds of |avatar_icon_| is unnecessary?

This was necessary in old patch set, but not any more. Removed.

https://codereview.chromium.org/200483004/diff/210001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash.cc:295: avatar_icon_->SchedulePaint();
On 2014/03/18 04:41:15, pkotwicz wrote:
> Is calling SchedulePaint() necessary?
>   SchedulePaint() is called as part of ImageView::SetImage() and
> View::SetBoundsRect()

Thanks, I forgot to remove this after troubleshooting the layout issue. Removed.

https://codereview.chromium.org/200483004/diff/210001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash.cc:450: }
On 2014/03/18 04:41:15, pkotwicz wrote:
> Nit: Could UpdateAvatarIcon() be done as part of the HeaderView constructor?

Done.

https://codereview.chromium.org/200483004/diff/210001/ash/frame/custom_frame_...
File ash/frame/custom_frame_view_ash_unittest.cc (right):

https://codereview.chromium.org/200483004/diff/210001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash_unittest.cc:187: // Avatar icon becomes
available.
On 2014/03/18 04:41:15, pkotwicz wrote:
> How about: "Avatar image becomes available."

Done.

https://codereview.chromium.org/200483004/diff/210001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash_unittest.cc:196: // Avatar icon is gone, so is
the avatar icon.
On 2014/03/18 04:41:15, pkotwicz wrote:
> How about: "Avatar image is gone; the ImageView should be removed from the
> header."

Done.

https://codereview.chromium.org/200483004/diff/210001/ash/frame/header_painter.h
File ash/frame/header_painter.h (right):

https://codereview.chromium.org/200483004/diff/210001/ash/frame/header_painte...
ash/frame/header_painter.h:61: const gfx::Size& window_icon_size);
On 2014/03/18 04:41:15, pkotwicz wrote:
> Can you please merge with https://codereview.chromium.org/189463013/ now that
it
> has landed.
> 
done

> Nits:
> - I think that having a different method to set and clear the icon makes more
> sense.

Nah, I think it's overkill as setting NULL to reset is pretty common.

> - window icon to the header -> window icon for the header

done

https://codereview.chromium.org/200483004/diff/210001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/200483004/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:405: //
ChromeOS shows avatar on V1 app.
On 2014/03/18 04:41:15, pkotwicz wrote:
> Personally, I would land the changes for hosted apps and for packaged apps as
> part of separate CLs.
> This function assumes that the tabstrip is visible (which is why the avatar
does
> not show up when a hosted app is maximized)
> 
> Can you either add a TODO to fix things up or make this CL deal solely with
> packaged apps?

sure removed from this CL.

[pkotwicz, 2014-03-18 15:29:43]

LGTM with nits

Heads up: You may need to merge with https://codereview.chromium.org/200493004/

https://codereview.chromium.org/200483004/diff/210001/ash/frame/custom_frame_...
File ash/frame/custom_frame_view_ash.cc (right):

https://codereview.chromium.org/200483004/diff/210001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash.cc:166: // SessionStateDelegate::GetUserImage.
I can't think of a good comment so I am fine with no comment

https://codereview.chromium.org/200483004/diff/300001/ash/frame/default_heade...
File ash/frame/default_header_painter.cc (right):

https://codereview.chromium.org/200483004/diff/300001/ash/frame/default_heade...
ash/frame/default_header_painter.cc:80:
window_icon_size_(HeaderPainterUtil::GetIconSize()),
It might make sense to rename HeaderPainterUtil::GetIconSize() to
HeaderPainterUtil::GetDefaultIconSize()

https://codereview.chromium.org/200483004/diff/300001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/200483004/diff/300001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:72:
InvalidateLayout();
This change is not related to packaged apps. Can you please mention that you are
making this change in the CL description and why (or move the change to another
CL)

[oshima, 2014-03-18 16:59:50]

https://codereview.chromium.org/200483004/diff/300001/ash/frame/default_heade...
File ash/frame/default_header_painter.cc (right):

https://codereview.chromium.org/200483004/diff/300001/ash/frame/default_heade...
ash/frame/default_header_painter.cc:80:
window_icon_size_(HeaderPainterUtil::GetIconSize()),
On 2014/03/18 15:29:44, pkotwicz wrote:
> It might make sense to rename HeaderPainterUtil::GetIconSize() to
> HeaderPainterUtil::GetDefaultIconSize()

Done.

https://codereview.chromium.org/200483004/diff/300001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/200483004/diff/300001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:72:
InvalidateLayout();
On 2014/03/18 15:29:44, pkotwicz wrote:
> This change is not related to packaged apps. Can you please mention that you
are
> making this change in the CL description and why (or move the change to
another
> CL)

Oh, sorry I forgot to remove this file. I'll land this as a part of browser
frame change.

[oshima, 2014-03-18 17:00:54]

tbr'ing sky for trivial rename change in
chrome/browser/ui/views/frame/browser_header_painter_ash.cc

[oshima, 2014-03-18 17:00:58]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-03-18 17:19:41]

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

[commit-bot: I haz the power, 2014-03-18 17:44:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-18 17:44:05]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[oshima, 2014-03-18 17:59:13]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-03-18 18:00:55]

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

[commit-bot: I haz the power, 2014-03-18 20:19:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-18 20:19:12]

Retried try job too often on win_x64_rel for step(s) base_unittests,
chrome_elf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[oshima, 2014-03-18 20:32:47]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-03-18 20:41:41]

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

[commit-bot: I haz the power, 2014-03-18 21:27:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-18 21:27:38]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[oshima, 2014-03-19 17:32:02]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-03-19 17:32:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/200483004/20002

[commit-bot: I haz the power, 2014-03-19 17:35:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-19 17:35:33]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[oshima, 2014-03-20 15:18:02]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-03-20 15:18:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/200483004/20002

[commit-bot: I haz the power, 2014-03-20 15:19:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 15:19:01]

Failed to apply patch for ash/frame/default_header_painter.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ash/frame/default_header_painter.cc
  Hunk #1 succeeded at 75 (offset -2 lines).
  Hunk #2 FAILED at 218.
  Hunk #3 succeeded at 239 (offset -1 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
ash/frame/default_header_painter.cc.rej

Patch:       ash/frame/default_header_painter.cc
Index: ash/frame/default_header_painter.cc
diff --git a/ash/frame/default_header_painter.cc
b/ash/frame/default_header_painter.cc
index
d76096190a20796f31f46cc5c23e1757bb7f3887..92381eefd408261b12333a4d133a10c40404e70a
100644
--- a/ash/frame/default_header_painter.cc
+++ b/ash/frame/default_header_painter.cc
@@ -77,6 +77,7 @@ DefaultHeaderPainter::DefaultHeaderPainter()
     : frame_(NULL),
       view_(NULL),
       window_icon_(NULL),
+      window_icon_size_(HeaderPainterUtil::GetDefaultIconSize()),
       caption_button_container_(NULL),
       height_(0),
       mode_(MODE_INACTIVE),
@@ -217,10 +218,10 @@ void DefaultHeaderPainter::LayoutHeader() {
   if (window_icon_) {
     // Vertically center the window icon with respect to the caption button
     // container.
-    int icon_size = HeaderPainterUtil::GetIconSize();
-    int icon_offset_y = (caption_button_container_->height() - icon_size) / 2;
+    int icon_offset_y =
+        (caption_button_container_->height() - window_icon_size_) / 2;
     window_icon_->SetBounds(HeaderPainterUtil::GetIconXOffset(), icon_offset_y,
-        icon_size, icon_size);
+                            window_icon_size_, window_icon_size_);
   }
 
   SetHeaderHeightForPainting(caption_button_container_->height() +
@@ -239,6 +240,12 @@ void DefaultHeaderPainter::SchedulePaintForTitle() {
   view_->SchedulePaintInRect(GetTitleBounds());
 }
 
+void DefaultHeaderPainter::UpdateWindowIcon(views::View* window_icon,
+                                            int window_icon_size) {
+  window_icon_ = window_icon;
+  window_icon_size_ = window_icon_size;
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // gfx::AnimationDelegate overrides:

[pkotwicz, 2014-03-20 15:31:40]

I should have notified you. You need to merge with
https://codereview.chromium.org/200493004/ which I landed this morning

[oshima, 2014-03-20 17:37:37]

On 2014/03/20 15:31:40, pkotwicz wrote:
> I should have notified you. You need to merge with
> https://codereview.chromium.org/200493004/ which I landed this morning

np, thank you for letting me know. rebased and will start CQ shortly

[oshima, 2014-03-20 17:38:59]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-03-20 17:39:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/200483004/350001

[commit-bot: I haz the power, 2014-03-20 17:48:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 17:49:01]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[oshima, 2014-03-20 20:06:52]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-03-20 20:07:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/200483004/350001

[commit-bot: I haz the power, 2014-03-20 21:38:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 21:38:56]

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

[oshima, 2014-03-20 23:23:51]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-03-20 23:24:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/200483004/350001

[commit-bot: I haz the power, 2014-03-21 02:22:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-21 02:22:40]

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

[oshima, 2014-03-21 02:45:53]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-03-21 02:46:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/200483004/350001

[commit-bot: I haz the power, 2014-03-21 05:51:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-21 05:51:04]

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

[oshima, 2014-03-21 06:46:14]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-03-21 06:46:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/200483004/370001

[commit-bot: I haz the power, 2014-03-21 12:48:47]

Change committed as 258550