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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 11896104: Refactor FullscreenController removing TogglePresentationMode & adding ToggleFullscreenWithChrome. (Closed)

Created:
7 years, 11 months ago by scheib
Modified:
7 years, 10 months ago
Reviewers:
Robert Sesek, sky
CC:
chromium-reviews, scheib+watch_chromium.org
Visibility:
Public.

Description

Refactor FullscreenController removing TogglePresentationMode & adding ToggleFullscreenWithChrome. Part of a series of refactoring changes that will enable simpler code in FullscreenController as well as correcting behavior there. This change attempts to make the minimal modifications required in order to have FullscreenController::ToggleFullscreenMode consistently mean fullscreen with no browser chrome on all platforms. Depends on https://codereview.chromium.org/12018007/ BUG=169138 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179554

Patch Set 1 : #

Total comments: 2

Patch Set 2 : #

Patch Set 3 : Merge TOT #

Total comments: 2

Patch Set 4 : moved to browser_commands_mac #

Patch Set 5 : Mac build fix for fullscreen_controller_interactive_browsertest.cc #

Patch Set 6 : Merge TOT #

Patch Set 7 : chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm compile fix #

Unified diffs Side-by-side diffs Delta from patch set Stats (+108 lines, -89 lines) Patch
M chrome/browser/ui/browser.h View 1 chunk +0 lines, -3 lines 0 comments Download
M chrome/browser/ui/browser.cc View 1 chunk +0 lines, -6 lines 0 comments Download
M chrome/browser/ui/browser_command_controller.cc View 1 2 3 3 chunks +9 lines, -1 line 0 comments Download
A chrome/browser/ui/browser_commands_mac.h View 1 2 3 1 chunk +16 lines, -0 lines 0 comments Download
A chrome/browser/ui/browser_commands_mac.cc View 1 2 3 1 chunk +16 lines, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm View 1 2 3 4 5 6 4 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/ui/fullscreen/fullscreen_controller.h View 1 3 chunks +12 lines, -6 lines 0 comments Download
M chrome/browser/ui/fullscreen/fullscreen_controller.cc View 1 2 9 chunks +48 lines, -65 lines 0 comments Download
M chrome/browser/ui/fullscreen/fullscreen_controller_interactive_browsertest.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/fullscreen/fullscreen_controller_state_test.cc View 1 2 3 4 5 1 chunk +0 lines, -4 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
scheib
7 years, 11 months ago (2013-01-25 00:41:33 UTC) #1
Robert Sesek
https://codereview.chromium.org/11896104/diff/3001/chrome/browser/ui/fullscreen/fullscreen_controller.h File chrome/browser/ui/fullscreen/fullscreen_controller.h (right): https://codereview.chromium.org/11896104/diff/3001/chrome/browser/ui/fullscreen/fullscreen_controller.h#newcode145 chrome/browser/ui/fullscreen/fullscreen_controller.h:145: void ToggleFullscreenModeInternal(bool for_tab, bool with_chrome); Since there's already a ...
7 years, 10 months ago (2013-01-28 19:46:22 UTC) #2
scheib
Thanks rsesek. sky, owners please. https://codereview.chromium.org/11896104/diff/3001/chrome/browser/ui/fullscreen/fullscreen_controller.h File chrome/browser/ui/fullscreen/fullscreen_controller.h (right): https://codereview.chromium.org/11896104/diff/3001/chrome/browser/ui/fullscreen/fullscreen_controller.h#newcode145 chrome/browser/ui/fullscreen/fullscreen_controller.h:145: void ToggleFullscreenModeInternal(bool for_tab, bool ...
7 years, 10 months ago (2013-01-28 20:40:02 UTC) #3
Robert Sesek
LGTM
7 years, 10 months ago (2013-01-29 20:39:40 UTC) #4
scheib
Thanks rsesek, sky, owners review for files in chrome/browser/ui/.
7 years, 10 months ago (2013-01-29 20:57:30 UTC) #5
sky
https://codereview.chromium.org/11896104/diff/12002/chrome/browser/ui/browser_commands.h File chrome/browser/ui/browser_commands.h (right): https://codereview.chromium.org/11896104/diff/12002/chrome/browser/ui/browser_commands.h#newcode144 chrome/browser/ui/browser_commands.h:144: #if defined(OS_MACOSX) How about moving this and the implementation ...
7 years, 10 months ago (2013-01-29 22:19:06 UTC) #6
scheib
Thx sky, https://codereview.chromium.org/11896104/diff/12002/chrome/browser/ui/browser_commands.h File chrome/browser/ui/browser_commands.h (right): https://codereview.chromium.org/11896104/diff/12002/chrome/browser/ui/browser_commands.h#newcode144 chrome/browser/ui/browser_commands.h:144: #if defined(OS_MACOSX) On 2013/01/29 22:19:06, sky wrote: ...
7 years, 10 months ago (2013-01-29 23:58:11 UTC) #7
sky
Thanks - LGTM
7 years, 10 months ago (2013-01-30 01:01:19 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scheib@chromium.org/11896104/10016
7 years, 10 months ago (2013-01-30 01:32:59 UTC) #9
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 10 months ago (2013-01-30 02:56:40 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scheib@chromium.org/11896104/10017
7 years, 10 months ago (2013-01-30 05:11:29 UTC) #11
commit-bot: I haz the power
7 years, 10 months ago (2013-01-30 07:40:43 UTC) #12
Message was sent while issue was closed. 
Change committed as 179554

Powered by Google App Engine
This is Rietveld 408576698