mac: Fix bug where exiting tab fullscreen would cause Chrome menubar to disappear.

mac: Fix bug where exiting tab fullscreen would cause Chrome menubar to disappear.

Prior to my major fullscreen refactor, Presentation Mode and Canonical
Fullscreen had significant implementation differences. As a result,
fullscreen_controller treated the two modes very differently. If a user tried
to enter tab fullscreen after entering browser fullscreen (Presentation Mode),
enterImmersiveFullscreen: would not be invoked. If a user tried to do the same
after Canonical Fullscreen, enterImmersiveFullscreen: would be invoked.

After my major fullscreen refactor, Presentation Mode and Canonical Fullscreen
are almost identical. However, I failed to update the logic in
fullscreen_controller, which still treated the two modes differently.

This CL updates the logic in fullscreen_controller to treat both modes like
Presentation Mode. If a user tries to enter tab fullscreen after entering
browser fullscreen, enterImmersiveFullscreen: is not invoked.

BUG=422191
Committed: https://crrev.com/da9c656f686f5b4aaa56d56ecc82024aefd94719
Cr-Commit-Position: refs/heads/master@{#299838}

[erikchen, 2014-10-14 22:26:05]

Patchset #1 (id:1) has been deleted

[erikchen, 2014-10-14 22:26:10]

Patchset #1 (id:20001) has been deleted

[erikchen, 2014-10-14 22:31:11]

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

[erikchen, 2014-10-14 22:31:12]

rsesek: Please review.

I couldn't come up with any tests that weren't regressive. There are 2 major
roadblocks: browser_tests and interactive_tests that use AppKit Fullscreen are
flaky, and fullscreen_controller_state_unittest doesn't support the relevant
transitions  (http://crbug.com/156968).

Given the size and nature of this CL, I'm inclined to not write tests for it.
Let me know if you disagree with this sentiment.

[Robert Sesek, 2014-10-15 20:28:14]

Does this pass the fullscreen_controller_unittest.cc ?

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

https://codereview.chromium.org/652983005/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:493:
[presentationModeController_ exitPresentationMode];
I don't follow this...

[erikchen, 2014-10-15 21:42:04]

Patchset #2 (id:60001) has been deleted

[erikchen, 2014-10-15 21:50:35]

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

https://codereview.chromium.org/652983005/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:493:
[presentationModeController_ exitPresentationMode];
On 2014/10/15 20:28:14, Robert Sesek wrote:
> I don't follow this...

presentationModeController_.get().slidingStyle = style; isn't sufficient to
reset all state of the presentationModeController. That being said, it doesn't
affect the current CL, or the current functionality, so I'm dropping it from
this CL

[erikchen, 2014-10-15 21:58:29]

On 2014/10/15 20:28:14, Robert Sesek wrote:
> Does this pass the fullscreen_controller_unittest.cc ?
Patch set 1 did not pass the test. Patch set 2 does.

Introducing minor errors to my patch did cause the fullscreen unit_tests to
fail, so it looks like this CL does have test coverage. 

> 
>
https://codereview.chromium.org/652983005/diff/40001/chrome/browser/ui/cocoa/...
> File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):
> 
>
https://codereview.chromium.org/652983005/diff/40001/chrome/browser/ui/cocoa/...
> chrome/browser/ui/cocoa/browser_window_controller_private.mm:493:
> [presentationModeController_ exitPresentationMode];
> I don't follow this...

[Robert Sesek, 2014-10-15 22:34:41]

LGTM

[erikchen, 2014-10-15 22:37:53]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-10-15 22:39:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/652983005/80001

[commit-bot: I haz the power, 2014-10-15 22:49:51]

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

[commit-bot: I haz the power, 2014-10-15 22:49:52]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[erikchen, 2014-10-15 22:59:48]

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

[erikchen, 2014-10-15 22:59:49]

scheib: Looking for an OWNER review of chrome/browser/ui/fullscreen/*

[scheib, 2014-10-15 23:12:29]

lgtm

[erikchen, 2014-10-15 23:16:41]

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

[erikchen, 2014-10-15 23:16:41]

sky: Looking for an OWNER review of chrome/browser/ui/browser_window.h.

[sky, 2014-10-15 23:59:08]

LGTM

[erikchen, 2014-10-16 00:49:44]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-10-16 00:54:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/652983005/80001

[commit-bot: I haz the power, 2014-10-16 04:23:11]

Committed patchset #2 (id:80001)

[commit-bot: I haz the power, 2014-10-16 04:23:54]

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