Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
402443003:
mac: [Yosemite] Fix bug where zoom button causes fullscreen mode. (Closed)
Descriptionmac: [Yosemite] Fix bug where zoom button causes fullscreen mode.
In Yosemite, holding down alt while clicking the fullscreen button is supposed
to zoom the window. This doesn't work because Chrome violates unstated AppKit
assumptions.
The problem seems to lie in hit testing the location of the zoom button.
Reordering the subviews by moving the tab strip and the content view behind the
zoom button fixes the problem.
BUG=393808
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284544
Patch Set 1 : #Patch Set 2 : Complete rewrite. #
Total comments: 6
Patch Set 3 : Comments from andresantoso. #
Total comments: 2
Patch Set 4 : Comments from andresantoso, round 2. #
Total comments: 2
Patch Set 5 : Rebase against top of tree. #Messages
Total messages: 22 (0 generated)
andresantoso: Please review.
andresantoso: Please Review.
https://codereview.chromium.org/402443003/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/version_independent_window.mm (right): https://codereview.chromium.org/402443003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/version_independent_window.mm:26: if ([firstView isKindOfClass:[FullSizeContentView class]]) How about (firstView == [[firstView window] contentView])? I think it reads better, and RTTI should be avoided when possible. You can also then delete the above comment about "more than 1 subclass ..." https://codereview.chromium.org/402443003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/version_independent_window.mm:34: return NSOrderedDescending; Suggestion: return (index1 < index2) ? NSOrderedAscending : NSOrderedDescending. https://codereview.chromium.org/402443003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/version_independent_window.mm:117: NSArray* subviews = [[[superview subviews] copy] autorelease]; Use scoped_nsobject?
andresantoso: PTAL https://codereview.chromium.org/402443003/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/version_independent_window.mm (right): https://codereview.chromium.org/402443003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/version_independent_window.mm:26: if ([firstView isKindOfClass:[FullSizeContentView class]]) On 2014/07/17 17:56:16, Andre wrote: > How about (firstView == [[firstView window] contentView])? > I think it reads better, and RTTI should be avoided when possible. > You can also then delete the above comment about "more than 1 subclass ..." Seems reasonable to me. Done. https://codereview.chromium.org/402443003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/version_independent_window.mm:34: return NSOrderedDescending; On 2014/07/17 17:56:16, Andre wrote: > Suggestion: return (index1 < index2) ? NSOrderedAscending : NSOrderedDescending. I see that you are a fan of the ternary operator. Done. https://codereview.chromium.org/402443003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/version_independent_window.mm:117: NSArray* subviews = [[[superview subviews] copy] autorelease]; On 2014/07/17 17:56:16, Andre wrote: > Use scoped_nsobject? Done.
https://codereview.chromium.org/402443003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/version_independent_window.mm (right): https://codereview.chromium.org/402443003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/version_independent_window.mm:24: NSWindow* contentView = [[firstView window] contentView]; NSView* contentView
https://codereview.chromium.org/402443003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/version_independent_window.mm (right): https://codereview.chromium.org/402443003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/version_independent_window.mm:24: NSWindow* contentView = [[firstView window] contentView]; On 2014/07/17 18:25:36, Andre wrote: > NSView* contentView Done.
lgtm
lgtm
shess: Looking for an OWNER review of chrome/browser/ui/cocoa
LGTM, your call as to whether you want to experiment with my suggestion. https://codereview.chromium.org/402443003/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/version_independent_window.mm (right): https://codereview.chromium.org/402443003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/version_independent_window.mm:117: context:subviews.get()]; Wow. I'm assuming the comment refers to an attempt to just [superview addSubview:contentView positioned:NSWindowBelow relativeTo:nil]? Another possibility which sounds like it might reorder things more directly: scoped_nsobject<NSMutableArray> subviews([[superview subviews] mutableCopy]); [subviews removeObject:contentView]; // assumes a ref is held elsewhere [subviews insertObject:contentView atIndex:0]; [superview setSubviews:subviews]; AFAICT it shouldn't call -addSubview: or -removeSubview: in this case, instead it will just reorder to match.
https://codereview.chromium.org/402443003/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/version_independent_window.mm (right): https://codereview.chromium.org/402443003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/version_independent_window.mm:117: context:subviews.get()]; On 2014/07/17 21:03:09, Scott Hess wrote: > Wow. > > I'm assuming the comment refers to an attempt to just [superview > addSubview:contentView positioned:NSWindowBelow relativeTo:nil]? > > Another possibility which sounds like it might reorder things more directly: > scoped_nsobject<NSMutableArray> subviews([[superview subviews] mutableCopy]); > [subviews removeObject:contentView]; // assumes a ref is held elsewhere > [subviews insertObject:contentView atIndex:0]; > [superview setSubviews:subviews]; > AFAICT it shouldn't call -addSubview: or -removeSubview: in this case, instead > it will just reorder to match. I tried out your suggestion. It works fine and doesn't raise any warnings. That being said, given that Apple has cracked down on addSubview:, setting the subviews property seems like the next obvious method to shut down, since that allows addition/removal of views. In place sorting of subviews seems much less likely to produce warnings from future releases of AppKit.
On 2014/07/17 21:17:08, erikchen wrote: > https://codereview.chromium.org/402443003/diff/100001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/version_independent_window.mm (right): > > https://codereview.chromium.org/402443003/diff/100001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/version_independent_window.mm:117: > context:subviews.get()]; > On 2014/07/17 21:03:09, Scott Hess wrote: > > Wow. > > > > I'm assuming the comment refers to an attempt to just [superview > > addSubview:contentView positioned:NSWindowBelow relativeTo:nil]? > > > > Another possibility which sounds like it might reorder things more directly: > > scoped_nsobject<NSMutableArray> subviews([[superview subviews] > mutableCopy]); > > [subviews removeObject:contentView]; // assumes a ref is held elsewhere > > [subviews insertObject:contentView atIndex:0]; > > [superview setSubviews:subviews]; > > AFAICT it shouldn't call -addSubview: or -removeSubview: in this case, instead > > it will just reorder to match. > > I tried out your suggestion. It works fine and doesn't raise any warnings. > > That being said, given that Apple has cracked down on addSubview:, setting the > subviews property seems like the next obvious method to shut down, since that > allows addition/removal of views. In place sorting of subviews seems much less > likely to produce warnings from future releases of AppKit. SGTM. Though the fact that the sorting method takes a function rather than a block doesn't bode well for the future of this method, IMHO.
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikchen@chromium.org/402443003/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikchen@chromium.org/402443003/120001
Message was sent while issue was closed.
Change committed as 284544
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/402323002/ by ksakamoto@chromium.org. The reason for reverting is: Broke Mac ASan 64 Builder http://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Builde... obj/chrome/browser/ui/cocoa/browser_ui.version_independent_window.o ../../chrome/browser/ui/cocoa/version_independent_window.mm:109:44:error: cannot initialize a parameter of type 'NSComparisonResult (*)(id, id, void *)' with an rvalue of type 'int (*)(id, id, void *)': different return type ('NSComparisonResult' (aka 'long') vs 'int') [superview sortSubviewsUsingFunction:&ReorderContentViewToBack ^~~~~~~~~~~~~~~~~~~~~~~~~ /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSView.h:175:75: note: passing argument to parameter 'compare' here - (void)sortSubviewsUsingFunction:(NSComparisonResult (*)(id, id, void *))compare context:(void *)context; .
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/408103003/ by thakis@chromium.org. The reason for reverting is: Doesn't build on the mac asan builder ( http://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Builde... ): ../../chrome/browser/ui/cocoa/version_independent_window.mm:109:44: error: cannot initialize a parameter of type 'NSComparisonResult (*)(id, id, void *)' with an rvalue of type 'int (*)(id, id, void *)': different return type ('NSComparisonResult' (aka 'long') vs 'int') [superview sortSubviewsUsingFunction:&ReorderContentViewToBack ^~~~~~~~~~~~~~~~~~~~~~~~~ /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSView.h:175:75: note: passing argument to parameter 'compare' here - (void)sortSubviewsUsingFunction:(NSComparisonResult (*)(id, id, void *))compare context:(void *)context; ^. |
