Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2001423002:
Mac: Start removing Cocoa fullscreen code. (Closed)
|
Created:
4 years, 7 months ago by Matt Giuca Modified:
4 years, 7 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMac: Start removing Cocoa fullscreen code.
Previously, the simplified-fullscreen-ui flag controlled whether to use
the new Views fullscreen notification bubble, or the old Cocoa one. Now
that this flag has been removed on Mac, the Cocoa bubble code is no
longer used. This change should have no visible effect.
BUG=610900
Committed: https://crrev.com/96345e63d03712e55fb306eef268265109a177e9
Cr-Commit-Position: refs/heads/master@{#396393}
Patch Set 1 #Patch Set 2 : Remove special case string on Mac. #
Total comments: 12
Patch Set 3 : Respond to review. #
Messages
Total messages: 22 (10 generated)
mgiuca@chromium.org changed reviewers: + tapted@chromium.org
Starting to delete all the old fullscreen code. Starting with making the flag on Mac work the same as on Windows.
Will follow up with this: https://codereview.chromium.org/2001103003
mgiuca@chromium.org changed reviewers: + scheib@chromium.org
scheib@chromium.org: exclusive_access_bubble.cc.
LGTM with: https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm (right): https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm:37: return; return; not needed.
https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm (left): https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm:12: #import "chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.h" Delete exclusive_access_bubble_window_controller.h and exclusive_access_bubble_window_controller.mm? (and exclusive_access_bubble_window_controller_unittest.mm). Nothing else calls [ExclusiveAccessBubbleWindowController alloc] after this. There's a #import in browser_window_controller.h but I think you can just delete it. edit: ah, I see you have this in a follow-up. Cool. https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm (right): https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm:141: NOTREACHED(); // Only used for non-simplified UI. Can this be hit now? That is, if we're partially adding back cursor tracking to re-show the bubble at the screen edge, Mac should still be excluded, since that bit isn't plumbed through here. Maybe we should first hardcode IsSimplifiedFullscreenUIEnabled() on Mac with #ifdefs so that the flag can't be disabled at all on Mac https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc (left): https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:194: return exclusive_access_bubble::GetInstructionTextForType(bubble_type_, Can ExclusiveAccessBubbleViews::UpdateViewContent() just call this directly? (i.e. remove GetInstructionText() completely). A follow-up is fine, but it's not in your current set.
https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm (left): https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm:12: #import "chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.h" On 2016/05/25 03:53:27, tapted wrote: > Delete exclusive_access_bubble_window_controller.h and > exclusive_access_bubble_window_controller.mm? (and > exclusive_access_bubble_window_controller_unittest.mm). Nothing else calls > [ExclusiveAccessBubbleWindowController alloc] after this. > > There's a #import in browser_window_controller.h but I think you can just delete > it. > > edit: ah, I see you have this in a follow-up. Cool. Acknowledged. https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm (right): https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm:37: return; On 2016/05/24 19:43:47, scheib wrote: > return; not needed. Done. https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm:141: NOTREACHED(); // Only used for non-simplified UI. On 2016/05/25 03:53:27, tapted wrote: > Can this be hit now? That is, if we're partially adding back cursor tracking to > re-show the bubble at the screen edge, Mac should still be excluded, since that > bit isn't plumbed through here. Maybe we should first hardcode > IsSimplifiedFullscreenUIEnabled() on Mac with #ifdefs so that the flag can't be > disabled at all on Mac I haven't actually tested this (outside of running try bots). Would you be able to patch it and see if it works with the flag disabled? If it does work on Mac then there's no harm in keeping it. If not, yeah I'll just send out a preliminary CL to remove the flag on Mac. https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc (left): https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:194: return exclusive_access_bubble::GetInstructionTextForType(bubble_type_, On 2016/05/25 03:53:27, tapted wrote: > Can ExclusiveAccessBubbleViews::UpdateViewContent() just call this directly? > (i.e. remove GetInstructionText() completely). A follow-up is fine, but it's not > in your current set. No, because EABV doesn't have access to bubble_type_. I think this is still useful abstraction despite only being one line, as it shields the view client from the bubble type stuff.
https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm (right): https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm:141: NOTREACHED(); // Only used for non-simplified UI. On 2016/05/26 04:15:40, Matt Giuca wrote: > On 2016/05/25 03:53:27, tapted wrote: > > Can this be hit now? That is, if we're partially adding back cursor tracking > to > > re-show the bubble at the screen edge, Mac should still be excluded, since > that > > bit isn't plumbed through here. Maybe we should first hardcode > > IsSimplifiedFullscreenUIEnabled() on Mac with #ifdefs so that the flag can't > be > > disabled at all on Mac > > I haven't actually tested this (outside of running try bots). Would you be able > to patch it and see if it works with the flag disabled? If it does work on Mac > then there's no harm in keeping it. If not, yeah I'll just send out a > preliminary CL to remove the flag on Mac. It crash: [21133:1295:0526/150654:FATAL:exclusive_access_controller_views.mm(140)] Check failed: false. 0 libbase.dylib 0x000000011e3b3ecf _ZN4base5debug10StackTraceC2Ev + 47 1 libbase.dylib 0x000000011e3b3f93 _ZN4base5debug10StackTraceC1Ev + 35 2 libbase.dylib 0x000000011e4463f0 _ZN7logging10LogMessageD2Ev + 80 3 libbase.dylib 0x000000011e443c33 _ZN7logging10LogMessageD1Ev + 35 4 libchrome_main_dll.dylib 0x00000001118a02fe _ZN25ExclusiveAccessController25GetBubbleAssociatedWidgetEv + 174 5 libchrome_main_dll.dylib 0x00000001118a037a _ZThn16_N25ExclusiveAccessController25GetBubbleAssociatedWidgetEv + 42 6 libchrome_main_dll.dylib 0x00000001116b0d06 _ZN26ExclusiveAccessBubbleViews14IsWindowActiveEv + 54 7 libchrome_main_dll.dylib 0x0000000111c64265 _ZN21ExclusiveAccessBubble18CheckMousePositionEv + 341 8 libchrome_main_dll.dylib 0x0000000111c6586e _ZN4base8internal15RunnableAdapterIM21ExclusiveAccessBubbleFvvEE3RunIPS2_JEEEvOT_DpOT0_ + 126 https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc (left): https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:194: return exclusive_access_bubble::GetInstructionTextForType(bubble_type_, On 2016/05/26 04:15:40, Matt Giuca wrote: > On 2016/05/25 03:53:27, tapted wrote: > > Can ExclusiveAccessBubbleViews::UpdateViewContent() just call this directly? > > (i.e. remove GetInstructionText() completely). A follow-up is fine, but it's > not > > in your current set. > > No, because EABV doesn't have access to bubble_type_. Sure it does. It assigns it in fact. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... [of course the data member is protected: which is a style guide violation] > I think this is still > useful abstraction despite only being one line, as it shields the view client > from the bubble type stuff. exclusive_access_bubble_views.h is the only file that #includes exclusive_access_bubble.h. I'd suggest we move all of ExclusiveAccessBubble into ExclusiveAccessBubbleViews - maybe that's another good follow-up.
Patchset #4 (id:60001) has been deleted
Description was changed from ========== Mac: Always use the new Views fullscreen bubble. Previously, the simplified-fullscreen-ui flag controlled whether to use the new Views fullscreen notification bubble, or the old Cocoa one. This paves the way to removing the old Cocoa bubble entirely. Note that the Views bubble behaves differently depending on the value of the flag. This change only affects users with simplified-fullscreen-ui flag disabled (it is enabled by default). BUG=610900 ========== to ========== Mac: Start removing Cocoa fullscreen code. Previously, the simplified-fullscreen-ui flag controlled whether to use the new Views fullscreen notification bubble, or the old Cocoa one. Now that this flag has been removed on Mac, the Cocoa bubble code is no longer used. This change should have no visible effect. BUG=610900 ==========
Description was changed from ========== Mac: Start removing Cocoa fullscreen code. Previously, the simplified-fullscreen-ui flag controlled whether to use the new Views fullscreen notification bubble, or the old Cocoa one. Now that this flag has been removed on Mac, the Cocoa bubble code is no longer used. This change should have no visible effect. BUG=610900 ========== to ========== Mac: Start removing Cocoa fullscreen code. Previously, the simplified-fullscreen-ui flag controlled whether to use the new Views fullscreen notification bubble, or the old Cocoa one. Now that this flag has been removed on Mac, the Cocoa bubble code is no longer used. This change should have no visible effect. BUG=610900 ==========
https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm (right): https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm:141: NOTREACHED(); // Only used for non-simplified UI. On 2016/05/26 05:08:41, tapted wrote: > On 2016/05/26 04:15:40, Matt Giuca wrote: > > On 2016/05/25 03:53:27, tapted wrote: > > > Can this be hit now? That is, if we're partially adding back cursor tracking > > to > > > re-show the bubble at the screen edge, Mac should still be excluded, since > > that > > > bit isn't plumbed through here. Maybe we should first hardcode > > > IsSimplifiedFullscreenUIEnabled() on Mac with #ifdefs so that the flag can't > > be > > > disabled at all on Mac > > > > I haven't actually tested this (outside of running try bots). Would you be > able > > to patch it and see if it works with the flag disabled? If it does work on Mac > > then there's no harm in keeping it. If not, yeah I'll just send out a > > preliminary CL to remove the flag on Mac. > > It crash: > > [21133:1295:0526/150654:FATAL:exclusive_access_controller_views.mm(140)] Check > failed: false. > 0 libbase.dylib 0x000000011e3b3ecf > _ZN4base5debug10StackTraceC2Ev + 47 > 1 libbase.dylib 0x000000011e3b3f93 > _ZN4base5debug10StackTraceC1Ev + 35 > 2 libbase.dylib 0x000000011e4463f0 > _ZN7logging10LogMessageD2Ev + 80 > 3 libbase.dylib 0x000000011e443c33 > _ZN7logging10LogMessageD1Ev + 35 > 4 libchrome_main_dll.dylib 0x00000001118a02fe > _ZN25ExclusiveAccessController25GetBubbleAssociatedWidgetEv + 174 > 5 libchrome_main_dll.dylib 0x00000001118a037a > _ZThn16_N25ExclusiveAccessController25GetBubbleAssociatedWidgetEv + 42 > 6 libchrome_main_dll.dylib 0x00000001116b0d06 > _ZN26ExclusiveAccessBubbleViews14IsWindowActiveEv + 54 > 7 libchrome_main_dll.dylib 0x0000000111c64265 > _ZN21ExclusiveAccessBubble18CheckMousePositionEv + 341 > 8 libchrome_main_dll.dylib 0x0000000111c6586e > _ZN4base8internal15RunnableAdapterIM21ExclusiveAccessBubbleFvvEE3RunIPS2_JEEEvOT_DpOT0_ > + 126 OK. Sent out a CL to disable the flag on Mac which should land before this. https://codereview.chromium.org/2012763005 Reworded the CL description (now this CL has no visible behaviour change). https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc (left): https://codereview.chromium.org/2001423002/diff/20001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:194: return exclusive_access_bubble::GetInstructionTextForType(bubble_type_, On 2016/05/26 05:08:41, tapted wrote: > On 2016/05/26 04:15:40, Matt Giuca wrote: > > On 2016/05/25 03:53:27, tapted wrote: > > > Can ExclusiveAccessBubbleViews::UpdateViewContent() just call this directly? > > > (i.e. remove GetInstructionText() completely). A follow-up is fine, but it's > > not > > > in your current set. > > > > No, because EABV doesn't have access to bubble_type_. > > Sure it does. It assigns it in fact. > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > [of course the data member is protected: which is a style guide violation] > > > I think this is still > > useful abstraction despite only being one line, as it shields the view client > > from the bubble type stuff. > > exclusive_access_bubble_views.h is the only file that #includes > exclusive_access_bubble.h. I'd suggest we move all of ExclusiveAccessBubble into > ExclusiveAccessBubbleViews - maybe that's another good follow-up. Oh OK, gross. Well I still think this is a useful abstraction for now to prevent further leaking of this variable. That's a good idea for a follow-up.
lgtm
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by mgiuca@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org Link to the patchset: https://codereview.chromium.org/2001423002/#ps40001 (title: "Respond to review.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001423002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2001423002/40001
Message was sent while issue was closed.
Description was changed from ========== Mac: Start removing Cocoa fullscreen code. Previously, the simplified-fullscreen-ui flag controlled whether to use the new Views fullscreen notification bubble, or the old Cocoa one. Now that this flag has been removed on Mac, the Cocoa bubble code is no longer used. This change should have no visible effect. BUG=610900 ========== to ========== Mac: Start removing Cocoa fullscreen code. Previously, the simplified-fullscreen-ui flag controlled whether to use the new Views fullscreen notification bubble, or the old Cocoa one. Now that this flag has been removed on Mac, the Cocoa bubble code is no longer used. This change should have no visible effect. BUG=610900 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Mac: Start removing Cocoa fullscreen code. Previously, the simplified-fullscreen-ui flag controlled whether to use the new Views fullscreen notification bubble, or the old Cocoa one. Now that this flag has been removed on Mac, the Cocoa bubble code is no longer used. This change should have no visible effect. BUG=610900 ========== to ========== Mac: Start removing Cocoa fullscreen code. Previously, the simplified-fullscreen-ui flag controlled whether to use the new Views fullscreen notification bubble, or the old Cocoa one. Now that this flag has been removed on Mac, the Cocoa bubble code is no longer used. This change should have no visible effect. BUG=610900 Committed: https://crrev.com/96345e63d03712e55fb306eef268265109a177e9 Cr-Commit-Position: refs/heads/master@{#396393} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/96345e63d03712e55fb306eef268265109a177e9 Cr-Commit-Position: refs/heads/master@{#396393} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
