Mac: Fix accidental changes to fullscreen logic from refactor.

Mac: Fix accidental changes to fullscreen logic from refactor.

The menu item "Enter Presentation Mode" has different effects in 10.6 and
10.7+. In 10.6, the menu item invokes Immersive Fullscreen. In 10.7+, the menu
item invokes AppKit Fullscreen. Similar logic applies to fullscreen invoked by
an extension.

I accidentally changed this logic during the fullscreen refactor by removing
the 10.6 vs 10.7+ conditional. This CL adds back the conditional. I also
updated the code to explicitly indicate the mechanism that is invoking
fullscreen to prevent similar future mistakes.

BUG=NONE

Committed: https://crrev.com/507054ccac1778eace34f76afd40ade6e855d6fc
Cr-Commit-Position: refs/heads/master@{#295321}

[erikchen, 2014-09-15 23:48:38]

Patchset #1 (id:1) has been deleted

[erikchen, 2014-09-15 23:55:36]

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

[erikchen, 2014-09-15 23:55:38]

rsesek: Please review.

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

https://codereview.chromium.org/575653003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:357: FullscreenExitBubbleType
bubble_type) {
I considered moving this logic into fullscreen_controller. It would be less
fragile that way, as these inferences wouldn't have to be made, but it would
also be adding complexity to cross-platform code. Given that I expect this logic
to be changed in the near future, I've kept this logic inside of
chrome/browser/ui/cocoa

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

https://codereview.chromium.org/575653003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller.mm:2128: -
(void)enterExtensionFullscreenForURL:(const GURL&)url
I've intentionally separated out this logic from enterPresentationMode to
enforce the restriction that selecting the NSMenuItem for "Enter Presentation
Mode" does not use fullscreenUrl_ or fullscreenBubbleType_

[Robert Sesek, 2014-09-16 15:49:03]

It's unclear from the CL description how you're changing the behavior, just that
you're changing it.

https://codereview.chromium.org/575653003/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/575653003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller.h:474: //   - User selects the
menu item corresponding to "Canonical Fullscreen".
We're not going to call it that in the menu, so this name seems wrong.

https://codereview.chromium.org/575653003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller.h:485: //   - User requests
fullscreen via Flash or js apis.
js ->  JavaScript

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

https://codereview.chromium.org/575653003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller.mm:2133: [self
enterPresentationMode];
Does this call -updateFullscreenExitBubble:?

[erikchen, 2014-09-16 17:12:36]

rsesek: PTAL

I updated the CL description to be more precise.

https://codereview.chromium.org/575653003/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/575653003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller.h:474: //   - User selects the
menu item corresponding to "Canonical Fullscreen".
On 2014/09/16 15:49:02, rsesek wrote:
> We're not going to call it that in the menu, so this name seems wrong.

I was intentionally using the internal names for these fullscreen styles, since
the official copy is liable to change at any time. As per your comment, I've
changed the wording of these comments to use the current names of the menu
items.

https://codereview.chromium.org/575653003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller.h:485: //   - User requests
fullscreen via Flash or js apis.
On 2014/09/16 15:49:02, rsesek wrote:
> js ->  JavaScript

Done.

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

https://codereview.chromium.org/575653003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller.mm:2133: [self
enterPresentationMode];
On 2014/09/16 15:49:02, rsesek wrote:
> Does this call -updateFullscreenExitBubble:?

The relevant method is -showFullscreenExitBubbleIfNecessary, which does get
called. If you prefer, I can add comments to the code or even restructure the
logic to make this more clear. My goal with this CL was just to replicate the
same behavior that existed prior to my refactor.

[Robert Sesek, 2014-09-16 21:28:53]

LGTM

[erikchen, 2014-09-17 04:24:36]

Patchset #3 (id:60001) has been deleted

[erikchen, 2014-09-17 18:29:08]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-09-17 18:30:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/575653003/40001

[commit-bot: I haz the power, 2014-09-17 19:28:40]

Committed patchset #2 (id:40001) as de9dd0d901545cbba891099b3ea663f6e884cb5f

[commit-bot: I haz the power, 2014-09-17 19:29:14]

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

[Dan Beam, 2014-09-17 23:00:08]

