[Refactor] Move code for painting the window header for browser windows out of ash

[Refactor] Move code for painting the window header for browser windows out of ash

BUG=340143
TEST=None

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

[pkotwicz, 2014-03-09 05:06:39]

James, PTAL

[James Cook, 2014-03-10 17:32:30]

A few comments.

https://codereview.chromium.org/189463013/diff/10022/ash/wm/custom_frame_view...
File ash/wm/custom_frame_view_ash_unittest.cc (right):

https://codereview.chromium.org/189463013/diff/10022/ash/wm/custom_frame_view...
ash/wm/custom_frame_view_ash_unittest.cc:50: TEST_F(CustomFrameViewAshTest,
HeaderHeight) {
Not for this CL but it would be nice to someday have tests for
GetBoundsForClientView() and GetWindowBoundsForClientBounds(). For some reason
those functions always make me nervous.

https://codereview.chromium.org/189463013/diff/10022/ash/wm/default_header_pa...
File ash/wm/default_header_painter.cc (right):

https://codereview.chromium.org/189463013/diff/10022/ash/wm/default_header_pa...
ash/wm/default_header_painter.cc:167: if (initial_paint_ || parent_animating) {
I'm a little confused about the usage of initial_paint_ here. If a window is
created in the inactive state (is that possible?) then later activated, you'll
see mode_ != old_mode but not start an animation?

https://codereview.chromium.org/189463013/diff/10022/ash/wm/default_header_pa...
File ash/wm/default_header_painter.h (right):

https://codereview.chromium.org/189463013/diff/10022/ash/wm/default_header_pa...
ash/wm/default_header_painter.h:26: class ASH_EXPORT DefaultHeaderPainter :
public HeaderPainter,
Needs a class comment.

https://codereview.chromium.org/189463013/diff/10022/ash/wm/default_header_pa...
ash/wm/default_header_painter.h:73: int height_;
nit: Comment which height this is.

https://codereview.chromium.org/189463013/diff/10022/ash/wm/default_header_pa...
ash/wm/default_header_painter.h:81: scoped_ptr<gfx::SlideAnimation>
activation_animation_;
Nice, I like this name better.

https://codereview.chromium.org/189463013/diff/10022/ash/wm/frame_border_hit_...
File ash/wm/frame_border_hit_test_controller.cc (right):

https://codereview.chromium.org/189463013/diff/10022/ash/wm/frame_border_hit_...
ash/wm/frame_border_hit_test_controller.cc:35: const gfx::Point& point) {
optional cleanup: maybe point_in_widget?

https://codereview.chromium.org/189463013/diff/10022/ash/wm/header_metrics.cc
File ash/wm/header_metrics.cc (right):

https://codereview.chromium.org/189463013/diff/10022/ash/wm/header_metrics.cc...
ash/wm/header_metrics.cc:1: #include "ash/wm/header_metrics.h"
needs copyright

https://codereview.chromium.org/189463013/diff/10022/ash/wm/header_metrics.cc...
ash/wm/header_metrics.cc:43: return kTopCornerRadiusWhenRestored;
Optional: Any particular reason these are returned by function calls rather than
just having constants?  I could see this whole class replaced with a namespace
"header_metrics" and these being constants.

https://codereview.chromium.org/189463013/diff/10022/ash/wm/header_metrics.h
File ash/wm/header_metrics.h (right):

https://codereview.chromium.org/189463013/diff/10022/ash/wm/header_metrics.h#...
ash/wm/header_metrics.h:37: // |window_icon| should  NULL if the header does not
use an icon.
nit: "should be"

https://codereview.chromium.org/189463013/diff/10022/ash/wm/header_metrics.h#...
ash/wm/header_metrics.h:38: static gfx::Rect GetTitleBounds(const views::View*
icon,
either |window_icon| or change comment above

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

https://codereview.chromium.org/189463013/diff/10022/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_header_painter_ash.cc:158: void
BrowserHeaderPainterAsh::PaintHeader(gfx::Canvas* canvas, Mode mode) {
Is there any way to share a little more code between this function and the one
in default_header_painter?

https://codereview.chromium.org/189463013/diff/10022/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_header_painter_ash.cc:163: aura::Window*
parent = frame_->GetNativeWindow()->parent();
Like maybe this parent-animating check could go in HeaderMetrics (which could
also be called header_util)?

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

https://codereview.chromium.org/189463013/diff/10022/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_header_painter_ash.h:28: class
BrowserHeaderPainterAsh : public ash::HeaderPainter,
Class comment please

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

https://codereview.chromium.org/189463013/diff/10022/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:112:
header_painter_.reset(header_painter);
Out of curiosity, why reset after Init()?  Does header_painter_ need to have a
different value during DefaultHeaderPainter::Init()?  If so, comment about that.

[pkotwicz, 2014-03-14 17:56:21]

James, can you please take another look?

I made the changes that you requested + merged with
https://codereview.chromium.org/199283004/

https://codereview.chromium.org/189463013/diff/10022/ash/wm/default_header_pa...
File ash/wm/default_header_painter.cc (right):

https://codereview.chromium.org/189463013/diff/10022/ash/wm/default_header_pa...
ash/wm/default_header_painter.cc:167: if (initial_paint_ || parent_animating) {
It is possible for session restore to restore > 1 windows. I want to avoid
animating the window header in this case.

https://codereview.chromium.org/189463013/diff/10022/ash/wm/header_metrics.cc
File ash/wm/header_metrics.cc (right):

https://codereview.chromium.org/189463013/diff/10022/ash/wm/header_metrics.cc...
ash/wm/header_metrics.cc:43: return kTopCornerRadiusWhenRestored;
Given that this class is now HeaderPainterUtil, I am not sure if your suggestion
still makes sense? (I presume your suggestion is to have both HeaderPainterUtil
and a constant only .h file) I want to avoid having too many header painting
related classes

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

https://codereview.chromium.org/189463013/diff/10022/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_header_painter_ash.cc:163: aura::Window*
parent = frame_->GetNativeWindow()->parent();
Done. I renamed HeaderMetrics to HeaderPainterUtil.
Is there any other functionality that you want me to move to HeaderPainterUtil?

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

https://codereview.chromium.org/189463013/diff/10022/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:112:
header_painter_.reset(header_painter);
Init is not part of the ash::HeaderPainter interface.
ash::DefaultHeaderPainter::Init() and BrowserHeaderPainterAsh::Init() take
different numbers of arguments. I set |header_painter_| after initing to avoid a
static_cast.

[James Cook, 2014-03-14 18:27:15]

LGTM

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

https://codereview.chromium.org/189463013/diff/10022/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_header_painter_ash.cc:163: aura::Window*
parent = frame_->GetNativeWindow()->parent();
On 2014/03/14 17:56:22, pkotwicz wrote:
> Done. I renamed HeaderMetrics to HeaderPainterUtil.
> Is there any other functionality that you want me to move to
HeaderPainterUtil?

It would be nice if even more of this code could be shared with
DefaultHeaderPainter, but if you're OK with this structure I'm OK with it.

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

https://codereview.chromium.org/189463013/diff/10022/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:112:
header_painter_.reset(header_painter);
On 2014/03/14 17:56:22, pkotwicz wrote:
> Init is not part of the ash::HeaderPainter interface.
> ash::DefaultHeaderPainter::Init() and BrowserHeaderPainterAsh::Init() take
> different numbers of arguments. I set |header_painter_| after initing to avoid
a
> static_cast.

That makes sense. It just looked odd to my reading. Please add a comment.

[pkotwicz, 2014-03-16 17:27:17]

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

https://codereview.chromium.org/189463013/diff/10022/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:112:
header_painter_.reset(header_painter);
I swapped the order of lines 110 and 112. Hopefully this will make things
clearer

[pkotwicz, 2014-03-16 17:39:45]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-03-16 17:39:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/189463013/260001

[commit-bot: I haz the power, 2014-03-16 19:05:30]

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

[commit-bot: I haz the power, 2014-03-16 19:05:30]

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

[pkotwicz, 2014-03-16 20:06:43]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-03-16 20:06:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/189463013/260001

[commit-bot: I haz the power, 2014-03-16 20:15:02]

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

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

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

[pkotwicz, 2014-03-17 14:29:25]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-03-17 14:29:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/189463013/260001

[commit-bot: I haz the power, 2014-03-17 15:50:04]

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

[commit-bot: I haz the power, 2014-03-17 15:50:05]

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

[pkotwicz, 2014-03-17 16:51:12]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-03-17 16:51:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/189463013/260001

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

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

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

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[pkotwicz, 2014-03-17 21:15:29]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-03-17 21:16:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/189463013/260001

[commit-bot: I haz the power, 2014-03-17 21:19:00]

Change committed as 257494