Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2519563002:
[Mac] Switch from NSObject categories to forward declarations for Touch Bar support. (Closed)
|
Created:
4 years, 1 month ago by Sidney San Martín Modified:
4 years ago Reviewers:
tapted CC:
chromium-reviews, mac-reviews_chromium.org, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Mac] Switch from NSObject categories to forward declarations for Touch Bar support.
This fixes a crash when -[BridgedContentView touchBar:] calls
-[NSGroupTouchBarItem groupItemWithIdentifier:items:], because
NSGroupTouchBarItem was typedef'd to NSObject.
BUG=666893
Committed: https://crrev.com/799f1635ddedf7fd2f69f7dca26b0f2bf6399cba
Cr-Commit-Position: refs/heads/master@{#433558}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Use availability macros, no longer need NSClassFromString #Patch Set 3 : Bring back NSClassFromString, keep the availability macros. #Patch Set 4 : Tweak comment, change `retain` back to `strong` in interfaces from Apple headers. #
Messages
Total messages: 40 (28 generated)
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
sdy@chromium.org changed reviewers: + tapted@chromium.org
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.
Description was changed from ========== [Mac] Switch from NSObject categories to forward declarations for Touch Bar support. This fixes a crash when -[BridgedContentView touchBar:] calls -[NSObject groupItemWithIdentifier:items:], because NSGroupTouchBarItem was typedef'd to NSObject. BUG=666893 ========== to ========== [Mac] Switch from NSObject categories to forward declarations for Touch Bar support. This fixes a crash when -[BridgedContentView touchBar:] calls -[NSGroupTouchBarItem groupItemWithIdentifier:items:], because NSGroupTouchBarItem was typedef'd to NSObject. BUG=666893 ==========
Ooh nice - lgtm - thanks! Somehow I'd always assumed this would lead to linker errors :/. https://codereview.chromium.org/2519563002/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view_touch_bar.mm (right): https://codereview.chromium.org/2519563002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view_touch_bar.mm:64: static Class NSGroupTouchBarItem = nit: no static (although the ban on non-POD static doesn't apply, this will still take up space in the data segment)
The CQ bit was checked by sdy@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 sdy@chromium.org
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by sdy@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 sdy@chromium.org
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by sdy@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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sdy@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.
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
The CQ bit was unchecked by sdy@chromium.org
The CQ bit was checked by sdy@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/2519563002/#ps100001 (title: "Tweak comment, change `retain` back to `strong` in interfaces from Apple headers.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1479743594324520,
"parent_rev": "368f507b14e1bbaaaf49575a5f1e1a0d22e0da2d", "commit_rev":
"362c740f8c9c53b2919d9b8fdcaa0d3fb5fbb7e8"}
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
On 2016/11/19 at 07:27:38, tapted wrote: > Ooh nice - lgtm - thanks! Somehow I'd always assumed this would lead to linker errors :/. Sure! As far as I know, linker errors show up when you refer to a class directly (which NSClassFromString works around). Interestingly, it sounds like the NS_CLASS_AVAILABLE macros imply weak linking if your deployment target is earlier, so once we build against the 10.12.1 SDK we shouldn't need NSClassFromString anymore. I didn't realize that before, so I uploaded a new patch to to re-add the availability macros. I also added them to the touch bar methods in bridged_content_view_touch_bar.mm, so those methods are now behind the same partial availability "shield" and are allowed to use 10.12.1 methods without redeclaring them. we should be able to get rid of the forward decls completely at that point because our own On 2016/11/19 at 07:27:38, tapted wrote: > https://codereview.chromium.org/2519563002/diff/1/ui/views/cocoa/bridged_cont... > File ui/views/cocoa/bridged_content_view_touch_bar.mm (right): > > https://codereview.chromium.org/2519563002/diff/1/ui/views/cocoa/bridged_cont... > ui/views/cocoa/bridged_content_view_touch_bar.mm:64: static Class NSGroupTouchBarItem = > nit: no static (although the ban on non-POD static doesn't apply, this will still take up space in the data segment) I got rid of this, but just out of curiosity, are statics like this discouraged? I didn't know that. My goal was just to do the NSClassFromString(…) lookup once. Thanks for the review!
Message was sent while issue was closed.
Description was changed from ========== [Mac] Switch from NSObject categories to forward declarations for Touch Bar support. This fixes a crash when -[BridgedContentView touchBar:] calls -[NSGroupTouchBarItem groupItemWithIdentifier:items:], because NSGroupTouchBarItem was typedef'd to NSObject. BUG=666893 ========== to ========== [Mac] Switch from NSObject categories to forward declarations for Touch Bar support. This fixes a crash when -[BridgedContentView touchBar:] calls -[NSGroupTouchBarItem groupItemWithIdentifier:items:], because NSGroupTouchBarItem was typedef'd to NSObject. BUG=666893 Committed: https://crrev.com/799f1635ddedf7fd2f69f7dca26b0f2bf6399cba Cr-Commit-Position: refs/heads/master@{#433558} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/799f1635ddedf7fd2f69f7dca26b0f2bf6399cba Cr-Commit-Position: refs/heads/master@{#433558}
Message was sent while issue was closed.
On 2016/11/21 at 16:34:18, Sidney San Martín wrote: > we should be able to get rid of the forward decls completely at that point because our own What I meant to say here was, "because touch bar APIs are only used by methods marked NS_AVAILABLE_MAC(10_12_1), touch_bar_forward_declarations.h can be dropped completely once we're on the 10.12.1 SDK".
Message was sent while issue was closed.
> On 2016/11/19 at 07:27:38, tapted wrote: > > > https://codereview.chromium.org/2519563002/diff/1/ui/views/cocoa/bridged_cont... > > File ui/views/cocoa/bridged_content_view_touch_bar.mm (right): > > > > > https://codereview.chromium.org/2519563002/diff/1/ui/views/cocoa/bridged_cont... > > ui/views/cocoa/bridged_content_view_touch_bar.mm:64: static Class > NSGroupTouchBarItem = > > nit: no static (although the ban on non-POD static doesn't apply, this will > still take up space in the data segment) > > I got rid of this, but just out of curiosity, are statics like this discouraged? > I didn't know that. My goal was just to do the NSClassFromString(…) lookup once. Well, there's no question for non-POD statics (go/cppguide#Static_and_Global_Variables clearly says they're banned). For other cases, if the initializer is a compile-time constant, then it's often encouraged. However, if the initializer is not not a compile-time constant, then it needs to be assessed whether `static` is appropriate. Sometimes it can cause subtle bugs (http://crrev.com/2507363003 is one example). Sometimes there are unexpected overheads (we use -fno-threadsafe-statics to avoid some, but the C++11 standard requires that the compiler inserts a mutex lock around static initializers). But it will always require a chunk of memory in every data segment and, unlike text segments, data segments can get fragmented easily - it may look like a pointer in size, but in weird cases it could cause an entire ~4k page of memory to get copied when it gets initialized. But also all these pointers eventually add up and increase Chrome's memory requirements. Since this counts as a performance optimization, we have to weigh up the potential downsides. Since this isn't "hot" code, we generally don't optimize for performance. https://codereview.chromium.org/2519563002/diff/1/ui/base/cocoa/touch_bar_for... File ui/base/cocoa/touch_bar_forward_declarations.h (right): https://codereview.chromium.org/2519563002/diff/1/ui/base/cocoa/touch_bar_for... ui/base/cocoa/touch_bar_forward_declarations.h:102: // declarations to suppress the partial availability warnings. I think I'm ok with it for now but, if I understand correctly, I think it's possible we will need to bring these back at some point. I don't think there's a precedent for putting `*_AVAILABLE_MAC` macros through our own code. In fact `git grep` doesn't show up any `*_AVAILABLE_MAC` in our codebase. It's possible that it makes sense for touchbar stuff -- if we can isolate our calls to touchbar APIs only within methods that are also declared NS_AVAILABLE_MAC, then I think that is a good design, and we should proceed with that approach. However, it might not always be possible to invoke touchbar APIs only in methods declared NS_AVAILABLE_MAC -- we might want to just have it between `if (base::mac::IsAtLeastOS10_Foo())` in a method that can't be declared NS_AVAILABLE_MAC.
Message was sent while issue was closed.
https://codereview.chromium.org/2519563002/diff/1/ui/base/cocoa/touch_bar_for... File ui/base/cocoa/touch_bar_forward_declarations.h (right): https://codereview.chromium.org/2519563002/diff/1/ui/base/cocoa/touch_bar_for... ui/base/cocoa/touch_bar_forward_declarations.h:102: // declarations to suppress the partial availability warnings. On 2016/11/21 22:32:25, tapted wrote: > I think I'm ok with it for now but, if I understand correctly, I think it's > possible we will need to bring these back at some point. > > I don't think there's a precedent for putting `*_AVAILABLE_MAC` macros through > our own code. In fact `git grep` doesn't show up any `*_AVAILABLE_MAC` in our > codebase. > > It's possible that it makes sense for touchbar stuff -- if we can isolate our > calls to touchbar APIs only within methods that are also declared > NS_AVAILABLE_MAC, then I think that is a good design, and we should proceed with > that approach. > > However, it might not always be possible to invoke touchbar APIs only in methods > declared NS_AVAILABLE_MAC -- we might want to just have it between `if > (base::mac::IsAtLeastOS10_Foo())` in a method that can't be declared > NS_AVAILABLE_MAC. Yeah I think removing these broke compilation against the 10.12.1 SDK. Googlers are probably insulated from it because of the toolchain updates. See https://echelog.com/logs/browse/chromium/1479855600
Message was sent while issue was closed.
On 2016/11/23 04:54:22, tapted wrote: > https://codereview.chromium.org/2519563002/diff/1/ui/base/cocoa/touch_bar_for... > File ui/base/cocoa/touch_bar_forward_declarations.h (right): > > https://codereview.chromium.org/2519563002/diff/1/ui/base/cocoa/touch_bar_for... > ui/base/cocoa/touch_bar_forward_declarations.h:102: // declarations to suppress > the partial availability warnings. > On 2016/11/21 22:32:25, tapted wrote: > > I think I'm ok with it for now but, if I understand correctly, I think it's > > possible we will need to bring these back at some point. > > > > I don't think there's a precedent for putting `*_AVAILABLE_MAC` macros through > > our own code. In fact `git grep` doesn't show up any `*_AVAILABLE_MAC` in our > > codebase. > > > > It's possible that it makes sense for touchbar stuff -- if we can isolate our > > calls to touchbar APIs only within methods that are also declared > > NS_AVAILABLE_MAC, then I think that is a good design, and we should proceed > with > > that approach. > > > > However, it might not always be possible to invoke touchbar APIs only in > methods > > declared NS_AVAILABLE_MAC -- we might want to just have it between `if > > (base::mac::IsAtLeastOS10_Foo())` in a method that can't be declared > > NS_AVAILABLE_MAC. > > Yeah I think removing these broke compilation against the 10.12.1 SDK. Googlers > are probably insulated from it because of the toolchain updates. See > https://echelog.com/logs/browse/chromium/1479855600 My pal on IRC confirmed that https://codereview.chromium.org/2515253007 will fix the build errors he was getting, but there are reports of more errors for official builders in http://crbug.com/667830, so I don't want to layer things on top of this commit. I think we should get rid of all the macros. Because of these issues and since the revised patchset didn't go through a proper review, I'm going to revert this.
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:100001) has been created in https://codereview.chromium.org/2526723002/ by tapted@chromium.org. The reason for reverting is: Persistent build failures on official.desktop. Breaks compilation with the 10.12.1 SDK. Also tapted doesn't like the macros..
Message was sent while issue was closed.
On 2016/11/23 at 05:10:32, tapted wrote: > On 2016/11/23 04:54:22, tapted wrote: > > https://codereview.chromium.org/2519563002/diff/1/ui/base/cocoa/touch_bar_for... > > File ui/base/cocoa/touch_bar_forward_declarations.h (right): > > > > https://codereview.chromium.org/2519563002/diff/1/ui/base/cocoa/touch_bar_for... > > ui/base/cocoa/touch_bar_forward_declarations.h:102: // declarations to suppress > > the partial availability warnings. > > On 2016/11/21 22:32:25, tapted wrote: > > > I think I'm ok with it for now but, if I understand correctly, I think it's > > > possible we will need to bring these back at some point. > > > > > > I don't think there's a precedent for putting `*_AVAILABLE_MAC` macros through > > > our own code. In fact `git grep` doesn't show up any `*_AVAILABLE_MAC` in our > > > codebase. > > > > > > It's possible that it makes sense for touchbar stuff -- if we can isolate our > > > calls to touchbar APIs only within methods that are also declared > > > NS_AVAILABLE_MAC, then I think that is a good design, and we should proceed > > with > > > that approach. > > > > > > However, it might not always be possible to invoke touchbar APIs only in > > methods > > > declared NS_AVAILABLE_MAC -- we might want to just have it between `if > > > (base::mac::IsAtLeastOS10_Foo())` in a method that can't be declared > > > NS_AVAILABLE_MAC. > > > > Yeah I think removing these broke compilation against the 10.12.1 SDK. Googlers > > are probably insulated from it because of the toolchain updates. See > > https://echelog.com/logs/browse/chromium/1479855600 > > My pal on IRC confirmed that https://codereview.chromium.org/2515253007 will fix the build errors he was getting, but there are reports of more errors for official builders in http://crbug.com/667830, so I don't want to layer things on top of this commit. I think we should get rid of all the macros. Because of these issues and since the revised patchset didn't go through a proper review, I'm going to revert this. Thanks. I should have reverted it immediately, *and* I should have given you a chance to review the revised CL instead of landing it — sorry for rushing ahead. I'll upload a fresh CL without the macros. On 2016/11/21 at 22:32:25, tapted wrote: > Well, there's no question for non-POD statics (go/cppguide#Static_and_Global_Variables clearly says they're banned). > > For other cases, if the initializer is a compile-time constant, then it's often encouraged. > > However, if the initializer is not not a compile-time constant, then it needs to be assessed whether `static` is appropriate. Sometimes it can cause subtle bugs (http://crrev.com/2507363003 is one example). Sometimes there are unexpected overheads (we use -fno-threadsafe-statics to avoid some, but the C++11 standard requires that the compiler inserts a mutex lock around static initializers). But it will always require a chunk of memory in every data segment and, unlike text segments, data segments can get fragmented easily - it may look like a pointer in size, but in weird cases it could cause an entire ~4k page of memory to get copied when it gets initialized. But also all these pointers eventually add up and increase Chrome's memory requirements. > > Since this counts as a performance optimization, we have to weigh up the potential downsides. Since this isn't "hot" code, we generally don't optimize for performance. Thanks for the detailed explanation. The time/memory tradeoff for cold code makes sense, and I had no idea about the fragmentation and implicit locks. On 2016/11/21 at 22:32:25, tapted wrote: > I think I'm ok with it for now but, if I understand correctly, I think it's possible we will need to bring these back at some point. > > I don't think there's a precedent for putting `*_AVAILABLE_MAC` macros through our own code. In fact `git grep` doesn't show up any `*_AVAILABLE_MAC` in our codebase. > > It's possible that it makes sense for touchbar stuff -- if we can isolate our calls to touchbar APIs only within methods that are also declared NS_AVAILABLE_MAC, then I think that is a good design, and we should proceed with that approach. > > However, it might not always be possible to invoke touchbar APIs only in methods declared NS_AVAILABLE_MAC -- we might want to just have it between `if (base::mac::IsAtLeastOS10_Foo())` in a method that can't be declared NS_AVAILABLE_MAC. I was wrong about how availability annotations work :/. Partially available functions *aren't* allowed to use partially available types, it just seemed like they could because the Mac SDK headers use @class liberally, which is enough to make -Wpartial-availability ignore it. It turns out that NSTouchBar is forward declared in NSTouchBarItem.h, but NSTouchBarItem never is. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
