Introduce new WebApp header style for hosted apps and fizzy apps on ash.

Introduce new WebApp header style for hosted apps and fizzy apps on ash.

The new header style currently has a back button and a title, as well as
the normal caption buttons (minimize, maximize / restore, close). The
new header style is only used in --enable-streamlined-hosted-apps is
enabled.

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

[benwells, 2014-08-05 07:29:07]

Chris, can you do a sanity check before I find an owner?

[benwells, 2014-08-06 01:32:44]

fixed a woopsie.

[benwells, 2014-08-06 01:34:02]

+oshima for binary stuff. When I run the optimize png script (with -o0 or -o2)
on the _inactive png, it removes its alpha channel. Have you seen that before?

[calamity, 2014-08-06 07:07:07]

Mostly nits, one name collision issue which may or may not be worth fixing.

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

https://codereview.chromium.org/441803004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:376: //
views::ButtonListener:
nit: CommandObserver.

https://codereview.chromium.org/441803004/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/webapp_header_painter_ash.h (right):

https://codereview.chromium.org/441803004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/webapp_header_painter_ash.h:18: }
nit: Move this above the views namespace.

https://codereview.chromium.org/441803004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/webapp_header_painter_ash.h:26: //
DefaultHeaderPainter does not take ownership of any of the parameters.
nit: // WebAppHeaderPainterAsh does not take ownership of any of the parameters.

https://codereview.chromium.org/441803004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/webapp_header_painter_ash.h:30:
ash::FrameCaptionButtonContainerView* caption_button_container);
It's a bit scary that this name collides with the parent class' method. You can
even call the parent's Init() with these args.

I'm not sure what the right way to deal with this is (or if it's even a big
issue).

[benwells, 2014-08-06 08:14:00]

All done.

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

https://codereview.chromium.org/441803004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:376: //
views::ButtonListener:
On 2014/08/06 07:07:06, calamity wrote:
> nit: CommandObserver.

Done.

https://codereview.chromium.org/441803004/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/webapp_header_painter_ash.h (right):

https://codereview.chromium.org/441803004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/webapp_header_painter_ash.h:18: }
On 2014/08/06 07:07:07, calamity wrote:
> nit: Move this above the views namespace.

Done.

https://codereview.chromium.org/441803004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/webapp_header_painter_ash.h:26: //
DefaultHeaderPainter does not take ownership of any of the parameters.
On 2014/08/06 07:07:07, calamity wrote:
> nit: // WebAppHeaderPainterAsh does not take ownership of any of the
parameters.

Done.

https://codereview.chromium.org/441803004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/webapp_header_painter_ash.h:30:
ash::FrameCaptionButtonContainerView* caption_button_container);
On 2014/08/06 07:07:07, calamity wrote:
> It's a bit scary that this name collides with the parent class' method. You
can
> even call the parent's Init() with these args.
> 
> I'm not sure what the right way to deal with this is (or if it's even a big
> issue).

Oh yeah, good catch. Done.

[benwells, 2014-08-06 08:16:21]

pkotwitz for owners of ash/frame. Before I send out more widely, is subclassing
DefaultHeaderPainter OK? Initially I had a separate implementation but there was
so much copied code.

Later the web app header will get more things and might make more sense to split
it out then.

[oshima, 2014-08-06 19:37:31]

On 2014/08/06 01:34:02, benwells wrote:
> +oshima for binary stuff. When I run the optimize png script (with -o0 or -o2)
> on the _inactive png, it removes its alpha channel. Have you seen that before?

no i don't. I'll look into it. 

It's missing 2x resources though. Can you get them from UX designer?

[pkotwicz, 2014-08-06 21:36:49]

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

https://codereview.chromium.org/441803004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:90:
webapp_back_button_(NULL),
Nit: Rename to web_app_back_button_

https://codereview.chromium.org/441803004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:128:
UpdateBackButtonState(false);
The back button should be enabled by default. Otherwise, the back button is
never enabled for a streamlined app opened via session-restore

https://codereview.chromium.org/441803004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:132:
WebAppHeaderPainterAsh* header_painter = new WebAppHeaderPainterAsh;
I would rather use DefaultHeaderPainter (as opposed to WebAppHeaderPainterAsh).
You can set |web_app_back_button_|'s images here too.

It is not an ideal solution, but I think that it is the least bad solution given
how little special logic is needed yet for streamlined apps.

