[Mac] Move the fullscreen toolbar style to FullscreenToolbarController

[Mac] Move the fullscreen toolbar style code into FullscreenToolbarController

- No intended behavioral changes.
- Renamed "FullscreenSlidingStyle" to "FullscreenToolbarStyle" and moved the logic
 into FullscreenToolbarController

BUG=643418
Committed: https://crrev.com/3c5b671d18c00040c5313257a619a197d4a7daaa
Cr-Commit-Position: refs/heads/master@{#429979}

[spqchan, 2016-11-01 21:36:17]

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 21:37:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[spqchan, 2016-11-01 22:00:18]

Description was changed from

==========
[Mac] Move the fullscreen toolbar style to FullscreenToolbarController

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

to

==========
[Mac] Move the fullscreen toolbar style code into FullscreenToolbarController

- Renamed "FullscreenSlidingStyle" to "FullscreenToolbarStyle"
- Moved the logic so that FullscreenToolbarController is handling it 

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

[commit-bot: I haz the power, 2016-11-01 22:43:00]

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

[commit-bot: I haz the power, 2016-11-01 22:43:01]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[spqchan, 2016-11-01 23:06:41]

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 23:07:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[spqchan, 2016-11-01 23:10:14]

Description was changed from

==========
[Mac] Move the fullscreen toolbar style code into FullscreenToolbarController

- Renamed "FullscreenSlidingStyle" to "FullscreenToolbarStyle"
- Moved the logic so that FullscreenToolbarController is handling it 

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

to

==========
[Mac] Move the fullscreen toolbar style code into FullscreenToolbarController

- No intended behavioral changes.
- Renamed "FullscreenSlidingStyle" to "FullscreenToolbarStyle"
- Moved the logic so that FullscreenToolbarController is handling it 

BUG=643418
==========

[spqchan, 2016-11-01 23:10:14]

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

[spqchan, 2016-11-01 23:23:01]

Description was changed from

==========
[Mac] Move the fullscreen toolbar style code into FullscreenToolbarController

- No intended behavioral changes.
- Renamed "FullscreenSlidingStyle" to "FullscreenToolbarStyle"
- Moved the logic so that FullscreenToolbarController is handling it 

BUG=643418
==========

to

==========
[Mac] Move the fullscreen toolbar style code into FullscreenToolbarController

- No intended behavioral changes.
- Renamed "FullscreenSlidingStyle" to "FullscreenToolbarStyle"
- Moved the logic into FullscreenToolbarController 

BUG=643418
==========

[commit-bot: I haz the power, 2016-11-01 23:51:27]

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

[commit-bot: I haz the power, 2016-11-01 23:51:28]

Dry run: This issue passed the CQ dry run.

[spqchan, 2016-11-01 23:58:53]

PTAL

[erikchen, 2016-11-02 00:25:37]

https://codereview.chromium.org/2467833003/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/2467833003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:782: if (![self
isInAnyFullscreenMode])
Should this be a DCHECK? How can this get called if we're not in or entering
fullscreen?

https://codereview.chromium.org/2467833003/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (left):

https://codereview.chromium.org/2467833003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:309: browserController_
= nil;
why did you get rid of this? It seems important.

https://codereview.chromium.org/2467833003/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):

https://codereview.chromium.org/2467833003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:115: if
(!inFullscreenMode_)
why the move away from DCHECK?

[spqchan, 2016-11-02 01:37:07]

Description was changed from

==========
[Mac] Move the fullscreen toolbar style code into FullscreenToolbarController

- No intended behavioral changes.
- Renamed "FullscreenSlidingStyle" to "FullscreenToolbarStyle"
- Moved the logic into FullscreenToolbarController 

BUG=643418
==========

to

==========
[Mac] Move the fullscreen toolbar style code into FullscreenToolbarController

- No intended behavioral changes.
- Renamed "FullscreenSlidingStyle" to "FullscreenToolbarStyle" and moved the
logic
 into FullscreenToolbarController 

