Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1432053002:
Hide the fullscreen toolbar only for FramedBrowserWindow (Closed)
DescriptionAdd a check to hide the fullscreen toolbar only if it's a FramedBrowserWindow. Currently, entering fullscreen will cause all the fullscreen toolbars to be hidden, which we do not want to happen in certain windows, such as for Chrome Apps.
BUG=552043
Committed: https://crrev.com/e026f4038aeb0b33904241654375e1a2d1afd9cb
Cr-Commit-Position: refs/heads/master@{#358907}
Patch Set 1 #Patch Set 2 : Remove extra import statement #Patch Set 3 : Cleaned up the code #Patch Set 4 : #
Total comments: 2
Patch Set 5 : Check for FramedBrowserWindow class instead of using a new method #Patch Set 6 : Updated a comment #Messages
Total messages: 27 (9 generated)
Description was changed from ========== Add a check to hide the toolbar only if the window is not a Chrome App window BUG=552043 ========== to ========== Add a check to hide the toolbar only if the toolbar is not for a Chrome App window. Currently entering fullscreen will cause all the fullscreen toolbars to be hidden, which we do not want to happen in a Chrome App window. BUG=552043 ==========
Description was changed from ========== Add a check to hide the toolbar only if the toolbar is not for a Chrome App window. Currently entering fullscreen will cause all the fullscreen toolbars to be hidden, which we do not want to happen in a Chrome App window. BUG=552043 ========== to ========== Add a check to hide the fullscreen toolbar only it's not for a Chrome App window. Currently, entering fullscreen will cause all the fullscreen toolbars to be hidden, which we do not want to happen in a Chrome App window. BUG=552043 ==========
spqchan@chromium.org changed reviewers: + avi@chromium.org
PTAL
Is there a cleaner way of doing this? Some kind of interface like -shouldHideFullscreenToolbar and then have the different kinds of windows implement it differently? It feels weird to have this core code know about the implementation details of the app windows.
On 2015/11/10 01:28:44, Avi wrote: > Is there a cleaner way of doing this? Some kind of interface like > -shouldHideFullscreenToolbar and then have the different kinds of windows > implement it differently? > > It feels weird to have this core code know about the implementation details of > the app windows. Hm, good point. I can add that method to ChromeProcessingWindow and then override it in AppNSWindow.
PTAL
avi@chromium.org changed reviewers: + erikchen@chromium.org - avi@chromium.org
This isn't event-related code, though... It's been a really long time since I've dealt with this. Erik, you've been dealing with Mac stuff more recently; wdyt?
https://codereview.chromium.org/1432053002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/1432053002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_private.mm:727: SEL selector = @selector(shouldHideFullscreenToolbar); If you're just trying to skip the Chrome App window, why don't you just check the class of the window, rather than adding the method shouldHideFullscreenToolbar? Alternatively, it seems cleaner to whitelist the window used by this class (FramedBrowserWindow) by checking isKindOfClass?
https://codereview.chromium.org/1432053002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/1432053002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_private.mm:727: SEL selector = @selector(shouldHideFullscreenToolbar); On 2015/11/10 08:17:22, erikchen wrote: > If you're just trying to skip the Chrome App window, why don't you just check > the class of the window, rather than adding the method > shouldHideFullscreenToolbar? > > Alternatively, it seems cleaner to whitelist the window used by this class > (FramedBrowserWindow) by checking isKindOfClass? Former: I did that earlier in Patch 2, please see the earlier messages in this CL. Latter: Sure, if all of the windows we want to hide the toolbar for is a FramedBrowserWindow, then I can do that. Otherwise I think having that method would be safer.
On 2015/11/10 17:14:28, spqchan wrote: > https://codereview.chromium.org/1432053002/diff/60001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): > > https://codereview.chromium.org/1432053002/diff/60001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_controller_private.mm:727: SEL selector = > @selector(shouldHideFullscreenToolbar); > On 2015/11/10 08:17:22, erikchen wrote: > > If you're just trying to skip the Chrome App window, why don't you just check > > the class of the window, rather than adding the method > > shouldHideFullscreenToolbar? > > > > Alternatively, it seems cleaner to whitelist the window used by this class > > (FramedBrowserWindow) by checking isKindOfClass? > > Former: I did that earlier in Patch 2, please see the earlier messages in this > CL. > Latter: Sure, if all of the windows we want to hide the toolbar for is a > FramedBrowserWindow, then I can do that. Otherwise I think having that method > would be safer. Let's go with the latter then.
PTAL
lgtm
On 2015/11/10 18:19:50, erikchen wrote: > lgtm please update CL description.
Description was changed from ========== Add a check to hide the fullscreen toolbar only it's not for a Chrome App window. Currently, entering fullscreen will cause all the fullscreen toolbars to be hidden, which we do not want to happen in a Chrome App window. BUG=552043 ========== to ========== Add a check to hide the fullscreen toolbar only if it's a FramedBrowserWindow. Currently, entering fullscreen will cause all the fullscreen toolbars to be hidden, which we do not want to happen in certain windows, such as for Chrome Apps. BUG=552043 ==========
The CQ bit was checked by spqchan@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1432053002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1432053002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
spqchan@chromium.org changed reviewers: + avi@chromium.org
+avi for LGTM stamp
lgtm stamp
The CQ bit was checked by spqchan@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1432053002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1432053002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e026f4038aeb0b33904241654375e1a2d1afd9cb Cr-Commit-Position: refs/heads/master@{#358907} |