A revert of this CL (patchset #2 id:40001) has been created in
https://codereview.chromium.org/580973003/ by dbeam@chromium.org.

The reason for reverting is: Seems to break tests on Mac ASAN 64 Tests (3):
https://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests...

AppShimInteractiveTest.Launch (run #1):
[ RUN      ] AppShimInteractiveTest.Launch
[3265:26883:0917/140040:ERROR:launch_services_util.cc(58)] LSOpenApplication:
procNotFound (-600)
BrowserTestBase signal handler received SIGTERM. Backtrace:
0   interactive_ui_tests                0x000000010b22c753
base::debug::StackTrace::StackTrace() + 19
1   interactive_ui_tests                0x0000000115c3ce69 content::(anonymous
namespace)::DumpStackTraceSignalHandler(int) + 185
2   libsystem_c.dylib                   0x00007fff8688f90a _sigtramp + 26
3   ???                                 0x000060e000009dc0 0x0 + 106515188981184
4   CoreFoundation                      0x00007fff86b16233
__CFRunLoopServiceMachPort + 195
5   CoreFoundation                      0x00007fff86b1b916 __CFRunLoopRun + 1078
6   CoreFoundation                      0x00007fff86b1b0e2 CFRunLoopRunSpecific
+ 290
7   HIToolbox                           0x00007fff8f4c8eb4
RunCurrentEventLoopInMode + 209
8   HIToolbox                           0x00007fff8f4c8c52
ReceiveNextEventCommon + 356
9   HIToolbox                           0x00007fff8f4c8ae3
BlockUntilNextEventMatchingListInMode + 62
10  AppKit                              0x00007fff8cc36533 _DPSNextEvent + 685
11  AppKit                              0x00007fff8cc35df2 -[NSApplication
nextEventMatchingMask:untilDate:inMode:dequeue:] + 128
12  AppKit                              0x00007fff8cc2d1a3 -[NSApplication run]
+ 517
13  interactive_ui_tests                0x000000010b1f5186
base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*) + 886
14  interactive_ui_tests                0x000000010b1f3b5a
base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*) + 474
15  interactive_ui_tests                0x000000010b2fbae2 base::RunLoop::Run()
+ 530
16  interactive_ui_tests                0x00000001088df0f8
apps::AppShimInteractiveTest_Launch_Test::RunTestOnMainThread() + 12296
17  interactive_ui_tests                0x0000000109e08b7a
InProcessBrowserTest::RunTestOnMainThreadLoop() + 810
18  interactive_ui_tests                0x0000000115c3c633
content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() + 995
19  interactive_ui_tests                0x0000000108ea85d4
ChromeBrowserMainParts::PreMainMessageLoopRunImpl() + 12132
20  interactive_ui_tests                0x0000000108ea5288
ChromeBrowserMainParts::PreMainMessageLoopRun() + 296
21  interactive_ui_tests                0x0000000112d726a7
content::BrowserMainLoop::PreMainMessageLoopRun() + 375
22  interactive_ui_tests                0x000000011349f906
content::StartupTaskRunner::RunAllTasksNow() + 294
23  interactive_ui_tests                0x0000000112d6df2d
content::BrowserMainLoop::CreateStartupTasks() + 1901
24  interactive_ui_tests                0x0000000112d79558
content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) +
1000
25  interactive_ui_tests                0x0000000112d67754
content::BrowserMain(content::MainFunctionParams const&) + 404
26  interactive_ui_tests                0x0000000115c39b0c
content::ContentMainRunnerImpl::Run() + 476
27  interactive_ui_tests                0x0000000115c37c2e
content::ContentMain(content::ContentMainParams const&) + 142
28  interactive_ui_tests                0x0000000115c3ba37
content::BrowserTestBase::SetUp() + 1799
29  interactive_ui_tests                0x0000000109e065e9
InProcessBrowserTest::SetUp() + 745
30  interactive_ui_tests                0x000000010a510647 testing::Test::Run()
+ 327
31  interactive_ui_tests                0x000000010a512ac1
testing::TestInfo::Run() + 2049
32  interactive_ui_tests                0x000000010a513772
testing::TestCase::Run() + 1042
33  interactive_ui_tests                0x000000010a5285dd
testing::internal::UnitTestImpl::RunAllTests() + 4173
34  interactive_ui_tests                0x000000010a527505
testing::UnitTest::Run() + 389
35  interactive_ui_tests                0x0000000115c35115
base::TestSuite::Run() + 565
36  interactive_ui_tests                0x0000000108db6494
InteractiveUITestSuiteRunner::RunTestSuite(int, char**) + 212
37  interactive_ui_tests                0x0000000115c46cc3
content::LaunchTests(content::TestLauncherDelegate*, int, int, char**) + 1763
38  interactive_ui_tests                0x0000000109e03f14
LaunchChromeTests(int, ChromeTestSuiteRunner*, int, char**) + 244
39  interactive_ui_tests                0x0000000108db6352 main + 194
40  interactive_ui_tests                0x00000001088dbb34 start + 52
41  ???                                 0x000000000000000a 0x0 + 10.