Enable showing the toolkit-views simplified fullscreen UI on Mac.

Enable showing the toolkit-views simplified fullscreen UI on Mac.

Puts the new simplified fullscreen UI behind
chrome://flags/#simplified-fullscreen-ui on Mac.

It uses the toolkit-views UI, since it's already written and works
pretty well. Only few things need to be farmed out to the delegate
since the bubble can't assume its parent is a views::Widget.

Fixes a quirk where opacity-animating bubbles would briefly change
their bounds to have zero-height and back during construction. (We
complain about visible but zero-sized Widgets on Mac).

If this all works we can delete some ~700 lines of Cocoa code when
this simplified fullscreen UI is default on Mac.

BUG=352425
Committed: https://crrev.com/eb0962214e28fcbe6349bae6214d020cfe67f7cc
Cr-Commit-Position: refs/heads/master@{#373386}

[tapted, 2016-02-03 02:05:31]

Patchset #3 (id:40001) has been deleted

[tapted, 2016-02-03 03:00:57]

Patchset #1 (id:1) has been deleted

[tapted, 2016-02-03 03:01:05]

Patchset #1 (id:20001) has been deleted

[tapted, 2016-02-03 03:01:12]

Patchset #1 (id:60001) has been deleted

[tapted, 2016-02-03 05:57:28]

Description was changed from

==========
Enable showing the toolkit-views simplified fullscreen UI on Mac.

BUG=352425
==========

to

==========
Enable showing the toolkit-views simplified fullscreen UI on Mac.

Puts the new simplified fullscreen UI behind
chrome://flags/#simplified-fullscreen-ui on Mac.

It uses the toolkit-views UI, since it's already written and works
pretty well. Only few things need to be farmed out to the delegate
since the bubble can't assume its parent is a views::Widget.

Fixes a quirk where opacity-animating bubbles would briefly change
their bounds to have zero-height and back during construction. (We complain
about visible but zero-sized Widgets on Mac).

If this all works we can delete some ~700 lines of Cocoa code when
this simplified fullscreen UI is default on Mac.

BUG=352425
==========

[tapted, 2016-02-03 05:57:28]

tapted@chromium.org changed reviewers:
+ msw@chromium.org, spqchan@chromium.org

[tapted, 2016-02-03 05:58:17]

Description was changed from

==========
Enable showing the toolkit-views simplified fullscreen UI on Mac.

Puts the new simplified fullscreen UI behind
chrome://flags/#simplified-fullscreen-ui on Mac.

It uses the toolkit-views UI, since it's already written and works
pretty well. Only few things need to be farmed out to the delegate
since the bubble can't assume its parent is a views::Widget.

Fixes a quirk where opacity-animating bubbles would briefly change
their bounds to have zero-height and back during construction. (We complain
about visible but zero-sized Widgets on Mac).

If this all works we can delete some ~700 lines of Cocoa code when
this simplified fullscreen UI is default on Mac.

BUG=352425
==========

to

==========
Enable showing the toolkit-views simplified fullscreen UI on Mac.

Puts the new simplified fullscreen UI behind
chrome://flags/#simplified-fullscreen-ui on Mac.

It uses the toolkit-views UI, since it's already written and works
pretty well. Only few things need to be farmed out to the delegate
since the bubble can't assume its parent is a views::Widget.

Fixes a quirk where opacity-animating bubbles would briefly change
their bounds to have zero-height and back during construction. (We
complain about visible but zero-sized Widgets on Mac).

If this all works we can delete some ~700 lines of Cocoa code when
this simplified fullscreen UI is default on Mac.

BUG=352425
==========

[tapted, 2016-02-03 06:20:53]

spqchan: please review the cocoa stuff
msw: please review the views stuff

cc mgiuca fyi

https://codereview.chromium.org/1654723002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right):

https://codereview.chromium.org/1654723002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/exclusive_access_bubble_views.cc:392:
animated_attribute_(ExpectedAnimationAttribute()) {
this used to happen in a call to UpdateForImmersiveState() at the end of the
constructor. See next comment why that's bad.

https://codereview.chromium.org/1654723002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/exclusive_access_bubble_views.cc:424:
popup_->SetBounds(GetPopupRect(false));
prior to the change to the animated_attribute_ initializer, this line would set
the size of the popup to have a zero height. (because GetPopupRect(..) thought
it was animating the bounds, not the opacity). We are overly cautious about
showing zero-sized windows on Mac for historical reasons (window server bugs) -
so this hit a DCHECK.

The bounds for opacity-animations would previously be fixed up by 
UpdateForImmersiveState() calling UpdateBounds() at the end of the constructor,
but now there's no need for that.

[spqchan, 2016-02-03 17:31:53]

Cocoa code LGTM

[msw, 2016-02-03 18:52:24]

https://codereview.chromium.org/1654723002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right):

https://codereview.chromium.org/1654723002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/exclusive_access_bubble_views.cc:392:
animated_attribute_(ExpectedAnimationAttribute()) {
On 2016/02/03 06:20:53, tapted wrote:
> this used to happen in a call to UpdateForImmersiveState() at the end of the
> constructor. See next comment why that's bad.

Acknowledged.

https://codereview.chromium.org/1654723002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/exclusive_access_bubble_views.cc:424:
popup_->SetBounds(GetPopupRect(false));
On 2016/02/03 06:20:53, tapted wrote:
> prior to the change to the animated_attribute_ initializer, this line would
set
> the size of the popup to have a zero height. (because GetPopupRect(..) thought
> it was animating the bounds, not the opacity). We are overly cautious about
> showing zero-sized windows on Mac for historical reasons (window server bugs)
-
> so this hit a DCHECK.
> 
> The bounds for opacity-animations would previously be fixed up by 
> UpdateForImmersiveState() calling UpdateBounds() at the end of the
constructor,
> but now there's no need for that.

Acknowledged.

https://codereview.chromium.org/1654723002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/exclusive_access_bubble_views_context.h (right):

https://codereview.chromium.org/1654723002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/exclusive_access_bubble_views_context.h:18: class
ExclusiveAccessBubbleViewsContext : public ui::AcceleratorProvider {
Eh, I'm on the fence about this; should this interface instead have a
GetAcceleratorProvider, which BrowserView and ExclusiveAccessController then
override to return |this|, and separately subclass ui::AcceleratorProvider? It
seems like the accelerator functionality of BrowserView isn't tightly coupled to
its exclusive access context role.

https://codereview.chromium.org/1654723002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/exclusive_access_bubble_views_context.h:29: virtual
gfx::NativeView GetParentView() const = 0;
nit: maybe |GetBubbleParentView|?

[tapted, 2016-02-03 21:57:53]

Patchset #3 (id:120001) has been deleted

[tapted, 2016-02-03 21:58:25]

https://codereview.chromium.org/1654723002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/exclusive_access_bubble_views_context.h (right):

https://codereview.chromium.org/1654723002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/exclusive_access_bubble_views_context.h:18: class
ExclusiveAccessBubbleViewsContext : public ui::AcceleratorProvider {
On 2016/02/03 18:52:24, msw wrote:
> Eh, I'm on the fence about this; should this interface instead have a
> GetAcceleratorProvider, which BrowserView and ExclusiveAccessController then
> override to return |this|, and separately subclass ui::AcceleratorProvider? It
> seems like the accelerator functionality of BrowserView isn't tightly coupled
to
> its exclusive access context role.

Ah, that's nicer. I started with something like
GetAcceleratorForExclusiveAccessForId... or something like that and it felt
wrong (GetAccelerator was taken). Then I considered virtual inheritance.. which
is the "right" tool in some ways, but has performance implications. Then what I
had. But your suggestion is nice. Done.

https://codereview.chromium.org/1654723002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/exclusive_access_bubble_views_context.h:29: virtual
gfx::NativeView GetParentView() const = 0;
On 2016/02/03 18:52:24, msw wrote:
> nit: maybe |GetBubbleParentView|?

Done.

[msw, 2016-02-03 22:11:56]

lgtm

https://codereview.chromium.org/1654723002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1654723002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:2667: // BrowserView,
ExclusiveAccessContext overrides
optional nit: "overrides:" (or "implementation:"); blank line below.

https://codereview.chromium.org/1654723002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:2688: // BrowserView,
ExclusiveAccessBubbleViewsContext overrides
optional nit: "overrides:" (or "implementation:"); blank line below.

[tapted, 2016-02-03 22:27:53]

https://codereview.chromium.org/1654723002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1654723002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:2667: // BrowserView,
ExclusiveAccessContext overrides
On 2016/02/03 22:11:56, msw wrote:
> optional nit: "overrides:" (or "implementation:"); blank line below.

Done. (went with `implementation:` - I think it's more common, and seems like a
good fit for pure virtuals)

https://codereview.chromium.org/1654723002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:2688: // BrowserView,
ExclusiveAccessBubbleViewsContext overrides
On 2016/02/03 22:11:56, msw wrote:
> optional nit: "overrides:" (or "implementation:"); blank line below.

Done.

[tapted, 2016-02-03 22:48:39]

The CQ bit was checked by tapted@chromium.org

[tapted, 2016-02-03 22:48:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
spqchan@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1654723002/#ps160001
(title: "Update section comments")

[commit-bot: I haz the power, 2016-02-03 22:50:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1654723002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1654723002/160001

[commit-bot: I haz the power, 2016-02-03 23:45:41]

Description was changed from

==========
Enable showing the toolkit-views simplified fullscreen UI on Mac.

Puts the new simplified fullscreen UI behind
chrome://flags/#simplified-fullscreen-ui on Mac.

It uses the toolkit-views UI, since it's already written and works
pretty well. Only few things need to be farmed out to the delegate
since the bubble can't assume its parent is a views::Widget.

Fixes a quirk where opacity-animating bubbles would briefly change
their bounds to have zero-height and back during construction. (We
complain about visible but zero-sized Widgets on Mac).

If this all works we can delete some ~700 lines of Cocoa code when
this simplified fullscreen UI is default on Mac.

BUG=352425
==========

to

==========
Enable showing the toolkit-views simplified fullscreen UI on Mac.

Puts the new simplified fullscreen UI behind
chrome://flags/#simplified-fullscreen-ui on Mac.

It uses the toolkit-views UI, since it's already written and works
pretty well. Only few things need to be farmed out to the delegate
since the bubble can't assume its parent is a views::Widget.

Fixes a quirk where opacity-animating bubbles would briefly change
their bounds to have zero-height and back during construction. (We
complain about visible but zero-sized Widgets on Mac).

If this all works we can delete some ~700 lines of Cocoa code when
this simplified fullscreen UI is default on Mac.

BUG=352425
==========

[commit-bot: I haz the power, 2016-02-03 23:45:41]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2016-02-03 23:46:58]

Description was changed from

==========
Enable showing the toolkit-views simplified fullscreen UI on Mac.

Puts the new simplified fullscreen UI behind
chrome://flags/#simplified-fullscreen-ui on Mac.

It uses the toolkit-views UI, since it's already written and works
pretty well. Only few things need to be farmed out to the delegate
since the bubble can't assume its parent is a views::Widget.

Fixes a quirk where opacity-animating bubbles would briefly change
their bounds to have zero-height and back during construction. (We
complain about visible but zero-sized Widgets on Mac).

If this all works we can delete some ~700 lines of Cocoa code when
this simplified fullscreen UI is default on Mac.

BUG=352425
==========

to

==========
Enable showing the toolkit-views simplified fullscreen UI on Mac.

Puts the new simplified fullscreen UI behind
chrome://flags/#simplified-fullscreen-ui on Mac.

It uses the toolkit-views UI, since it's already written and works
pretty well. Only few things need to be farmed out to the delegate
since the bubble can't assume its parent is a views::Widget.

Fixes a quirk where opacity-animating bubbles would briefly change
their bounds to have zero-height and back during construction. (We
complain about visible but zero-sized Widgets on Mac).

If this all works we can delete some ~700 lines of Cocoa code when
this simplified fullscreen UI is default on Mac.

BUG=352425

Committed: https://crrev.com/eb0962214e28fcbe6349bae6214d020cfe67f7cc
Cr-Commit-Position: refs/heads/master@{#373386}
==========

[commit-bot: I haz the power, 2016-02-03 23:46:59]

Patchset 4 (id:??) landed as
https://crrev.com/eb0962214e28fcbe6349bae6214d020cfe67f7cc
Cr-Commit-Position: refs/heads/master@{#373386}