Make packaged apps use AppNonClientFrameViewAsh when maximized

Adds a command line flag --ash-use-same-frame-for-v1-and-v2-apps which makes packaged apps use AppNonClientFrameViewAsh when maximized.

BUG=297174
TEST=Manual, see bug

[pkotwicz, 2013-10-02 19:41:49]

James, can you please take a look?

This CL depends on https://codereview.chromium.org/25535005/

https://codereview.chromium.org/25536010/diff/14001/ash/wm/caption_buttons/hi...
File ash/wm/caption_buttons/hideable_caption_button_container.h (right):

https://codereview.chromium.org/25536010/diff/14001/ash/wm/caption_buttons/hi...
ash/wm/caption_buttons/hideable_caption_button_container.h:25: class ASH_EXPORT
HideableCaptionButtonContainer : public views::WidgetObserver {
Do you have a suggestion for a better class name? (I realize the name is not
very correct because the controls are not hideable yet, but I am planning on
changing that very shortly and I want to avoid needless code churn)
I thought of HideableCaptionButtonContainerController but it is too long.
I would rather not use HideableCaptionButtonWidget given that the class adds a
views::WidgetObserver on |frame_|, not |button_widget_|.

https://codereview.chromium.org/25536010/diff/14001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/app_non_client_frame_view_ash.cc (left):

https://codereview.chromium.org/25536010/diff/14001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/app_non_client_frame_view_ash.cc:132:
frame->GetNativeView()->set_hit_test_bounds_override_inner(gfx::Insets());
I removed this call because an empty gfx::Insets() is the default.

https://codereview.chromium.org/25536010/diff/14001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/app_non_client_frame_view_ash.cc:135:
set_background(views::Background::CreateSolidBackground(SK_ColorBLACK));
I believe that setting the background is no longer needed. I believe that
setting it dates back to when AppNonClientFrameViewAsh had a 1px top border
which we used to hide and show the top controls

https://codereview.chromium.org/25536010/diff/14001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_frame_ash.cc (left):

https://codereview.chromium.org/25536010/diff/14001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_frame_ash.cc:59: // Don't do anything if
we don't have our non-client view yet.
Removed this method because it is obsolete

https://codereview.chromium.org/25536010/diff/14001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_factory_aura.cc
(right):

https://codereview.chromium.org/25536010/diff/14001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_non_client_frame_view_factory_aura.cc:34:
browser_view->browser()->app_type() != Browser::APP_TYPE_CHILD &&
I am trying to figure out whether we can remove this check. I am currently
blocked on getting design direction from kuscher@. http://crbug.com/302712 (and
candidate CL at https://codereview.chromium.org/25565002/)

[James Cook, 2013-10-02 21:07:55]

https://codereview.chromium.org/25536010/diff/14001/ash/wm/caption_buttons/hi...
File ash/wm/caption_buttons/hideable_caption_button_container.cc (right):

https://codereview.chromium.org/25536010/diff/14001/ash/wm/caption_buttons/hi...
ash/wm/caption_buttons/hideable_caption_button_container.cc:23: // The size of
the shadow around the caption buttons.
If this is related to a particular art asset you might mention that art asset's
ID here.  (I seem to remember there was some resource this had to match)

https://codereview.chromium.org/25536010/diff/14001/ash/wm/caption_buttons/hi...
ash/wm/caption_buttons/hideable_caption_button_container.cc:87:
HideableCaptionButtonContainer::HideableCaptionButtonContainer(
Can you add a basic test for this class, something that instantiates it and
makes one or two basic assertions?  It'll help the next guy who needs to add a
more complex test.  Which will probably be you.  :-)

https://codereview.chromium.org/25536010/diff/14001/ash/wm/caption_buttons/hi...
ash/wm/caption_buttons/hideable_caption_button_container.cc:137: int x =
base::i18n::IsRTL() ? 0 : frame_bounds.width() - size.width();
It's great you're careful about handling RTL. +1!

https://codereview.chromium.org/25536010/diff/14001/ash/wm/caption_buttons/hi...
File ash/wm/caption_buttons/hideable_caption_button_container.h (right):

https://codereview.chromium.org/25536010/diff/14001/ash/wm/caption_buttons/hi...
ash/wm/caption_buttons/hideable_caption_button_container.h:23: //
TODO(pkotwicz): Auto hide the buttons when they are not in use and reveal
Just to double-check, are we sure we want to hide the close button? Is there a
design doc on window controls I can look at?

https://codereview.chromium.org/25536010/diff/14001/ash/wm/caption_buttons/hi...
ash/wm/caption_buttons/hideable_caption_button_container.h:25: class ASH_EXPORT
HideableCaptionButtonContainer : public views::WidgetObserver {
On 2013/10/02 19:41:49, pkotwicz wrote:
> Do you have a suggestion for a better class name? (I realize the name is not
> very correct because the controls are not hideable yet, but I am planning on
> changing that very shortly and I want to avoid needless code churn)
> I thought of HideableCaptionButtonContainerController but it is too long.
> I would rather not use HideableCaptionButtonWidget given that the class adds a
> views::WidgetObserver on |frame_|, not |button_widget_|.

HideableCaptionButtonController?  Or just keep it the way it is.

https://codereview.chromium.org/25536010/diff/14001/ash/wm/caption_buttons/hi...
ash/wm/caption_buttons/hideable_caption_button_container.h:29: //
|background_id| is the idr of the image to use as the background.
idr -> "resource id" or "IDR resource id"

https://codereview.chromium.org/25536010/diff/14001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/apps/native_app_window_views.cc (right):

https://codereview.chromium.org/25536010/diff/14001/chrome/browser/ui/views/a...
chrome/browser/ui/views/apps/native_app_window_views.cc:200:
views::NativeWidgetAura* native_widget =
Why do you need to introduce a new native widget here?  Is this just creating
the native widget before window_->Init() does it below?  If so, that wasn't
clear to me.

https://codereview.chromium.org/25536010/diff/14001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/app_non_client_frame_view_ash.cc (left):

https://codereview.chromium.org/25536010/diff/14001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/app_non_client_frame_view_ash.cc:132:
frame->GetNativeView()->set_hit_test_bounds_override_inner(gfx::Insets());
On 2013/10/02 19:41:49, pkotwicz wrote:
> I removed this call because an empty gfx::Insets() is the default.

Are you sure this is safe? I seem to recall we did this because the window was
transitioning from restored (which had a hit test bounds override) to maximized
(where we don't want it any more).

https://codereview.chromium.org/25536010/diff/14001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/app_non_client_frame_view_ash.h (right):

https://codereview.chromium.org/25536010/diff/14001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/app_non_client_frame_view_ash.h:20: class
AppNonClientFrameViewAsh : public views::NonClientFrameView {
Dumb question - remind me why we need both AppNonClientFrameViewAsh and
AppBrowserFrameViewAsh ?

[pkotwicz, 2013-10-02 21:11:08]

https://codereview.chromium.org/25536010/diff/14001/ash/wm/caption_buttons/hi...
File ash/wm/caption_buttons/hideable_caption_button_container.cc (right):

https://codereview.chromium.org/25536010/diff/14001/ash/wm/caption_buttons/hi...
ash/wm/caption_buttons/hideable_caption_button_container.cc:100:
button_container_ = new FrameCaptionButtonContainerView(frame_,
Note to self: This is wrong. We need to select which value to pass in a way
similar to CustomFrameViewAsh.

[pkotwicz, 2013-10-03 20:45:34]

James, can you please take another look?

https://codereview.chromium.org/25536010/diff/14001/ash/wm/caption_buttons/hi...
File ash/wm/caption_buttons/hideable_caption_button_container.cc (right):

https://codereview.chromium.org/25536010/diff/14001/ash/wm/caption_buttons/hi...
ash/wm/caption_buttons/hideable_caption_button_container.cc:23: // The size of
the shadow around the caption buttons.
The sizes are derived from IDR_AURA_WINDOW_FULLSCREEN_SHADOW. I have added a
comment to this effect. (And yes, IDR_AURA_WINDOW_FULLSCREEN_SHADOW is still too
tall, but I expect we will want to get new assets because the current image is
also not wide enough for a minimize button)

https://codereview.chromium.org/25536010/diff/14001/ash/wm/caption_buttons/hi...
ash/wm/caption_buttons/hideable_caption_button_container.cc:44:
canvas->DrawImageInt(border_, 0, view.height() - border_.height());
This code deals with IDR_AURA_WINDOW_FULLSCREEN_SHADOW being too tall.

https://codereview.chromium.org/25536010/diff/14001/ash/wm/caption_buttons/hi...
ash/wm/caption_buttons/hideable_caption_button_container.cc:87:
HideableCaptionButtonContainer::HideableCaptionButtonContainer(
AppBrowserFrameViewAshTest is a sufficiently good test for this class for now. I
will migrate the tests and add plenty more tests once I make |button_widget_|
slide in and slide out.
(I expect that migrating AppBrowserFrameViewAshTest as part of this CL is wasted
work because I will have to rewrite the tests once |button_widget_| slides in
and out.)

https://codereview.chromium.org/25536010/diff/14001/ash/wm/caption_buttons/hi...
ash/wm/caption_buttons/hideable_caption_button_container.cc:100:
button_container_ = new FrameCaptionButtonContainerView(frame_,
The "Note to self" is wrong. We can assume that minimizing is allowed because
HideableCaptionButtonContainer is only used when |frame_| is maximized. This is
subtle, so I have added a comment.

https://codereview.chromium.org/25536010/diff/14001/ash/wm/caption_buttons/hi...
File ash/wm/caption_buttons/hideable_caption_button_container.h (right):

https://codereview.chromium.org/25536010/diff/14001/ash/wm/caption_buttons/hi...
ash/wm/caption_buttons/hideable_caption_button_container.h:23: //
TODO(pkotwicz): Auto hide the buttons when they are not in use and reveal
I think that we are sure that we want to hide the close button

https://codereview.chromium.org/25536010/diff/14001/ash/wm/caption_buttons/hi...
ash/wm/caption_buttons/hideable_caption_button_container.h:25: class ASH_EXPORT
HideableCaptionButtonContainer : public views::WidgetObserver {
HideableCaptionButtonContainer it is then.

https://codereview.chromium.org/25536010/diff/14001/ash/wm/caption_buttons/hi...
ash/wm/caption_buttons/hideable_caption_button_container.h:29: //
|background_id| is the idr of the image to use as the background.
On 2013/10/02 21:07:55, James Cook wrote:
> idr -> "resource id" or "IDR resource id"

Done.

https://codereview.chromium.org/25536010/diff/14001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/apps/native_app_window_views.cc (right):

https://codereview.chromium.org/25536010/diff/14001/chrome/browser/ui/views/a...
chrome/browser/ui/views/apps/native_app_window_views.cc:200:
views::NativeWidgetAura* native_widget =
I have added a better comment.
The reason is that NonClientFrameViewSwitcherAsh must set a WindowObserver on
|native_widget| before BaseLayoutManager does so. (And yes, BaseLayoutManager
adds a WindowObserver on |native_widget| as part of window_->Init()) To avoid an
extra layout of the web contents, NonClientFrameViewSwitcherAsh must be notified
of a maximized state change before BaseLayoutManager/WorkspaceLayoutManager

I have not managed to pull up numbers as to how much an extra layout would cost.
(There is no 1-1 mapping between content::RenderWidgetHostImpl::WasResized() and
WebCore::FrameView::layout())

https://codereview.chromium.org/25536010/diff/14001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/app_non_client_frame_view_ash.cc (left):

https://codereview.chromium.org/25536010/diff/14001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/app_non_client_frame_view_ash.cc:132:
frame->GetNativeView()->set_hit_test_bounds_override_inner(gfx::Insets());
You are completely right! Thanks for finding this bug.

I removed the comment because the comment is misleading. Returning HTNOWHERE
from AppNonClientFrameViewAsh::NonClientHitTest() is sufficient to make the
resize handles disappear. Clearing the inner insets makes all of the window
controls clickable. (Otherwise, you can't click the window controls in the top
right of the screen)

+Bonus, your comment made me find an unrelated bug. Will send you a CL later
today.

https://codereview.chromium.org/25536010/diff/14001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/app_non_client_frame_view_ash.h (right):

https://codereview.chromium.org/25536010/diff/14001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/app_non_client_frame_view_ash.h:20: class
AppNonClientFrameViewAsh : public views::NonClientFrameView {
Answer: Mostly to make me happy.

- I feel uneasy about using AppBrowserFrameViewAsh in the NativeAppWindowViews
case and passing a NULL BrowserView to AppBrowserFrameViewAsh.
- I cannot return a views::NonClientFrameView* from
chrome::CreateBrowserNonClientFrameView() because BrowserFrame relies on the
return value being a BrowserNonClientFrameView so that it can query things like
BrowserNonClientFrameView::GetTabStripInsets().

[James Cook, 2013-10-03 22:56:02]

LGTM

[pkotwicz, 2013-10-03 23:08:59]

benwells for chrome/browser/ui/views/apps

[benwells, 2013-10-04 04:42:20]

On 2013/10/03 23:08:59, pkotwicz wrote:
> benwells for chrome/browser/ui/views/apps

OK - I've just asked a few questions on the linked bug ...

[benwells, 2013-10-08 22:44:21]

lgtm.

sorry for the delays.

[pkotwicz, 2013-11-07 06:24:09]

I am closing this issue because we did a redesign. A CL for the new design (as
well as a link to the design doc is at
https://codereview.chromium.org/59043013/)