- Changed how FullscreenToolbarController is set up in the BWC. Before a new one
is created when the browser window enters fullscreen and is destroyed when the
window exits fullscreen.
- Now the controller is cre

BUG=643418
==========

[spqchan, 2016-11-02 17:26:47]

Description was changed from

==========
[Mac] Move the fullscreen toolbar style code into FullscreenToolbarController

- No intended behavioral changes.
- Renamed "FullscreenSlidingStyle" to "FullscreenToolbarStyle" and moved the
logic
 into FullscreenToolbarController 

- Changed how FullscreenToolbarController is set up in the BWC. Before a new one
is created when the browser window enters fullscreen and is destroyed when the
window exits fullscreen.
- Now the controller is cre

BUG=643418
==========

to

==========
[Mac] Move the fullscreen toolbar style code into FullscreenToolbarController

- No intended behavioral changes.

- Renamed "FullscreenSlidingStyle" to "FullscreenToolbarStyle" and moved the
logic
 into FullscreenToolbarController 

- Changed how FullscreenToolbarController is set up in the BWC. 
- Previously, a new one is created when the browser window enters fullscreen.
It's
 destroyed when the window exits fullscreen.
- Now the BWC creates the FullscreenToolbarController in the initializer and
destroys
 it when it deallocs.

BUG=643418
==========

[spqchan, 2016-11-02 18:57:21]

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 18:58:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 19:42:57]

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

[commit-bot: I haz the power, 2016-11-02 19:42:57]

Dry run: This issue passed the CQ dry run.

[spqchan, 2016-11-02 20:00:28]

PTAL

https://codereview.chromium.org/2467833003/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/2467833003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:782: if (![self
isInAnyFullscreenMode])
On 2016/11/02 00:25:36, erikchen wrote:
> Should this be a DCHECK? How can this get called if we're not in or entering
> fullscreen?

That's true, this was pulled from |adjustUIForSlidingFullscreenStyle:|. Now that
the style logic is moved to FullscreenToolbarController, this is no longer
necessary.

https://codereview.chromium.org/2467833003/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (left):

https://codereview.chromium.org/2467833003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:309: browserController_
= nil;
On 2016/11/02 00:25:36, erikchen wrote:
> why did you get rid of this? It seems important.

I didn't get rid of it, I moved it into dealloc. Sorry, I should've mentioned
this earlier. I updated the CL description about these changes. Let me know if
you need any clarifications!

https://codereview.chromium.org/2467833003/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):

https://codereview.chromium.org/2467833003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:115: if
(!inFullscreenMode_)
On 2016/11/02 00:25:36, erikchen wrote:
> why the move away from DCHECK?

Whoops, this shouldn't be here. Thanks for catching it

[erikchen, 2016-11-02 20:07:09]

What's the benefit of keeping around the FullscreenToolbarController when the
window is not fullscreen? A possible downside is that it can carry over
incorrect state.

[spqchan, 2016-11-02 20:20:39]

On 2016/11/02 20:07:09, erikchen wrote:
> What's the benefit of keeping around the FullscreenToolbarController when the
> window is not fullscreen? A possible downside is that it can carry over
> incorrect state.

I'm trying to remember the exact reason why I did that before I split up the
code, I believe it had something to do with the tests or the layout.
I can't think of a good benefit now, and considering the downside it's better to
just stick with having it around for fullscreen. I'll update the CL and let you
know when it's ready. Thanks!

[spqchan, 2016-11-02 20:33:58]

Description was changed from

==========
[Mac] Move the fullscreen toolbar style code into FullscreenToolbarController

- No intended behavioral changes.

- Renamed "FullscreenSlidingStyle" to "FullscreenToolbarStyle" and moved the
logic
 into FullscreenToolbarController 

- Changed how FullscreenToolbarController is set up in the BWC. 
- Previously, a new one is created when the browser window enters fullscreen.
It's
 destroyed when the window exits fullscreen.
- Now the BWC creates the FullscreenToolbarController in the initializer and
destroys
 it when it deallocs.

BUG=643418
==========

to

