Refactor: Untangle Mac's ExclusiveAccessContext from BrowserWindow

Refactor: Untangle Mac's ExclusiveAccessContext from BrowserWindow

This is a pure refactor CL. One problem: BrowserWindowCocoa (and views'
BrowserView) multiply-inherit from BrowserWindow and
ExclusiveAccessContext. BW and EAC declare some of the same methods, so
it gets weird. Let's fix that.

Then, toolkit-views has the "simplified fullscreen UI" implemented. It
works fine on Mac, but BrowserWindowCocoa has lots of dependencies and
we don't want to add more.

Instead, start by decoupling Mac's ExclusiveAccessContext implementation
from BrowserWindowCocoa into its own class, ExclusiveAccessController.
This wraps and manages the ephemeral fullscreen bubble window which, for
Cocoa, is a ExclusiveAccessBubbleWindowController.

A follow-up CL will allow ExclusiveAccessController to pick between a
Cocoa bubble and a toolkit-views bubble at runtime.

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

[tapted, 2016-02-01 04:02:47]

Patchset #7 (id:120001) has been deleted

[tapted, 2016-02-01 04:03:00]

Patchset #6 (id:100001) has been deleted

[tapted, 2016-02-01 04:03:15]

Patchset #1 (id:1) has been deleted

[tapted, 2016-02-01 04:29:53]

Description was changed from

==========
Support the views-based simplified fullscreen UI behind a flag

BUG=
==========

to

==========
Refactor: Untangle Mac's ExclusiveAccessContext from BrowserWindow

This is a pure refactor CL. One problem: BrowserWindowCocoa (and views'
BrowserView) multiply-inherit from BrowserWindow and
ExclusiveAccessContext. BW and EAC declare some of the same methods, so
it gets weird. Let's fix that.

Then, toolkit-views has the "simplified fullscreen UI" implemented. It
works fine on Mac, but BrowserWindowCocoa has lots of dependencies and
we don't want to add more.

Instead, start by decoupling Mac's ExclusiveAccessContext implementation
from BrowserWindowCocoa into its own class, ExclusiveAccessController.
This wraps and manages the ephemeral fullscreen bubble window which, for
Cocoa, is a ExclusiveAccessBubbleWindowController.

A follow-up CL will allow ExclusiveAccessController to pick between a
Cocoa bubble and a toolkit-views bubble at runtime.

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

[tapted, 2016-02-01 06:04:52]

Patchset #1 (id:20001) has been deleted

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

Patchset #1 (id:40001) has been deleted

[tapted, 2016-02-01 06:05:35]

Patchset #3 (id:140001) has been deleted

[tapted, 2016-02-01 06:05:48]

Patchset #3 (id:160001) has been deleted

[tapted, 2016-02-01 06:06:15]

Patchset #3 (id:180001) has been deleted

[tapted, 2016-02-01 07:19:20]

Patchset #3 (id:200001) has been deleted

[tapted, 2016-02-01 07:23:04]

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

[tapted, 2016-02-01 07:25:08]

Hi Sarah, could you please review before I send off to OWNERS. (The follow-up
that enables the views UI behind chrome://flags/#simplified-fullscreen-ui is in
http://crrev.com/1654723002 -- it's pretty neat!)

[spqchan, 2016-02-01 19:55:05]

It's great to see that this is getting refactored! The code has been getting
needlessly bulky

https://codereview.chromium.org/1650483002/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.h
(right):

https://codereview.chromium.org/1650483002/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.h:48: bool
IsFullscreen() const override;
Can you also move the comments that describe these methods?
There should be existing comments in the files you moved them from.

https://codereview.chromium.org/1650483002/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/1650483002/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:209: [controller_
exitAnyFullscreen];
It's probably safer to just use [controller_
exclusiveAccessContext]->ExitFullscreen(), in case the implementation ends up
changing.

https://codereview.chromium.org/1650483002/diff/220001/chrome/browser/ui/excl...
File chrome/browser/ui/exclusive_access/fullscreen_controller_state_test.cc
(right):

https://codereview.chromium.org/1650483002/diff/220001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller_state_test.cc:749:
EXPECT_EQ(context->IsFullscreenWithToolbar(), !!fullscreen_with_toolbar)
Why not just fullscreen_with_toolbar?

[tapted, 2016-02-02 11:57:06]

