Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2404783002:
[Mac] Avoid "adding unknown subview" warning. (Closed)
|
Created:
4 years, 2 months ago by Eugene But (OOO till 7-30) Modified:
4 years, 2 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Mac] Avoid "adding unknown subview" warning.
Chrome Mac has added a subview above the window's content view, which
the Appkit warns about at runtime. Presumably, doing so may break in a
future macOS release. This cl adds Chrome's top-level subview to the
window's content view, which is what the Appkit wants. The change is
done only for macOS 10.11 and later.
Using correct view hierarchy breaks a few things which had to be fixed:
1.) Window buttons are now placed in NSToolbarContainerView which height
is 22 points and to move buttons down the whole NSToolbarContainerView
should be moved down. NSToolbarContainerView is not movable via
NSLayoutConstraints and simply resized every time its bounds changed.
2.) Window buttons are not movable anymore by changing their frame.
Their position is controlled by layout constraints received from
autoresizing mask and buttons can be moved via NSLayoutConstraints.
3.) The hight of browserFrameViewPaintHeight should be the same as tab strip
height in order to paint content area correctly.
4.) NSVisualEffect view for titlebar should be hidden in fullscreen.
With old view hierarchy NSVisualEffectView was moved offscreen for some reason.
Now it is always shown on the titlebar.
5.) Window does not update tracking area which highlights window
buttons when those buttons are moved (rdar://28535344). As a workaround
addTrackingArea: method is swizzled to prevent unnecessarily adding
an extra tracking area. Swizzling is necessary because OS does not
provide any signals that tracking area was updated (
NSViewDidUpdateTrackingAreasNotification is called before adding a
tracking area, not after).
6.) Windows w/o tab strip should not use NSFullSizeContentViewWindowMask
style mask, otherwise they will show up without toolbar.
7.) Tab should not allow moving window when dragged. This is done by
overriding _opaqueRectForWindowMoveWhenInTitlebar (AppKit follows the
same approach for NSButton, to prevent it from moving the window).
BUG=605219
Committed: https://crrev.com/42c6683729d8e049fdcc89e552bdcff5fb337227
Cr-Commit-Position: refs/heads/master@{#424609}
Patch Set 1 #Patch Set 2 : Merged with head #
Total comments: 18
Patch Set 3 : Addressed review comments #Patch Set 4 : Added comments to the code that adds fullscreen enter/exit observer #
Total comments: 2
Patch Set 5 : Addressed final comments #
Total comments: 2
Patch Set 6 : Merged with head #
Total comments: 3
Messages
Total messages: 55 (28 generated)
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
eugenebut@chromium.org changed reviewers: + tapted@chromium.org
I'm waiting for a build to finish to have more of a play, but this looks pretty good https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:235: - (NSRect)_opaqueRectForWindowMoveWhenInTitlebar { nit: we typically forward-declare these - I guess it's only strictly necessary when calling [super ..] but it helps document where the private methods are coming from. E.g. in a `@interface NSView (PrivateAPI)` at the top of the file. (and we haven't started putting private APIs into sdk_forward_declarations, so I guess we should copy it in tab_view.mm as well, [or give it its own header..]). https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:236: return [self bounds]; Maybe comment here what the base class returns? (I'm guessing NSZeroRect?) -- i.e. say why [self bounds] is the right thing to convince AppKit not to move the window https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_window_controller.mm (right): https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:31: - (void)windowWillEnterFullScreenNotification:(NSNotification*)notification; nit: windowWillEnterFullScreen: (consistent with BrowserWindowController) https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:32: // Called when NSWindowWillExitFullScreenNotification notification received. nit: blank line before [consistency] https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:387: [rootView addSubview:tabStripBackgroundView_ before this, perhaps DCHECK(!chrome::ShouldUseFullSizeContentView()) https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:432: visualEffectView_.reset([visualEffectView retain]); since visualEffectView is also scoped_nsobject, we should just be able to assign here. Although I'd probably just go with visualEffectView_.reset([[foo alloc] initWith..]) up on line 399, and add an underscore to the stuff above https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:442: [[visualEffectView_ animator] setAlphaValue:0.0]; I wonder if there are implications for text rendering / supbixel AA if we change the opacity of this. E.g. do tab titles and the profile selector still look right. (although at a glance, the profile selector is doing something to force subpixel AA and it's getting the backgroudn colour wrong anyway) https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:446: [[visualEffectView_ animator] setAlphaValue:1.0]; Maybe worth a note for these - visualEffectView_ will be nil for popups (i.e. when hasTitleBar is false in insertTabStripBackgroundViewIntoWindow:) - the code works, but it could be a trap for the unwary later
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:235: - (NSRect)_opaqueRectForWindowMoveWhenInTitlebar { On 2016/10/10 23:38:06, tapted wrote: > nit: we typically forward-declare these - I guess it's only strictly necessary > when calling [super ..] but it helps document where the private methods are > coming from. E.g. in a `@interface NSView (PrivateAPI)` at the top of the file. > (and we haven't started putting private APIs into sdk_forward_declarations, so I > guess we should copy it in tab_view.mm as well, [or give it its own header..]). Done. https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:236: return [self bounds]; On 2016/10/10 23:38:06, tapted wrote: > Maybe comment here what the base class returns? (I'm guessing NSZeroRect?) -- > i.e. say why [self bounds] is the right thing to convince AppKit not to move the > window Done in predeclaration. https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_window_controller.mm (right): https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:31: - (void)windowWillEnterFullScreenNotification:(NSNotification*)notification; On 2016/10/10 23:38:06, tapted wrote: > nit: windowWillEnterFullScreen: (consistent with BrowserWindowController) |windowWillEnterFullScreen:| does not follow Coding Guidelines for Cocoa which say "Make the word before the argument describe the argument.": https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Co... Coding Guidelines for Cocoa is the basis for Google Objective-C Style Guide and following style guide naming seems to be more important than being consistent with another file (which does not follow naming rules strictly). WDYT? https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:32: // Called when NSWindowWillExitFullScreenNotification notification received. On 2016/10/10 23:38:06, tapted wrote: > nit: blank line before [consistency] Done. https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:387: [rootView addSubview:tabStripBackgroundView_ On 2016/10/10 23:38:06, tapted wrote: > before this, perhaps > DCHECK(!chrome::ShouldUseFullSizeContentView()) Are you sure? Availability of NSVisualEffectView and using it for toolbar effect seems to be unrelated to view hierarchy fix. Am I missing something? https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:432: visualEffectView_.reset([visualEffectView retain]); On 2016/10/10 23:38:06, tapted wrote: > since visualEffectView is also scoped_nsobject, we should just be able to assign > here. Although I'd probably just go with visualEffectView_.reset([[foo alloc] > initWith..]) up on line 399, and add an underscore to the stuff above Done. https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:442: [[visualEffectView_ animator] setAlphaValue:0.0]; On 2016/10/10 23:38:06, tapted wrote: > I wonder if there are implications for text rendering / supbixel AA if we change > the opacity of this. E.g. do tab titles and the profile selector still look > right. (although at a glance, the profile selector is doing something to force > subpixel AA and it's getting the backgroudn colour wrong anyway) Visually looks the same before and after the changes. https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:446: [[visualEffectView_ animator] setAlphaValue:1.0]; On 2016/10/10 23:38:06, tapted wrote: > Maybe worth a note for these - visualEffectView_ will be nil for popups (i.e. > when hasTitleBar is false in insertTabStripBackgroundViewIntoWindow:) - the code > works, but it could be a trap for the unwary later Done. Added comments to the header.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Added comments to tab_window_controller.mm to explain why notification center is used.
sweet! lgtm. https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_window_controller.mm (right): https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:387: [rootView addSubview:tabStripBackgroundView_ On 2016/10/11 00:33:10, Eugene But wrote: > On 2016/10/10 23:38:06, tapted wrote: > > before this, perhaps > > DCHECK(!chrome::ShouldUseFullSizeContentView()) > Are you sure? Availability of NSVisualEffectView and using it for toolbar effect > seems to be unrelated to view hierarchy fix. Am I missing something? The problem here is that |rootView| is [contentView superview], which we shouldn't be calling addSubview on. (ideally we'd move the rootView declaration into the `else` block below of `if (chrome::ShouldUseFullSizeContentView())` but it gets used here as well - so DCHECK feels like a nice compromise :) https://codereview.chromium.org/2404783002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_window_controller.h (right): https://codereview.chromium.org/2404783002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.h:30: // Used to blur the titlebar. nil if window does not have titlebar. nit: blank line before
Thanks! https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_window_controller.mm (right): https://codereview.chromium.org/2404783002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:387: [rootView addSubview:tabStripBackgroundView_ On 2016/10/11 04:21:43, tapted wrote: > On 2016/10/11 00:33:10, Eugene But wrote: > > On 2016/10/10 23:38:06, tapted wrote: > > > before this, perhaps > > > DCHECK(!chrome::ShouldUseFullSizeContentView()) > > Are you sure? Availability of NSVisualEffectView and using it for toolbar > effect > > seems to be unrelated to view hierarchy fix. Am I missing something? > > The problem here is that |rootView| is [contentView superview], which we > shouldn't be calling addSubview on. (ideally we'd move the rootView declaration > into the `else` block below of `if (chrome::ShouldUseFullSizeContentView())` but > it gets used here as well - so DCHECK feels like a nice compromise :) Oh, right. Done. https://codereview.chromium.org/2404783002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_window_controller.h (right): https://codereview.chromium.org/2404783002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.h:30: // Used to blur the titlebar. nil if window does not have titlebar. On 2016/10/11 04:21:43, tapted wrote: > nit: blank line before Done.
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
eugenebut@chromium.org changed reviewers: + thakis@chromium.org
+thakis for base/mac/sdk_forward_declarations.h
eugenebut@chromium.org changed reviewers: - thakis@chromium.org
-thakis (tapted has ownership for base/mac)
eugenebut@chromium.org changed reviewers: + erikchen@chromium.org, shrike@chromium.org
erikchen@, shrike@, do you want to take a look before I land this? Had to delete the old CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with a question. Thanks for looking into this! https://codereview.chromium.org/2404783002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_window_controller.mm (right): https://codereview.chromium.org/2404783002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:449: [[visualEffectView_ animator] setAlphaValue:0.0]; why is this necessary now but not before?
Took a quick look - lgtm. I defer to other reviewers for a more thorough evaluation.
Description was changed from ========== [Mac] Avoid "adding unknown subview" warning. Chrome Mac has added a subview above the window's content view, which the Appkit warns about at runtime. Presumably, doing so may break in a future macOS release. This cl adds Chrome's top-level subview to the window's content view, which is what the Appkit wants. The change is done only for macOS 10.11 and later. Using correct view hierarchy breaks a few things which had to be fixed: 1.) Window buttons are now placed in NSToolbarContainerView which height is 22 points and to move buttons down the whole NSToolbarContainerView should be moved down. NSToolbarContainerView is not movable via NSLayoutConstraints and simply resized every time its bounds changed. 2.) Window buttons are not movable anymore by changing their frame. Their position is controlled by layout constraints received from autoresizing mask and buttons can be moved via NSLayoutConstraints. 3.) The hight of browserFrameViewPaintHeight should be the same as tab strip height in order to paint content area correctly. 4.) NSVisualEffect view for titlebar should be hidden in fullscreen. 5.) Window does not update tracking area which highlights window buttons when those buttons are moved (rdar://28535344). As a workaround addTrackingArea: method is swizzled to prevent unnecessarily adding an extra tracking area. Swizzling is necessary because OS does not provide any signals that tracking area was updated ( NSViewDidUpdateTrackingAreasNotification is called before adding a tracking area, not after). 6.) Windows w/o tab strip should not use NSFullSizeContentViewWindowMask style mask, otherwise they will show up without toolbar. 7.) Tab should not allow moving window when dragged. This is done by overriding _opaqueRectForWindowMoveWhenInTitlebar (AppKit follows the same approach for NSButton, to prevent it from moving the window). BUG=605219 ========== to ========== [Mac] Avoid "adding unknown subview" warning. Chrome Mac has added a subview above the window's content view, which the Appkit warns about at runtime. Presumably, doing so may break in a future macOS release. This cl adds Chrome's top-level subview to the window's content view, which is what the Appkit wants. The change is done only for macOS 10.11 and later. Using correct view hierarchy breaks a few things which had to be fixed: 1.) Window buttons are now placed in NSToolbarContainerView which height is 22 points and to move buttons down the whole NSToolbarContainerView should be moved down. NSToolbarContainerView is not movable via NSLayoutConstraints and simply resized every time its bounds changed. 2.) Window buttons are not movable anymore by changing their frame. Their position is controlled by layout constraints received from autoresizing mask and buttons can be moved via NSLayoutConstraints. 3.) The hight of browserFrameViewPaintHeight should be the same as tab strip height in order to paint content area correctly. 4.) NSVisualEffect view for titlebar should be hidden in fullscreen. With old view hierarchy NSVisualEffectView was moved offscreen for some reason. Now it is always shown on the titlebar. 5.) Window does not update tracking area which highlights window buttons when those buttons are moved (rdar://28535344). As a workaround addTrackingArea: method is swizzled to prevent unnecessarily adding an extra tracking area. Swizzling is necessary because OS does not provide any signals that tracking area was updated ( NSViewDidUpdateTrackingAreasNotification is called before adding a tracking area, not after). 6.) Windows w/o tab strip should not use NSFullSizeContentViewWindowMask style mask, otherwise they will show up without toolbar. 7.) Tab should not allow moving window when dragged. This is done by overriding _opaqueRectForWindowMoveWhenInTitlebar (AppKit follows the same approach for NSButton, to prevent it from moving the window). BUG=605219 ==========
Thanks everyone! https://codereview.chromium.org/2404783002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_window_controller.mm (right): https://codereview.chromium.org/2404783002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:449: [[visualEffectView_ animator] setAlphaValue:0.0]; On 2016/10/11 17:39:45, erikchen wrote: > why is this necessary now but not before? With old view hierarchy NSVisualEffectVew was moved offscreen for some reason. Now it is always shown on the titlebar. I've tried simply hiding it, but this way the color change happens quickly and it is visible. Animated alpha change looks much smoother.
The CQ bit was checked by eugenebut@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2404783002/#ps80001 (title: "Addressed final comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by eugenebut@chromium.org
The CQ bit was checked by eugenebut@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, erikchen@chromium.org, shrike@chromium.org Link to the patchset: https://codereview.chromium.org/2404783002/#ps100001 (title: "Merged with head")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Mac] Avoid "adding unknown subview" warning. Chrome Mac has added a subview above the window's content view, which the Appkit warns about at runtime. Presumably, doing so may break in a future macOS release. This cl adds Chrome's top-level subview to the window's content view, which is what the Appkit wants. The change is done only for macOS 10.11 and later. Using correct view hierarchy breaks a few things which had to be fixed: 1.) Window buttons are now placed in NSToolbarContainerView which height is 22 points and to move buttons down the whole NSToolbarContainerView should be moved down. NSToolbarContainerView is not movable via NSLayoutConstraints and simply resized every time its bounds changed. 2.) Window buttons are not movable anymore by changing their frame. Their position is controlled by layout constraints received from autoresizing mask and buttons can be moved via NSLayoutConstraints. 3.) The hight of browserFrameViewPaintHeight should be the same as tab strip height in order to paint content area correctly. 4.) NSVisualEffect view for titlebar should be hidden in fullscreen. With old view hierarchy NSVisualEffectView was moved offscreen for some reason. Now it is always shown on the titlebar. 5.) Window does not update tracking area which highlights window buttons when those buttons are moved (rdar://28535344). As a workaround addTrackingArea: method is swizzled to prevent unnecessarily adding an extra tracking area. Swizzling is necessary because OS does not provide any signals that tracking area was updated ( NSViewDidUpdateTrackingAreasNotification is called before adding a tracking area, not after). 6.) Windows w/o tab strip should not use NSFullSizeContentViewWindowMask style mask, otherwise they will show up without toolbar. 7.) Tab should not allow moving window when dragged. This is done by overriding _opaqueRectForWindowMoveWhenInTitlebar (AppKit follows the same approach for NSButton, to prevent it from moving the window). BUG=605219 ========== to ========== [Mac] Avoid "adding unknown subview" warning. Chrome Mac has added a subview above the window's content view, which the Appkit warns about at runtime. Presumably, doing so may break in a future macOS release. This cl adds Chrome's top-level subview to the window's content view, which is what the Appkit wants. The change is done only for macOS 10.11 and later. Using correct view hierarchy breaks a few things which had to be fixed: 1.) Window buttons are now placed in NSToolbarContainerView which height is 22 points and to move buttons down the whole NSToolbarContainerView should be moved down. NSToolbarContainerView is not movable via NSLayoutConstraints and simply resized every time its bounds changed. 2.) Window buttons are not movable anymore by changing their frame. Their position is controlled by layout constraints received from autoresizing mask and buttons can be moved via NSLayoutConstraints. 3.) The hight of browserFrameViewPaintHeight should be the same as tab strip height in order to paint content area correctly. 4.) NSVisualEffect view for titlebar should be hidden in fullscreen. With old view hierarchy NSVisualEffectView was moved offscreen for some reason. Now it is always shown on the titlebar. 5.) Window does not update tracking area which highlights window buttons when those buttons are moved (rdar://28535344). As a workaround addTrackingArea: method is swizzled to prevent unnecessarily adding an extra tracking area. Swizzling is necessary because OS does not provide any signals that tracking area was updated ( NSViewDidUpdateTrackingAreasNotification is called before adding a tracking area, not after). 6.) Windows w/o tab strip should not use NSFullSizeContentViewWindowMask style mask, otherwise they will show up without toolbar. 7.) Tab should not allow moving window when dragged. This is done by overriding _opaqueRectForWindowMoveWhenInTitlebar (AppKit follows the same approach for NSButton, to prevent it from moving the window). BUG=605219 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Mac] Avoid "adding unknown subview" warning. Chrome Mac has added a subview above the window's content view, which the Appkit warns about at runtime. Presumably, doing so may break in a future macOS release. This cl adds Chrome's top-level subview to the window's content view, which is what the Appkit wants. The change is done only for macOS 10.11 and later. Using correct view hierarchy breaks a few things which had to be fixed: 1.) Window buttons are now placed in NSToolbarContainerView which height is 22 points and to move buttons down the whole NSToolbarContainerView should be moved down. NSToolbarContainerView is not movable via NSLayoutConstraints and simply resized every time its bounds changed. 2.) Window buttons are not movable anymore by changing their frame. Their position is controlled by layout constraints received from autoresizing mask and buttons can be moved via NSLayoutConstraints. 3.) The hight of browserFrameViewPaintHeight should be the same as tab strip height in order to paint content area correctly. 4.) NSVisualEffect view for titlebar should be hidden in fullscreen. With old view hierarchy NSVisualEffectView was moved offscreen for some reason. Now it is always shown on the titlebar. 5.) Window does not update tracking area which highlights window buttons when those buttons are moved (rdar://28535344). As a workaround addTrackingArea: method is swizzled to prevent unnecessarily adding an extra tracking area. Swizzling is necessary because OS does not provide any signals that tracking area was updated ( NSViewDidUpdateTrackingAreasNotification is called before adding a tracking area, not after). 6.) Windows w/o tab strip should not use NSFullSizeContentViewWindowMask style mask, otherwise they will show up without toolbar. 7.) Tab should not allow moving window when dragged. This is done by overriding _opaqueRectForWindowMoveWhenInTitlebar (AppKit follows the same approach for NSButton, to prevent it from moving the window). BUG=605219 ========== to ========== [Mac] Avoid "adding unknown subview" warning. Chrome Mac has added a subview above the window's content view, which the Appkit warns about at runtime. Presumably, doing so may break in a future macOS release. This cl adds Chrome's top-level subview to the window's content view, which is what the Appkit wants. The change is done only for macOS 10.11 and later. Using correct view hierarchy breaks a few things which had to be fixed: 1.) Window buttons are now placed in NSToolbarContainerView which height is 22 points and to move buttons down the whole NSToolbarContainerView should be moved down. NSToolbarContainerView is not movable via NSLayoutConstraints and simply resized every time its bounds changed. 2.) Window buttons are not movable anymore by changing their frame. Their position is controlled by layout constraints received from autoresizing mask and buttons can be moved via NSLayoutConstraints. 3.) The hight of browserFrameViewPaintHeight should be the same as tab strip height in order to paint content area correctly. 4.) NSVisualEffect view for titlebar should be hidden in fullscreen. With old view hierarchy NSVisualEffectView was moved offscreen for some reason. Now it is always shown on the titlebar. 5.) Window does not update tracking area which highlights window buttons when those buttons are moved (rdar://28535344). As a workaround addTrackingArea: method is swizzled to prevent unnecessarily adding an extra tracking area. Swizzling is necessary because OS does not provide any signals that tracking area was updated ( NSViewDidUpdateTrackingAreasNotification is called before adding a tracking area, not after). 6.) Windows w/o tab strip should not use NSFullSizeContentViewWindowMask style mask, otherwise they will show up without toolbar. 7.) Tab should not allow moving window when dragged. This is done by overriding _opaqueRectForWindowMoveWhenInTitlebar (AppKit follows the same approach for NSButton, to prevent it from moving the window). BUG=605219 Committed: https://crrev.com/42c6683729d8e049fdcc89e552bdcff5fb337227 Cr-Commit-Position: refs/heads/master@{#424609} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/42c6683729d8e049fdcc89e552bdcff5fb337227 Cr-Commit-Position: refs/heads/master@{#424609}
Message was sent while issue was closed.
yunchao.he@intel.com changed reviewers: + yunchao.he@intel.com
Message was sent while issue was closed.
latest Chromium failed to build by this CL. Could you take a look? https://codereview.chromium.org/2404783002/diff/100001/base/mac/sdk_forward_d... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/2404783002/diff/100001/base/mac/sdk_forward_d... base/mac/sdk_forward_declarations.h:270: @interface NSLayoutXAxisAnchor : NSObject Sorry to bother you, I failed to build chrome on MacOSX (both on Mac 10.11 and 10.12). The build error showed that there is duplicated definition for this class. It has been defined in MacOSX system lib. Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSLayoutAnchor.h:50:12: note: previous definition is here @interface NSLayoutXAxisAnchor : NSLayoutAnchor<NSLayoutXAxisAnchor*> Could you help to verify that Chrome can build on MacOSX 10.11/10.12? BTW, do we have MacOSX 10.11/10.12 buildbots before merge a patch?
Message was sent while issue was closed.
https://codereview.chromium.org/2404783002/diff/100001/base/mac/sdk_forward_d... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/2404783002/diff/100001/base/mac/sdk_forward_d... base/mac/sdk_forward_declarations.h:270: @interface NSLayoutXAxisAnchor : NSObject On 2016/10/12 04:18:56, yunchao wrote: > Sorry to bother you, I failed to build chrome on MacOSX (both on Mac 10.11 and > 10.12). The build error showed that there is duplicated definition for this > class. It has been defined in MacOSX system lib. > > Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSLayoutAnchor.h:50:12: > note: previous definition is here > @interface NSLayoutXAxisAnchor : NSLayoutAnchor<NSLayoutXAxisAnchor*> > > Could you help to verify that Chrome can build on MacOSX 10.11/10.12? I think this lines needs to be @interface NSLayoutXAxisAnchor (ElCapitanSDK) : NSObject does that fix the error for you? > BTW, do we have MacOSX 10.11/10.12 buildbots before merge a patch? We run tests on the OSes, but don't iterate all developer build configurations - Apple update them too regularly. Still, it's important to fix this, so if the above works for you I'll land a quick fix.
Message was sent while issue was closed.
Thanks for your quick response. please see the comment inline. https://codereview.chromium.org/2404783002/diff/100001/base/mac/sdk_forward_d... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/2404783002/diff/100001/base/mac/sdk_forward_d... base/mac/sdk_forward_declarations.h:270: @interface NSLayoutXAxisAnchor : NSObject On 2016/10/12 04:22:34, tapted wrote: > On 2016/10/12 04:18:56, yunchao wrote: > > Sorry to bother you, I failed to build chrome on MacOSX (both on Mac 10.11 and > > 10.12). The build error showed that there is duplicated definition for this > > class. It has been defined in MacOSX system lib. > > > > > Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSLayoutAnchor.h:50:12: > > note: previous definition is here > > @interface NSLayoutXAxisAnchor : NSLayoutAnchor<NSLayoutXAxisAnchor*> > > > > Could you help to verify that Chrome can build on MacOSX 10.11/10.12? > > > I think this lines needs to be > > @interface NSLayoutXAxisAnchor (ElCapitanSDK) : NSObject > > does that fix the error for you? > > > > BTW, do we have MacOSX 10.11/10.12 buildbots before merge a patch? > > We run tests on the OSes, but don't iterate all developer build configurations - > Apple update them too regularly. Still, it's important to fix this, so if the > above works for you I'll land a quick fix. Still not work. But the error is "expected unqualified-id" now, the error at the ":"
Message was sent while issue was closed.
On 2016/10/12 04:30:37, yunchao wrote: > Thanks for your quick response. please see the comment inline. > > https://codereview.chromium.org/2404783002/diff/100001/base/mac/sdk_forward_d... > File base/mac/sdk_forward_declarations.h (right): > > https://codereview.chromium.org/2404783002/diff/100001/base/mac/sdk_forward_d... > base/mac/sdk_forward_declarations.h:270: @interface NSLayoutXAxisAnchor : > NSObject > On 2016/10/12 04:22:34, tapted wrote: > > On 2016/10/12 04:18:56, yunchao wrote: > > > Sorry to bother you, I failed to build chrome on MacOSX (both on Mac 10.11 > and > > > 10.12). The build error showed that there is duplicated definition for this > > > class. It has been defined in MacOSX system lib. > > > > > > > > > Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSLayoutAnchor.h:50:12: > > > note: previous definition is here > > > @interface NSLayoutXAxisAnchor : NSLayoutAnchor<NSLayoutXAxisAnchor*> > > > > > > Could you help to verify that Chrome can build on MacOSX 10.11/10.12? > > > > > > I think this lines needs to be > > > > @interface NSLayoutXAxisAnchor (ElCapitanSDK) : NSObject > > > > does that fix the error for you? > > > > > > > BTW, do we have MacOSX 10.11/10.12 buildbots before merge a patch? > > > > We run tests on the OSes, but don't iterate all developer build configurations > - > > Apple update them too regularly. Still, it's important to fix this, so if the > > above works for you I'll land a quick fix. > > Still not work. But the error is "expected unqualified-id" now, the error at the > ":" gah - sorry. Just @interface NSLayoutXAxisAnchor (ElCapitanSDK) should do the trick. I'm fetching HEAD now to test it out
Message was sent while issue was closed.
On 2016/10/12 04:30:37, yunchao wrote: > Thanks for your quick response. please see the comment inline. > > https://codereview.chromium.org/2404783002/diff/100001/base/mac/sdk_forward_d... > File base/mac/sdk_forward_declarations.h (right): > > https://codereview.chromium.org/2404783002/diff/100001/base/mac/sdk_forward_d... > base/mac/sdk_forward_declarations.h:270: @interface NSLayoutXAxisAnchor : > NSObject > On 2016/10/12 04:22:34, tapted wrote: > > On 2016/10/12 04:18:56, yunchao wrote: > > > Sorry to bother you, I failed to build chrome on MacOSX (both on Mac 10.11 > and > > > 10.12). The build error showed that there is duplicated definition for this > > > class. It has been defined in MacOSX system lib. > > > > > > > > > Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSLayoutAnchor.h:50:12: > > > note: previous definition is here > > > @interface NSLayoutXAxisAnchor : NSLayoutAnchor<NSLayoutXAxisAnchor*> > > > > > > Could you help to verify that Chrome can build on MacOSX 10.11/10.12? > > > > > > I think this lines needs to be > > > > @interface NSLayoutXAxisAnchor (ElCapitanSDK) : NSObject > > > > does that fix the error for you? > > > > > > > BTW, do we have MacOSX 10.11/10.12 buildbots before merge a patch? > > > > We run tests on the OSes, but don't iterate all developer build configurations > - > > Apple update them too regularly. Still, it's important to fix this, so if the > > above works for you I'll land a quick fix. > > Still not work. But the error is "expected unqualified-id" now, the error at the > ":" gah - sorry. Just @interface NSLayoutXAxisAnchor (ElCapitanSDK) should do the trick. I'm fetching HEAD now to test it out
Message was sent while issue was closed.
On 2016/10/12 04:32:05, tapted wrote: > On 2016/10/12 04:30:37, yunchao wrote: > > Thanks for your quick response. please see the comment inline. > > > > > https://codereview.chromium.org/2404783002/diff/100001/base/mac/sdk_forward_d... > > File base/mac/sdk_forward_declarations.h (right): > > > > > https://codereview.chromium.org/2404783002/diff/100001/base/mac/sdk_forward_d... > > base/mac/sdk_forward_declarations.h:270: @interface NSLayoutXAxisAnchor : > > NSObject > > On 2016/10/12 04:22:34, tapted wrote: > > > On 2016/10/12 04:18:56, yunchao wrote: > > > > Sorry to bother you, I failed to build chrome on MacOSX (both on Mac 10.11 > > and > > > > 10.12). The build error showed that there is duplicated definition for > this > > > > class. It has been defined in MacOSX system lib. > > > > > > > > > > > > > > Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSLayoutAnchor.h:50:12: > > > > note: previous definition is here > > > > @interface NSLayoutXAxisAnchor : NSLayoutAnchor<NSLayoutXAxisAnchor*> > > > > > > > > Could you help to verify that Chrome can build on MacOSX 10.11/10.12? > > > > > > > > > I think this lines needs to be > > > > > > @interface NSLayoutXAxisAnchor (ElCapitanSDK) : NSObject > > > > > > does that fix the error for you? > > > > > > > > > > BTW, do we have MacOSX 10.11/10.12 buildbots before merge a patch? > > > > > > We run tests on the OSes, but don't iterate all developer build > configurations > > - > > > Apple update them too regularly. Still, it's important to fix this, so if > the > > > above works for you I'll land a quick fix. > > > > Still not work. But the error is "expected unqualified-id" now, the error at > the > > ":" > > gah - sorry. Just > > @interface NSLayoutXAxisAnchor (ElCapitanSDK) > > should do the trick. I'm fetching HEAD now to test it out https://codereview.chromium.org/2416513002/ is in the CQ to fix this.
Message was sent while issue was closed.
On 2016/10/12 04:32:05, tapted wrote: > On 2016/10/12 04:30:37, yunchao wrote: > > Thanks for your quick response. please see the comment inline. > > > > > https://codereview.chromium.org/2404783002/diff/100001/base/mac/sdk_forward_d... > > File base/mac/sdk_forward_declarations.h (right): > > > > > https://codereview.chromium.org/2404783002/diff/100001/base/mac/sdk_forward_d... > > base/mac/sdk_forward_declarations.h:270: @interface NSLayoutXAxisAnchor : > > NSObject > > On 2016/10/12 04:22:34, tapted wrote: > > > On 2016/10/12 04:18:56, yunchao wrote: > > > > Sorry to bother you, I failed to build chrome on MacOSX (both on Mac 10.11 > > and > > > > 10.12). The build error showed that there is duplicated definition for > this > > > > class. It has been defined in MacOSX system lib. > > > > > > > > > > > > > > Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSLayoutAnchor.h:50:12: > > > > note: previous definition is here > > > > @interface NSLayoutXAxisAnchor : NSLayoutAnchor<NSLayoutXAxisAnchor*> > > > > > > > > Could you help to verify that Chrome can build on MacOSX 10.11/10.12? > > > > > > > > > I think this lines needs to be > > > > > > @interface NSLayoutXAxisAnchor (ElCapitanSDK) : NSObject > > > > > > does that fix the error for you? > > > > > > > > > > BTW, do we have MacOSX 10.11/10.12 buildbots before merge a patch? > > > > > > We run tests on the OSes, but don't iterate all developer build > configurations > > - > > > Apple update them too regularly. Still, it's important to fix this, so if > the > > > above works for you I'll land a quick fix. > > > > Still not work. But the error is "expected unqualified-id" now, the error at > the > > ":" > > gah - sorry. Just > > @interface NSLayoutXAxisAnchor (ElCapitanSDK) > > should do the trick. I'm fetching HEAD now to test it out Sorry, haven't try building this on 10.12.
Message was sent while issue was closed.
On 2016/10/12 04:32:05, tapted wrote: > On 2016/10/12 04:30:37, yunchao wrote: > > Thanks for your quick response. please see the comment inline. > > > > > https://codereview.chromium.org/2404783002/diff/100001/base/mac/sdk_forward_d... > > File base/mac/sdk_forward_declarations.h (right): > > > > > https://codereview.chromium.org/2404783002/diff/100001/base/mac/sdk_forward_d... > > base/mac/sdk_forward_declarations.h:270: @interface NSLayoutXAxisAnchor : > > NSObject > > On 2016/10/12 04:22:34, tapted wrote: > > > On 2016/10/12 04:18:56, yunchao wrote: > > > > Sorry to bother you, I failed to build chrome on MacOSX (both on Mac 10.11 > > and > > > > 10.12). The build error showed that there is duplicated definition for > this > > > > class. It has been defined in MacOSX system lib. > > > > > > > > > > > > > > Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSLayoutAnchor.h:50:12: > > > > note: previous definition is here > > > > @interface NSLayoutXAxisAnchor : NSLayoutAnchor<NSLayoutXAxisAnchor*> > > > > > > > > Could you help to verify that Chrome can build on MacOSX 10.11/10.12? > > > > > > > > > I think this lines needs to be > > > > > > @interface NSLayoutXAxisAnchor (ElCapitanSDK) : NSObject > > > > > > does that fix the error for you? > > > > > > > > > > BTW, do we have MacOSX 10.11/10.12 buildbots before merge a patch? > > > > > > We run tests on the OSes, but don't iterate all developer build > configurations > > - > > > Apple update them too regularly. Still, it's important to fix this, so if > the > > > above works for you I'll land a quick fix. > > > > Still not work. But the error is "expected unqualified-id" now, the error at > the > > ":" > > gah - sorry. Just > > @interface NSLayoutXAxisAnchor (ElCapitanSDK) > > should do the trick. I'm fetching HEAD now to test it out It works on MacOSX 10.11 now. What about for 10.12?
Message was sent while issue was closed.
On 2016/10/12 04:39:56, yunchao wrote: > On 2016/10/12 04:32:05, tapted wrote: > > On 2016/10/12 04:30:37, yunchao wrote: > > > Thanks for your quick response. please see the comment inline. > > > > > > > > > https://codereview.chromium.org/2404783002/diff/100001/base/mac/sdk_forward_d... > > > File base/mac/sdk_forward_declarations.h (right): > > > > > > > > > https://codereview.chromium.org/2404783002/diff/100001/base/mac/sdk_forward_d... > > > base/mac/sdk_forward_declarations.h:270: @interface NSLayoutXAxisAnchor : > > > NSObject > > > On 2016/10/12 04:22:34, tapted wrote: > > > > On 2016/10/12 04:18:56, yunchao wrote: > > > > > Sorry to bother you, I failed to build chrome on MacOSX (both on Mac > 10.11 > > > and > > > > > 10.12). The build error showed that there is duplicated definition for > > this > > > > > class. It has been defined in MacOSX system lib. > > > > > > > > > > > > > > > > > > > > Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSLayoutAnchor.h:50:12: > > > > > note: previous definition is here > > > > > @interface NSLayoutXAxisAnchor : NSLayoutAnchor<NSLayoutXAxisAnchor*> > > > > > > > > > > Could you help to verify that Chrome can build on MacOSX 10.11/10.12? > > > > > > > > > > > > I think this lines needs to be > > > > > > > > @interface NSLayoutXAxisAnchor (ElCapitanSDK) : NSObject > > > > > > > > does that fix the error for you? > > > > > > > > > > > > > BTW, do we have MacOSX 10.11/10.12 buildbots before merge a patch? > > > > > > > > We run tests on the OSes, but don't iterate all developer build > > configurations > > > - > > > > Apple update them too regularly. Still, it's important to fix this, so if > > the > > > > above works for you I'll land a quick fix. > > > > > > Still not work. But the error is "expected unqualified-id" now, the error at > > the > > > ":" > > > > gah - sorry. Just > > > > @interface NSLayoutXAxisAnchor (ElCapitanSDK) > > > > should do the trick. I'm fetching HEAD now to test it out > > It works on MacOSX 10.11 now. What about for 10.12? What kind of error do you get on 10.12?
Message was sent while issue was closed.
On 2016/10/12 04:43:50, Eugene But wrote: > On 2016/10/12 04:39:56, yunchao wrote: > > On 2016/10/12 04:32:05, tapted wrote: > > > On 2016/10/12 04:30:37, yunchao wrote: > > > > Thanks for your quick response. please see the comment inline. > > > > > > > > > > > > > > https://codereview.chromium.org/2404783002/diff/100001/base/mac/sdk_forward_d... > > > > File base/mac/sdk_forward_declarations.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2404783002/diff/100001/base/mac/sdk_forward_d... > > > > base/mac/sdk_forward_declarations.h:270: @interface NSLayoutXAxisAnchor : > > > > NSObject > > > > On 2016/10/12 04:22:34, tapted wrote: > > > > > On 2016/10/12 04:18:56, yunchao wrote: > > > > > > Sorry to bother you, I failed to build chrome on MacOSX (both on Mac > > 10.11 > > > > and > > > > > > 10.12). The build error showed that there is duplicated definition for > > > this > > > > > > class. It has been defined in MacOSX system lib. > > > > > > > > > > > > > > > > > > > > > > > > > > > Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSLayoutAnchor.h:50:12: > > > > > > note: previous definition is here > > > > > > @interface NSLayoutXAxisAnchor : NSLayoutAnchor<NSLayoutXAxisAnchor*> > > > > > > > > > > > > Could you help to verify that Chrome can build on MacOSX 10.11/10.12? > > > > > > > > > > > > > > > I think this lines needs to be > > > > > > > > > > @interface NSLayoutXAxisAnchor (ElCapitanSDK) : NSObject > > > > > > > > > > does that fix the error for you? > > > > > > > > > > > > > > > > BTW, do we have MacOSX 10.11/10.12 buildbots before merge a patch? > > > > > > > > > > We run tests on the OSes, but don't iterate all developer build > > > configurations > > > > - > > > > > Apple update them too regularly. Still, it's important to fix this, so > if > > > the > > > > > above works for you I'll land a quick fix. > > > > > > > > Still not work. But the error is "expected unqualified-id" now, the error > at > > > the > > > > ":" > > > > > > gah - sorry. Just > > > > > > @interface NSLayoutXAxisAnchor (ElCapitanSDK) > > > > > > should do the trick. I'm fetching HEAD now to test it out > > > > It works on MacOSX 10.11 now. What about for 10.12? > What kind of error do you get on 10.12? Sorry, the fix works on 10.12 too. |