==========
[Mac] Move the fullscreen toolbar style code into FullscreenToolbarController

- No intended behavioral changes.
- Renamed "FullscreenSlidingStyle" to "FullscreenToolbarStyle" and moved the
logic
 into FullscreenToolbarController 

BUG=643418
==========

[spqchan, 2016-11-02 20:41:05]

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 20:41:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 21:54:11]

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

[commit-bot: I haz the power, 2016-11-02 21:54:11]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[spqchan, 2016-11-03 00:30:39]

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 00:31:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 01:23:01]

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

[commit-bot: I haz the power, 2016-11-03 01:23:02]

Dry run: This issue passed the CQ dry run.

[spqchan, 2016-11-03 02:41:17]

PTAL

[spqchan, 2016-11-03 02:41:21]

PTAL

[erikchen, 2016-11-03 17:02:17]

lgtm

[spqchan, 2016-11-03 17:06:15]

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

[spqchan, 2016-11-03 17:06:16]

thanks!

+rsesek for ownership

[Robert Sesek, 2016-11-04 16:31:57]

LGTM w/ two doc nits. Really nice cleanup!

https://codereview.chromium.org/2467833003/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.h (right):

https://codereview.chromium.org/2467833003/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.h:22: // +
TOOLBAR_PRESENT: The toolbar is present. Moving the cursor to the top
You could move this documentation to be above each value, so you don't have to
repeat the type.

https://codereview.chromium.org/2467833003/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.h:83: // mode.
|enterFullscreenMode| should be called when the window is about to
nit: Need a dash before enterFullscreenMode (like on line 84)

[spqchan, 2016-11-04 18:56:32]

thanks!

https://codereview.chromium.org/2467833003/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.h (right):

https://codereview.chromium.org/2467833003/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.h:22: // +
TOOLBAR_PRESENT: The toolbar is present. Moving the cursor to the top
On 2016/11/04 16:31:57, Robert Sesek wrote:
> You could move this documentation to be above each value, so you don't have to
> repeat the type.

Done.

https://codereview.chromium.org/2467833003/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.h:83: // mode.
|enterFullscreenMode| should be called when the window is about to
On 2016/11/04 16:31:57, Robert Sesek wrote:
> nit: Need a dash before enterFullscreenMode (like on line 84)

Done.

[spqchan, 2016-11-04 18:56:37]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-11-04 18:56:38]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
erikchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2467833003/#ps140001
(title: "fix for rsesek")

[commit-bot: I haz the power, 2016-11-04 18:57:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 19:36:38]

Description was changed from

==========
[Mac] Move the fullscreen toolbar style code into FullscreenToolbarController

- No intended behavioral changes.
- Renamed "FullscreenSlidingStyle" to "FullscreenToolbarStyle" and moved the
logic
 into FullscreenToolbarController 

BUG=643418
==========

to

==========
[Mac] Move the fullscreen toolbar style code into FullscreenToolbarController

- No intended behavioral changes.
- Renamed "FullscreenSlidingStyle" to "FullscreenToolbarStyle" and moved the
logic
 into FullscreenToolbarController 

BUG=643418
==========

[commit-bot: I haz the power, 2016-11-04 19:36:39]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-11-04 20:00:23]

Description was changed from

==========
[Mac] Move the fullscreen toolbar style code into FullscreenToolbarController

- No intended behavioral changes.
- Renamed "FullscreenSlidingStyle" to "FullscreenToolbarStyle" and moved the
logic
 into FullscreenToolbarController 

BUG=643418
==========

to

==========
[Mac] Move the fullscreen toolbar style code into FullscreenToolbarController

- No intended behavioral changes.
- Renamed "FullscreenSlidingStyle" to "FullscreenToolbarStyle" and moved the
logic
 into FullscreenToolbarController

BUG=643418

Committed: https://crrev.com/3c5b671d18c00040c5313257a619a197d4a7daaa
Cr-Commit-Position: refs/heads/master@{#429979}
==========

[commit-bot: I haz the power, 2016-11-04 20:00:24]

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