https://codereview.chromium.org/1650483002/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.h
(right):

https://codereview.chromium.org/1650483002/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.h:48: bool
IsFullscreen() const override;
On 2016/02/01 19:55:04, spqchan wrote:
> Can you also move the comments that describe these methods?
> There should be existing comments in the files you moved them from.

There weren't any on BrowserWindowController - I think for overrides we usually
just rely on the comments in the superclass so they don't get out of sync.
ExclusiveAccessContext is reasonably well documented.

https://codereview.chromium.org/1650483002/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/1650483002/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:209: [controller_
exitAnyFullscreen];
On 2016/02/01 19:55:04, spqchan wrote:
> It's probably safer to just use [controller_
> exclusiveAccessContext]->ExitFullscreen(), in case the implementation ends up
> changing.

Yah, good idea. Done.

https://codereview.chromium.org/1650483002/diff/220001/chrome/browser/ui/excl...
File chrome/browser/ui/exclusive_access/fullscreen_controller_state_test.cc
(right):

https://codereview.chromium.org/1650483002/diff/220001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller_state_test.cc:749:
EXPECT_EQ(context->IsFullscreenWithToolbar(), !!fullscreen_with_toolbar)
On 2016/02/01 19:55:04, spqchan wrote:
> Why not just fullscreen_with_toolbar?

So this was interesting. FullscreenForBrowserExpectation is an enum with values
FULLSCREEN_FOR_BROWSER_{FALSE, TRUE, NO_EXPECTATION}. `!!` converts
NO_EXPECTATION to ~TRUE (or true), but there doesn't seem to be a need for that
with the `if` condition just above. So I tried that -- it worked fine locally,
but MSVC kicked up a stink on Windows about matching a template with an enum on
one side and a bool on the other.

something like

DCHECK_EQ(IsFullscreen(), fullscreen_with_toolbar ==
FULLSCREEN_FOR_BROWSER_TRUE)

would fix that, but it would make it inconsistent with the surrounding code, so
I've left the `!!` in.

[tapted, 2016-02-02 21:58:43]

https://codereview.chromium.org/1650483002/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/1650483002/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:209: [controller_
exitAnyFullscreen];
On 2016/02/02 11:57:06, tapted wrote:
> On 2016/02/01 19:55:04, spqchan wrote:
> > It's probably safer to just use [controller_
> > exclusiveAccessContext]->ExitFullscreen(), in case the implementation ends
up
> > changing.
> 
> Yah, good idea. Done.

