Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(79)

Issue 575653003: Mac: Fix accidental changes to fullscreen logic from refactor. (Closed)

Created:
6 years, 3 months ago by erikchen
Modified:
6 years, 3 months ago
Reviewers:
Robert Sesek
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@fullscreen_layout_refactor3
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 : #

Total comments: 8

Patch Set 2 : Comments from rsesek. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+68 lines, -15 lines) Patch
M chrome/browser/ui/cocoa/browser_window_cocoa.mm View 2 chunks +15 lines, -3 lines 0 comments Download
M chrome/browser/ui/cocoa/browser_window_controller.h View 1 2 chunks +33 lines, -5 lines 0 comments Download
M chrome/browser/ui/cocoa/browser_window_controller.mm View 2 chunks +20 lines, -7 lines 0 comments Download

Messages

Total messages: 12 (4 generated)
erikchen
rsesek: Please review. https://codereview.chromium.org/575653003/diff/20001/chrome/browser/ui/cocoa/browser_window_cocoa.mm File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): https://codereview.chromium.org/575653003/diff/20001/chrome/browser/ui/cocoa/browser_window_cocoa.mm#newcode357 chrome/browser/ui/cocoa/browser_window_cocoa.mm:357: FullscreenExitBubbleType bubble_type) { I considered moving ...
6 years, 3 months ago (2014-09-15 23:55:38 UTC) #3
Robert Sesek
It's unclear from the CL description how you're changing the behavior, just that you're changing ...
6 years, 3 months ago (2014-09-16 15:49:03 UTC) #4
erikchen
rsesek: PTAL I updated the CL description to be more precise. https://codereview.chromium.org/575653003/diff/20001/chrome/browser/ui/cocoa/browser_window_controller.h File chrome/browser/ui/cocoa/browser_window_controller.h (right): ...
6 years, 3 months ago (2014-09-16 17:12:36 UTC) #5
Robert Sesek
LGTM
6 years, 3 months ago (2014-09-16 21:28:53 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/575653003/40001
6 years, 3 months ago (2014-09-17 18:30:47 UTC) #9
commit-bot: I haz the power
Committed patchset #2 (id:40001) as de9dd0d901545cbba891099b3ea663f6e884cb5f
6 years, 3 months ago (2014-09-17 19:28:40 UTC) #10
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/507054ccac1778eace34f76afd40ade6e855d6fc Cr-Commit-Position: refs/heads/master@{#295321}
6 years, 3 months ago (2014-09-17 19:29:14 UTC) #11
Dan Beam
6 years, 3 months ago (2014-09-17 23:00:08 UTC) #12
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld 408576698