https://codereview.chromium.org/441803004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:221: if
(hit_test == HTCAPTION && ((avatar_button() &&
Nit: Something like this might be more readable.

if (hit_test == HTCAPTION &&
    (ConvertPointToViewAndHitTest(this, avatar_button(), point) ||
     ConvertPointToViewAndHitTest(this, web_app_back_button_, point)) {
  return HTCLIENT;
}

where ConvertPointToViewAndHitTest() needs to be written and would have to
handle the NULL case

I have ommitted avatar_label() on purpose because it is not used on CrOS (e.g.
it is not laid out in BrowserNonClientFrameViewAsh::Layout() )

https://codereview.chromium.org/441803004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:282:
caption_button_container_->SetPaintAsActive(ShouldPaintAsActive());
To be consistent, you also need to change the |web_app_back_button_|'s paint as
active state here

https://codereview.chromium.org/441803004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:310:
header_painter_->SetHeaderHeightForPainting(painted_height);
When both the multi-profile avatar and the back button are visible, the avatar
and the button overlap and the back button is not clickable.

To make the multi-profile avatar appear:
- You must have at least two profiles which show up in the login page
- Once logged in, click on the user name in the ash tray. It should give you the
option to "Sign in another account".
- Right click on the window header to move a streamlined app to another profile

I am unsure what the best solution is. What makes it hard is that LayoutAvatar()
depends on GetTopInset() having been computed which depends on
HeaderPainter::LayoutHeader() having been run.

Oshima, do you have any ideas?

https://codereview.chromium.org/441803004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:459: // apps
have the experimental WebApp header style.
Nit: Maybe change the return value to:
return browser_view()->browser()->is_app() && !UseWebAppHeaderStyle();

Doing this should enable you to remove the comment wrt to streamlined hosted
apps

[benwells, 2014-08-11 07:06:16]

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

https://codereview.chromium.org/441803004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:132:
WebAppHeaderPainterAsh* header_painter = new WebAppHeaderPainterAsh;
On 2014/08/06 21:36:48, pkotwicz wrote:
> I would rather use DefaultHeaderPainter (as opposed to
WebAppHeaderPainterAsh).
> You can set |web_app_back_button_|'s images here too.
> 
> It is not an ideal solution, but I think that it is the least bad solution
given
> how little special logic is needed yet for streamlined apps.

Yeah there is currently little special logic. I'll send you mocks so you can see
where it is heading.

If I use the DefaultHeaderPainter, I don't think it will get the title bounds
correct. Are you suggesting I replace the window_icon field in
DefaultHeaderPainter with something like left_header_view_ and make it handle
both the back button or the window icon?

[pkotwicz, 2014-08-11 16:49:22]

On 2014/08/11 07:06:16, benwells wrote:
>
https://codereview.chromium.org/441803004/diff/100001/chrome/browser/ui/views...
> File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc
(right):
> 
>
https://codereview.chromium.org/441803004/diff/100001/chrome/browser/ui/views...
> chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:132:
> WebAppHeaderPainterAsh* header_painter = new WebAppHeaderPainterAsh;
> On 2014/08/06 21:36:48, pkotwicz wrote:
> > I would rather use DefaultHeaderPainter (as opposed to
> WebAppHeaderPainterAsh).
> > You can set |web_app_back_button_|'s images here too.
> > 
> > It is not an ideal solution, but I think that it is the least bad solution
> given
> > how little special logic is needed yet for streamlined apps.
> 
> Yeah there is currently little special logic. I'll send you mocks so you can
see
> where it is heading.
> 
> If I use the DefaultHeaderPainter, I don't think it will get the title bounds
> correct. Are you suggesting I replace the window_icon field in
> DefaultHeaderPainter with something like left_header_view_ and make it handle
> both the back button or the window icon?

Yes, I was thinking of that. You also need to make sure that |left_header_view_|
is correctly positioned wrt to the multiprofile avatar icon

[benwells, 2014-08-12 11:17:29]

On 2014/08/06 19:37:31, oshima wrote:
> On 2014/08/06 01:34:02, benwells wrote:
> > +oshima for binary stuff. When I run the optimize png script (with -o0 or
-o2)
> > on the _inactive png, it removes its alpha channel. Have you seen that
before?
> 
> no i don't. I'll look into it. 
> 
> It's missing 2x resources though. Can you get them from UX designer?

I'm still waiting for the proper assets - I made the 1x myself. About the weird
corruption, is it OK to bring these in and worry about it if it happens with the
proper assets, when they're available?

[benwells, 2014-08-12 11:17:30]

On 2014/08/06 19:37:31, oshima wrote:
> On 2014/08/06 01:34:02, benwells wrote:
> > +oshima for binary stuff. When I run the optimize png script (with -o0 or
-o2)
> > on the _inactive png, it removes its alpha channel. Have you seen that
before?
> 
> no i don't. I'll look into it. 
> 
> It's missing 2x resources though. Can you get them from UX designer?

I'm still waiting for the proper assets - I made the 1x myself. About the weird
corruption, is it OK to bring these in and worry about it if it happens with the
proper assets, when they're available?

[pkotwicz, 2014-08-12 23:58:30]

benwells@, did you send me the mocks?

[benwells, 2014-08-13 00:53:45]

Mocks sent offline.

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

https://codereview.chromium.org/441803004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:90:
webapp_back_button_(NULL),
On 2014/08/06 21:36:49, pkotwicz wrote:
> Nit: Rename to web_app_back_button_

Done.

https://codereview.chromium.org/441803004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:128:
UpdateBackButtonState(false);
On 2014/08/06 21:36:48, pkotwicz wrote:
> The back button should be enabled by default. Otherwise, the back button is
> never enabled for a streamlined app opened via session-restore

Done.

https://codereview.chromium.org/441803004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:132:
WebAppHeaderPainterAsh* header_painter = new WebAppHeaderPainterAsh;
On 2014/08/11 07:06:15, benwells wrote:
> On 2014/08/06 21:36:48, pkotwicz wrote:
> > I would rather use DefaultHeaderPainter (as opposed to
> WebAppHeaderPainterAsh).
> > You can set |web_app_back_button_|'s images here too.
> > 
> > It is not an ideal solution, but I think that it is the least bad solution
> given
> > how little special logic is needed yet for streamlined apps.
> 
> Yeah there is currently little special logic. I'll send you mocks so you can
see
> where it is heading.
> 
> If I use the DefaultHeaderPainter, I don't think it will get the title bounds
> correct. Are you suggesting I replace the window_icon field in
> DefaultHeaderPainter with something like left_header_view_ and make it handle
> both the back button or the window icon?

Done.

https://codereview.chromium.org/441803004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:221: if
(hit_test == HTCAPTION && ((avatar_button() &&
On 2014/08/06 21:36:49, pkotwicz wrote:
> Nit: Something like this might be more readable.
> 
> if (hit_test == HTCAPTION &&
>     (ConvertPointToViewAndHitTest(this, avatar_button(), point) ||
>      ConvertPointToViewAndHitTest(this, web_app_back_button_, point)) {
>   return HTCLIENT;
> }
> 
> where ConvertPointToViewAndHitTest() needs to be written and would have to
> handle the NULL case
> 
> I have ommitted avatar_label() on purpose because it is not used on CrOS (e.g.
> it is not laid out in BrowserNonClientFrameViewAsh::Layout() )

Done.

https://codereview.chromium.org/441803004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:282:
caption_button_container_->SetPaintAsActive(ShouldPaintAsActive());
On 2014/08/06 21:36:48, pkotwicz wrote:
> To be consistent, you also need to change the |web_app_back_button_|'s paint
as
> active state here

Done.

https://codereview.chromium.org/441803004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:310:
header_painter_->SetHeaderHeightForPainting(painted_height);
On 2014/08/06 21:36:48, pkotwicz wrote:
> When both the multi-profile avatar and the back button are visible, the avatar
> and the button overlap and the back button is not clickable.
> 
> To make the multi-profile avatar appear:
> - You must have at least two profiles which show up in the login page
> - Once logged in, click on the user name in the ash tray. It should give you
the
> option to "Sign in another account".
> - Right click on the window header to move a streamlined app to another
profile
> 
> I am unsure what the best solution is. What makes it hard is that
LayoutAvatar()
> depends on GetTopInset() having been computed which depends on
> HeaderPainter::LayoutHeader() having been run.
> 
> Oshima, do you have any ideas?

OK, I'm now taking this into account. It feels a bit yuck. Let me know what you
think, I'm wondering if it would be better if DefaultHeaderPainter knew about
avatars in some way (like it knew about window icons / now knows about left
header views).

https://codereview.chromium.org/441803004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:459: // apps
have the experimental WebApp header style.
On 2014/08/06 21:36:48, pkotwicz wrote:
> Nit: Maybe change the return value to:
> return browser_view()->browser()->is_app() && !UseWebAppHeaderStyle();
> 
> Doing this should enable you to remove the comment wrt to streamlined hosted
> apps

Done.

[pkotwicz, 2014-08-13 01:19:48]

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

https://codereview.chromium.org/441803004/diff/160001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash.cc:16: #include
"ash/frame/header_painter_util.h"
Nit: Remove unnecessary include

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

https://codereview.chromium.org/441803004/diff/160001/ash/frame/default_heade...
ash/frame/default_header_painter.cc:229: void
DefaultHeaderPainter::UpdateHeaderOffset(int left_header_view_offset) {
I think that |left_view_x_inset| is a better name. In particular,
|left_view_x_inset| does not apply to the header background which is also drawn
by this class
UpdateLeftViewXInset() is probably a better function name too

https://codereview.chromium.org/441803004/diff/160001/ash/frame/default_heade...
ash/frame/default_header_painter.cc:321: : HeaderPainterUtil::GetIconXOffset();
Nit: I think that checking equality to zero here would be clearer. But I could
be convinced otherwise.

https://codereview.chromium.org/441803004/diff/160001/ash/frame/default_heade...
ash/frame/default_header_painter.cc:323: offset, icon_offset_y,
left_header_view_size_, left_header_view_size_);
Can we use left_header_view_->GetPreferredSize() and get rid of the
|left_header_view_size_| member variable completely?

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

https://codereview.chromium.org/441803004/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:74: 
I was thinking of something similar to ConvertedHitTest() in
browser_view_layout.cc
I do not want us to assume what the parent of |view| is.

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

https://codereview.chromium.org/441803004/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:627:
web_app_back_button_->set_paint_as_active(enabled);
This is not right.
- It might be possible for UpdateBackButtonState() to be called while the
underlying browser widget is not active. (It might be possible to add a tab to a
browser window without activating the browser). In this case,
|web_app_back_button_| should not paint as active
- Can you please confirm with the UX designers what we want the disabled button
to look like? I am fine with you assuming that the disabled button looks the
same as the button does when the widget is inactive for the sake of getting
something in for the designers to look at. (With a comment saying that we are
assuming this of course)

[benwells, 2014-08-13 08:09:28]

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

https://codereview.chromium.org/441803004/diff/160001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash.cc:16: #include
"ash/frame/header_painter_util.h"
On 2014/08/13 01:19:47, pkotwicz wrote:
> Nit: Remove unnecessary include

Done.

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

https://codereview.chromium.org/441803004/diff/160001/ash/frame/default_heade...
ash/frame/default_header_painter.cc:229: void
DefaultHeaderPainter::UpdateHeaderOffset(int left_header_view_offset) {
On 2014/08/13 01:19:47, pkotwicz wrote:
> I think that |left_view_x_inset| is a better name. In particular,
> |left_view_x_inset| does not apply to the header background which is also
drawn
> by this class
> UpdateLeftViewXInset() is probably a better function name too

Done.

https://codereview.chromium.org/441803004/diff/160001/ash/frame/default_heade...
ash/frame/default_header_painter.cc:321: : HeaderPainterUtil::GetIconXOffset();
On 2014/08/13 01:19:47, pkotwicz wrote:
> Nit: I think that checking equality to zero here would be clearer. But I could
> be convinced otherwise.

Done.

https://codereview.chromium.org/441803004/diff/160001/ash/frame/default_heade...
ash/frame/default_header_painter.cc:323: offset, icon_offset_y,
left_header_view_size_, left_header_view_size_);
On 2014/08/13 01:19:47, pkotwicz wrote:
> Can we use left_header_view_->GetPreferredSize() and get rid of the
> |left_header_view_size_| member variable completely?

Seems so. The code sure is nicer! I hope it doesn't break panels or the custom
frame stuff, I'm not sure how to test that on a local build.

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

https://codereview.chromium.org/441803004/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:74: 
On 2014/08/13 01:19:47, pkotwicz wrote:
> I was thinking of something similar to ConvertedHitTest() in
> browser_view_layout.cc
> I do not want us to assume what the parent of |view| is.

Done.

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

https://codereview.chromium.org/441803004/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:627:
web_app_back_button_->set_paint_as_active(enabled);
On 2014/08/13 01:19:48, pkotwicz wrote:
> This is not right.
> - It might be possible for UpdateBackButtonState() to be called while the
> underlying browser widget is not active. (It might be possible to add a tab to
a
> browser window without activating the browser). In this case,
> |web_app_back_button_| should not paint as active
> - Can you please confirm with the UX designers what we want the disabled
button
> to look like? I am fine with you assuming that the disabled button looks the
> same as the button does when the widget is inactive for the sake of getting
> something in for the designers to look at. (With a comment saying that we are
> assuming this of course)

Done.

[pkotwicz, 2014-08-14 01:59:45]

I think you can get an OWNER for the next review.
Of course, you still need 2x assets

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

https://codereview.chromium.org/441803004/diff/220001/ash/frame/header_painte...
ash/frame/header_painter.h:45: // inset.
Nit: "Updates the x inset of the leftmost view in the header."

https://codereview.chromium.org/441803004/diff/220001/ash/frame/header_painte...
File ash/frame/header_painter_util.h (right):

https://codereview.chromium.org/441803004/diff/220001/ash/frame/header_painte...
ash/frame/header_painter_util.h:34: static int GetDefaultIconSize();
We should be able to get rid of this method entirely

https://codereview.chromium.org/441803004/diff/220001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_header_painter_ash.cc (right):

https://codereview.chromium.org/441803004/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_header_painter_ash.cc:268: // Not
implemented.
This should be implemented

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

https://codereview.chromium.org/441803004/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:75: //
Combines View::ConvertPointToTarget and View::HitTest for a given |point|.
Nit: "View::ConvertPointToTarget() and View::HitTest()"

https://codereview.chromium.org/441803004/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:245:
ConvertedHitTest(this, web_app_back_button_, point)))
Nit: I like braces with multi-line if statements

https://codereview.chromium.org/441803004/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:339:
avatar_button()->bounds().width());
Nit: bounds().right()

https://codereview.chromium.org/441803004/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:341:
header_painter_->UpdateLeftViewXInset(0);
I think it would be more readable if we passed in
ash::HeaderPainterUtil::GetIconXOffset()

That method needs a new name. ash::HeaderPainterUtil::GetDefaultLeftViewXInset()
perhaps?

https://codereview.chromium.org/441803004/diff/220001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h (right):

https://codereview.chromium.org/441803004/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h:32: public
views::ButtonListener {
Shouldn't we be including ui/views/controls/button/button.h

https://codereview.chromium.org/441803004/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h:139: // View
which contains the back button for WebApp style header.
How about: "The back button for the web app style header."

[benwells, 2014-08-14 04:42:50]

jamescook - ptal for owners for ash stuff in c/b/ui/views/frame
sky - ptal for browser.cc and browser_view.cc

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

https://codereview.chromium.org/441803004/diff/220001/ash/frame/header_painte...
ash/frame/header_painter.h:45: // inset.
On 2014/08/14 01:59:44, pkotwicz wrote:
> Nit: "Updates the x inset of the leftmost view in the header."

Done.

https://codereview.chromium.org/441803004/diff/220001/ash/frame/header_painte...
File ash/frame/header_painter_util.h (right):

https://codereview.chromium.org/441803004/diff/220001/ash/frame/header_painte...
ash/frame/header_painter_util.h:34: static int GetDefaultIconSize();
On 2014/08/14 01:59:44, pkotwicz wrote:
> We should be able to get rid of this method entirely

Done.

https://codereview.chromium.org/441803004/diff/220001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_header_painter_ash.cc (right):

https://codereview.chromium.org/441803004/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_header_painter_ash.cc:268: // Not
implemented.
On 2014/08/14 01:59:44, pkotwicz wrote:
> This should be implemented

Done.

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

https://codereview.chromium.org/441803004/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:75: //
Combines View::ConvertPointToTarget and View::HitTest for a given |point|.
On 2014/08/14 01:59:44, pkotwicz wrote:
> Nit: "View::ConvertPointToTarget() and View::HitTest()"

Done.

https://codereview.chromium.org/441803004/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:245:
ConvertedHitTest(this, web_app_back_button_, point)))
On 2014/08/14 01:59:44, pkotwicz wrote:
> Nit: I like braces with multi-line if statements

Done.

https://codereview.chromium.org/441803004/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:339:
avatar_button()->bounds().width());
On 2014/08/14 01:59:44, pkotwicz wrote:
> Nit: bounds().right()

Done.

https://codereview.chromium.org/441803004/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:341:
header_painter_->UpdateLeftViewXInset(0);
On 2014/08/14 01:59:44, pkotwicz wrote:
> I think it would be more readable if we passed in
> ash::HeaderPainterUtil::GetIconXOffset()
> 
> That method needs a new name.
ash::HeaderPainterUtil::GetDefaultLeftViewXInset()
> perhaps?

Done.

https://codereview.chromium.org/441803004/diff/220001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h (right):

https://codereview.chromium.org/441803004/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h:32: public
views::ButtonListener {
On 2014/08/14 01:59:44, pkotwicz wrote:
> Shouldn't we be including ui/views/controls/button/button.h

Done.

https://codereview.chromium.org/441803004/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h:139: // View
which contains the back button for WebApp style header.
On 2014/08/14 01:59:44, pkotwicz wrote:
> How about: "The back button for the web app style header."

Done.

[sky, 2014-08-14 16:49:36]

browser.cc and browser_view.cc LGTM

[James Cook, 2014-08-15 00:00:47]

c/b/ui/views/frame/* LGTM with a few suggestions

Please make sure pkotwicz is satisfied before landing.

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

https://codereview.chromium.org/441803004/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:77: //
Converts |point| from |src| to |dst| and hit tests it against |dst|.
wrap comment together

https://codereview.chromium.org/441803004/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:113:
chrome::RemoveCommandObserver(browser_view()->browser(), IDC_BACK, this);
You might comment here on the memory ownership (why browser_view() outlives
|this|).  I had to add logging to my build to satisfy myself that the lifetimes
were correct.

https://codereview.chromium.org/441803004/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:156:
header_painter_.reset(header_painter);
Collapse this with the line above and use header_painter_->foo() below.  Feel
free to do the same below.

https://codereview.chromium.org/441803004/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:246:
ConvertedHitTest(this, web_app_back_button_, point))) {
I would prefer to see this unrolled as two hit tests, skipping the second one if
the web app back button doesn't exist.  Otherwise I have to know that it's OK to
hit test against NULL.

This would also let you DCHECK(src), DCHECK(dst) in the ConvertedHitTest()
function.

[benwells, 2014-08-18 05:59:13]

pkotwicz: still waiting for your lg
oshima: owners for binaries.

oshima - fyi the crushing weirdness also involved the image being converted to
greyscale. I 'fixed' it by making some very faint grey pixels very faint
slightly off grey instead.

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

https://codereview.chromium.org/441803004/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:77: //
Converts |point| from |src| to |dst| and hit tests it against |dst|.
On 2014/08/15 00:00:47, James Cook wrote:
> wrap comment together

Done.

https://codereview.chromium.org/441803004/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:113:
chrome::RemoveCommandObserver(browser_view()->browser(), IDC_BACK, this);
On 2014/08/15 00:00:47, James Cook wrote:
> You might comment here on the memory ownership (why browser_view() outlives
> |this|).  I had to add logging to my build to satisfy myself that the
lifetimes
> were correct.

Done.

https://codereview.chromium.org/441803004/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:156:
header_painter_.reset(header_painter);
On 2014/08/15 00:00:47, James Cook wrote:
> Collapse this with the line above and use header_painter_->foo() below.  Feel
> free to do the same below.

Init and UpdateLeftHeaderView are not on the HeaderPainter interface, which is
why these variables are typed. Note the signatures of DefaultHeaderPainter and
BrowserHeaderPainterAsh are also different (3 args versus 5).

https://codereview.chromium.org/441803004/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:246:
ConvertedHitTest(this, web_app_back_button_, point))) {
On 2014/08/15 00:00:47, James Cook wrote:
> I would prefer to see this unrolled as two hit tests, skipping the second one
if
> the web app back button doesn't exist.  Otherwise I have to know that it's OK
to
> hit test against NULL.
> 
> This would also let you DCHECK(src), DCHECK(dst) in the ConvertedHitTest()
> function.

Done.

[pkotwicz, 2014-08-19 00:17:07]

LGTM with nits

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

https://codereview.chromium.org/441803004/diff/260001/ash/frame/default_heade...
ash/frame/default_header_painter.cc:232: LayoutLeftHeaderView();
Nit - This is slightly shorter:

if (left_view_x_inset_ != left_view_x_inset) {
  left_view_x_inset_ = left_view_x_inset;
  LayoutLeftHeaderView();
}

https://codereview.chromium.org/441803004/diff/260001/ash/frame/default_heade...
ash/frame/default_header_painter.cc:316: left_header_view_->height() / 2;
Shouldn't this be size.height() instead of left_header_view_->height() ?

https://codereview.chromium.org/441803004/diff/260001/ash/frame/default_heade...
File ash/frame/default_header_painter_unittest.cc (right):

https://codereview.chromium.org/441803004/diff/260001/ash/frame/default_heade...
ash/frame/default_header_painter_unittest.cc:8: #include
"ash/frame/header_painter_util.h"
This include is unnecessary. Remove it

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

https://codereview.chromium.org/441803004/diff/260001/ash/frame/header_painte...
ash/frame/header_painter.h:44: // Updates the x inset of the leftmost view of
the header.
Nit: of the header -> in the header

https://codereview.chromium.org/441803004/diff/260001/ash/frame/header_painte...
File ash/frame/header_painter_util.h (right):

https://codereview.chromium.org/441803004/diff/260001/ash/frame/header_painte...
ash/frame/header_painter_util.h:30: // icon.
Can you please update the stale comment?

https://codereview.chromium.org/441803004/diff/260001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_header_painter_ash.h (right):

https://codereview.chromium.org/441803004/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_header_painter_ash.h:106: int
left_view_x_inset_;
Nit: Rename to window_icon_x_inset_

[oshima, 2014-08-19 00:36:58]

ash/resources lgtm

[benwells, 2014-08-19 01:42:43]

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

https://codereview.chromium.org/441803004/diff/260001/ash/frame/default_heade...
ash/frame/default_header_painter.cc:232: LayoutLeftHeaderView();
On 2014/08/19 00:17:07, pkotwicz wrote:
> Nit - This is slightly shorter:
> 
> if (left_view_x_inset_ != left_view_x_inset) {
>   left_view_x_inset_ = left_view_x_inset;
>   LayoutLeftHeaderView();
> }

Done.

https://codereview.chromium.org/441803004/diff/260001/ash/frame/default_heade...
ash/frame/default_header_painter.cc:316: left_header_view_->height() / 2;
On 2014/08/19 00:17:07, pkotwicz wrote:
> Shouldn't this be size.height() instead of left_header_view_->height() ?

Done.

https://codereview.chromium.org/441803004/diff/260001/ash/frame/default_heade...
File ash/frame/default_header_painter_unittest.cc (right):

https://codereview.chromium.org/441803004/diff/260001/ash/frame/default_heade...
ash/frame/default_header_painter_unittest.cc:8: #include
"ash/frame/header_painter_util.h"
On 2014/08/19 00:17:07, pkotwicz wrote:
> This include is unnecessary. Remove it

Done.

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

https://codereview.chromium.org/441803004/diff/260001/ash/frame/header_painte...
ash/frame/header_painter.h:44: // Updates the x inset of the leftmost view of
the header.
On 2014/08/19 00:17:07, pkotwicz wrote:
> Nit: of the header -> in the header

Done.

https://codereview.chromium.org/441803004/diff/260001/ash/frame/header_painte...
File ash/frame/header_painter_util.h (right):

https://codereview.chromium.org/441803004/diff/260001/ash/frame/header_painte...
ash/frame/header_painter_util.h:30: // icon.
On 2014/08/19 00:17:07, pkotwicz wrote:
> Can you please update the stale comment?

Done.

https://codereview.chromium.org/441803004/diff/260001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_header_painter_ash.h (right):

https://codereview.chromium.org/441803004/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_header_painter_ash.h:106: int
left_view_x_inset_;
On 2014/08/19 00:17:07, pkotwicz wrote:
> Nit: Rename to window_icon_x_inset_

Done.

[benwells, 2014-08-19 04:30:28]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2014-08-19 04:32:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/benwells@chromium.org/441803004/300001

[commit-bot: I haz the power, 2014-08-19 05:48:30]

Committed patchset #15 (300001) as 290502