(note with this change I also needed to move the initialization of the
ExclusiveAccessController to before the initial SetBounds call in the
BrowserWindowController initializer, so that's now in patchset 6)

[spqchan, 2016-02-02 22:10:05]

LGTM

https://codereview.chromium.org/1650483002/diff/220001/chrome/browser/ui/excl...
File chrome/browser/ui/exclusive_access/fullscreen_controller_state_test.cc
(right):

https://codereview.chromium.org/1650483002/diff/220001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller_state_test.cc:749:
EXPECT_EQ(context->IsFullscreenWithToolbar(), !!fullscreen_with_toolbar)
On 2016/02/02 11:57:06, tapted wrote:
> On 2016/02/01 19:55:04, spqchan wrote:
> > Why not just fullscreen_with_toolbar?
> 
> So this was interesting. FullscreenForBrowserExpectation is an enum with
values
> FULLSCREEN_FOR_BROWSER_{FALSE, TRUE, NO_EXPECTATION}. `!!` converts
> NO_EXPECTATION to ~TRUE (or true), but there doesn't seem to be a need for
that
> with the `if` condition just above. So I tried that -- it worked fine locally,
> but MSVC kicked up a stink on Windows about matching a template with an enum
on
> one side and a bool on the other.
> 
> something like
> 
> DCHECK_EQ(IsFullscreen(), fullscreen_with_toolbar ==
> FULLSCREEN_FOR_BROWSER_TRUE)
> 
> would fix that, but it would make it inconsistent with the surrounding code,
so
> I've left the `!!` in.

Ah, that makes a lot of sense.

[tapted, 2016-02-02 22:16:58]

tapted@chromium.org changed reviewers:
+ scheib@chromium.org, sky@chromium.org

[tapted, 2016-02-02 22:16:59]

+scheib,sky for OWNERS

scheib for chrome/browser/ui/exclusive_access/*
sky for c/b/ui/views, c/b/test, and browser_window.h [just deleting stuff :)]

(this cleans up some unnecessary/multiply-inherited methods on BrowserWindow
that ExclusiveAccessContext was also declaring)

[scheib, 2016-02-02 23:14:41]

LGTM with comment fix and I think avoiding the need for a static_cast (see
comments).

I think it wise to check in with mgiuca who has had recent code in flight in
this area.

https://codereview.chromium.org/1650483002/diff/280001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.h
(right):

https://codereview.chromium.org/1650483002/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.h:24: // with
the exclusive access bubble, which appears when entering fullscreen.
which appears for events such as entering fullscreen.
              ^^^^^^^^^^^^^^^^^^^

https://codereview.chromium.org/1650483002/diff/280001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm
(right):

https://codereview.chromium.org/1650483002/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm:69: // See
browser_window_controller.h for a detailed explanation of the logic in
Be more specific -- the "Fullscreen terminology" section?

https://codereview.chromium.org/1650483002/diff/280001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller_unittest.mm
(right):

https://codereview.chromium.org/1650483002/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller_unittest.mm:114:
static_cast<ExclusiveAccessController*>([bwc exclusiveAccessContext]);
Oh no, static_cast!
Please avoid if possible, which I think can be done by having
exclusiveAccessContext (defined in cocoa/browser_window_controller.h) return the
cocoa specific type ExclusiveAccessController* from the already specifically
typed member exclusiveAccessController_.

[tapted, 2016-02-02 23:55:23]

On 2016/02/02 23:14:41, scheib wrote:
> LGTM with comment fix and I think avoiding the need for a static_cast (see
> comments).
> 
> I think it wise to check in with mgiuca who has had recent code in flight in
> this area.

Yep, it's mgiuca's work that prompted this :). He's a few desks away from me -
we're going to try running on Mac the views UI he did, since it all Just Works
(tm), and would let us delete some ~600 lines of Cocoa code (770 if you include
tests) which is tricky to maintain, once this becomes default (gist:
http://crrev.com/1579813002/)

https://codereview.chromium.org/1650483002/diff/280001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.h
(right):

https://codereview.chromium.org/1650483002/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.h:24: // with
the exclusive access bubble, which appears when entering fullscreen.
On 2016/02/02 23:14:41, scheib wrote:
> which appears for events such as entering fullscreen.
>               ^^^^^^^^^^^^^^^^^^^

Done.

https://codereview.chromium.org/1650483002/diff/280001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm
(right):

https://codereview.chromium.org/1650483002/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm:69: // See
browser_window_controller.h for a detailed explanation of the logic in
On 2016/02/02 23:14:41, scheib wrote:
> Be more specific -- the "Fullscreen terminology" section?

Done.

https://codereview.chromium.org/1650483002/diff/280001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller_unittest.mm
(right):

https://codereview.chromium.org/1650483002/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller_unittest.mm:114:
static_cast<ExclusiveAccessController*>([bwc exclusiveAccessContext]);
On 2016/02/02 23:14:41, scheib wrote:
> Oh no, static_cast!
> Please avoid if possible, which I think can be done by having
> exclusiveAccessContext (defined in cocoa/browser_window_controller.h) return
the
> cocoa specific type ExclusiveAccessController* from the already specifically
> typed member exclusiveAccessController_.

Done (this means browser_window_cocoa.mm needs an extra #import so it knows how
to upcast, but I guess that's not so bad).

[scheib, 2016-02-03 00:12:16]

Thanks

https://codereview.chromium.org/1650483002/diff/300001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/1650483002/diff/300001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.h:590: -
(ExclusiveAccessController*)exclusiveAccessContext;
Accessor name should probably follow type name change.

[tapted, 2016-02-03 00:19:45]

https://codereview.chromium.org/1650483002/diff/300001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/1650483002/diff/300001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.h:590: -
(ExclusiveAccessController*)exclusiveAccessContext;
On 2016/02/03 00:12:16, scheib wrote:
> Accessor name should probably follow type name change. 

Done.

[sky, 2016-02-03 00:42:35]

LGTM

[tapted, 2016-02-03 01:37:45]

The CQ bit was checked by tapted@chromium.org

[tapted, 2016-02-03 01:37:46]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
spqchan@chromium.org
Link to the patchset: https://codereview.chromium.org/1650483002/#ps320001
(title: "rename accessor")

[commit-bot: I haz the power, 2016-02-03 01:39:00]

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

[commit-bot: I haz the power, 2016-02-03 02:24:44]

Description was changed from

==========
Refactor: Untangle Mac's ExclusiveAccessContext from BrowserWindow

This is a pure refactor CL. One problem: BrowserWindowCocoa (and views'
BrowserView) multiply-inherit from BrowserWindow and
ExclusiveAccessContext. BW and EAC declare some of the same methods, so
it gets weird. Let's fix that.

Then, toolkit-views has the "simplified fullscreen UI" implemented. It
works fine on Mac, but BrowserWindowCocoa has lots of dependencies and
we don't want to add more.

Instead, start by decoupling Mac's ExclusiveAccessContext implementation
from BrowserWindowCocoa into its own class, ExclusiveAccessController.
This wraps and manages the ephemeral fullscreen bubble window which, for
Cocoa, is a ExclusiveAccessBubbleWindowController.

A follow-up CL will allow ExclusiveAccessController to pick between a
Cocoa bubble and a toolkit-views bubble at runtime.

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

to

==========
Refactor: Untangle Mac's ExclusiveAccessContext from BrowserWindow

This is a pure refactor CL. One problem: BrowserWindowCocoa (and views'
BrowserView) multiply-inherit from BrowserWindow and
ExclusiveAccessContext. BW and EAC declare some of the same methods, so
it gets weird. Let's fix that.

Then, toolkit-views has the "simplified fullscreen UI" implemented. It
works fine on Mac, but BrowserWindowCocoa has lots of dependencies and
we don't want to add more.

Instead, start by decoupling Mac's ExclusiveAccessContext implementation
from BrowserWindowCocoa into its own class, ExclusiveAccessController.
This wraps and manages the ephemeral fullscreen bubble window which, for
Cocoa, is a ExclusiveAccessBubbleWindowController.

A follow-up CL will allow ExclusiveAccessController to pick between a
Cocoa bubble and a toolkit-views bubble at runtime.

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

[commit-bot: I haz the power, 2016-02-03 02:24:44]

Committed patchset #8 (id:320001)

[commit-bot: I haz the power, 2016-02-03 02:25:26]

Description was changed from

==========
Refactor: Untangle Mac's ExclusiveAccessContext from BrowserWindow

This is a pure refactor CL. One problem: BrowserWindowCocoa (and views'
BrowserView) multiply-inherit from BrowserWindow and
ExclusiveAccessContext. BW and EAC declare some of the same methods, so
it gets weird. Let's fix that.

Then, toolkit-views has the "simplified fullscreen UI" implemented. It
works fine on Mac, but BrowserWindowCocoa has lots of dependencies and
we don't want to add more.

Instead, start by decoupling Mac's ExclusiveAccessContext implementation
from BrowserWindowCocoa into its own class, ExclusiveAccessController.
This wraps and manages the ephemeral fullscreen bubble window which, for
Cocoa, is a ExclusiveAccessBubbleWindowController.

A follow-up CL will allow ExclusiveAccessController to pick between a
Cocoa bubble and a toolkit-views bubble at runtime.

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

to

==========
Refactor: Untangle Mac's ExclusiveAccessContext from BrowserWindow

This is a pure refactor CL. One problem: BrowserWindowCocoa (and views'
BrowserView) multiply-inherit from BrowserWindow and
ExclusiveAccessContext. BW and EAC declare some of the same methods, so
it gets weird. Let's fix that.

Then, toolkit-views has the "simplified fullscreen UI" implemented. It
works fine on Mac, but BrowserWindowCocoa has lots of dependencies and
we don't want to add more.

Instead, start by decoupling Mac's ExclusiveAccessContext implementation
from BrowserWindowCocoa into its own class, ExclusiveAccessController.
This wraps and manages the ephemeral fullscreen bubble window which, for
Cocoa, is a ExclusiveAccessBubbleWindowController.

A follow-up CL will allow ExclusiveAccessController to pick between a
Cocoa bubble and a toolkit-views bubble at runtime.

BUG=352425

Committed: https://crrev.com/8d830c628767a7d2327a006f1d033b1d03cd070c
Cr-Commit-Position: refs/heads/master@{#373125}
==========

[commit-bot: I haz the power, 2016-02-03 02:25:27]

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