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

Issue 2484973004: [Mac] Refactor Immersive Fullscreen Logic (Closed)

Created:
4 years, 1 month ago by spqchan
Modified:
4 years, 1 month ago
Reviewers:
Robert Sesek, erikchen
CC:
chromium-reviews, mac-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[Mac] Refactor Immersive Fullscreen Logic - Move immersive fullscreen logic from FullscreenToolbarController to its own class. - Fixed a bug where the menubar doesn't appear properly for immersive fullscreen BUG=643418 Committed: https://crrev.com/69c6fdec22b2e94128f6ad016ff7b496b2e6704a Cr-Commit-Position: refs/heads/master@{#431099}

Patch Set 1 #

Patch Set 2 : Refactored #

Total comments: 6

Patch Set 3 : Fix for erikchen #

Total comments: 4

Patch Set 4 : fix for rsesek #

Unified diffs Side-by-side diffs Delta from patch set Stats (+275 lines, -181 lines) Patch
M chrome/browser/ui/BUILD.gn View 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/browser_window_controller_private.mm View 1 chunk +0 lines, -7 lines 0 comments Download
M chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_mouse_tracker.mm View 1 chunk +1 line, -6 lines 0 comments Download
A chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.h View 1 2 3 1 chunk +30 lines, -0 lines 0 comments Download
A chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm View 1 2 3 1 chunk +205 lines, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/fullscreen_toolbar_controller.h View 3 chunks +4 lines, -19 lines 0 comments Download
M chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm View 9 chunks +22 lines, -149 lines 0 comments Download
M ui/base/cocoa/tracking_area.h View 1 chunk +4 lines, -0 lines 0 comments Download
M ui/base/cocoa/tracking_area.mm View 1 chunk +7 lines, -0 lines 0 comments Download

Messages

Total messages: 26 (16 generated)
spqchan
PTAL
4 years, 1 month ago (2016-11-09 00:24:05 UTC) #6
erikchen
lgtm https://codereview.chromium.org/2484973004/diff/20001/chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm File chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm (right): https://codereview.chromium.org/2484973004/diff/20001/chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm#newcode113 chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm:113: return [self doesScreenHaveMenubar] && shouldShowMenubar_; This method nomenclature ...
4 years, 1 month ago (2016-11-09 00:39:43 UTC) #7
erikchen
On 2016/11/09 00:39:43, erikchen wrote: > lgtm > > https://codereview.chromium.org/2484973004/diff/20001/chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm > File chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm > (right): ...
4 years, 1 month ago (2016-11-09 00:40:00 UTC) #8
spqchan
thanks! +rsesek https://codereview.chromium.org/2484973004/diff/20001/chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm File chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm (right): https://codereview.chromium.org/2484973004/diff/20001/chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm#newcode113 chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm:113: return [self doesScreenHaveMenubar] && shouldShowMenubar_; On 2016/11/09 ...
4 years, 1 month ago (2016-11-09 17:26:06 UTC) #15
Robert Sesek
LGTM https://codereview.chromium.org/2484973004/diff/40001/chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.h File chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.h (right): https://codereview.chromium.org/2484973004/diff/40001/chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.h#newcode18 chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.h:18: - (id)initWithBrowserController:(BrowserWindowController*)bwc; instancetype https://codereview.chromium.org/2484973004/diff/40001/chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm File chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm (right): https://codereview.chromium.org/2484973004/diff/40001/chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm#newcode130 ...
4 years, 1 month ago (2016-11-09 19:23:03 UTC) #16
spqchan
thanks! https://codereview.chromium.org/2484973004/diff/40001/chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.h File chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.h (right): https://codereview.chromium.org/2484973004/diff/40001/chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.h#newcode18 chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.h:18: - (id)initWithBrowserController:(BrowserWindowController*)bwc; On 2016/11/09 19:23:03, Robert Sesek wrote: ...
4 years, 1 month ago (2016-11-09 22:34:40 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2484973004/60001
4 years, 1 month ago (2016-11-09 22:35:19 UTC) #20
commit-bot: I haz the power
Failed to apply the patch. On branch working_branch Your branch is up-to-date with 'origin/refs/pending/heads/master'. nothing ...
4 years, 1 month ago (2016-11-10 00:33:37 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2484973004/60001
4 years, 1 month ago (2016-11-10 00:38:05 UTC) #24
commit-bot: I haz the power
4 years, 1 month ago (2016-11-10 00:38:23 UTC) #26
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/69c6fdec22b2e94128f6ad016ff7b496b2e6704a
Cr-Commit-Position: refs/heads/master@{#431099}

Powered by Google App Engine
This is Rietveld 408576698