Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2543703002:
[Mac] Switch from NSObject categories to forward declarations for Touch Bar support. (Closed)
|
Created:
4 years ago by Sidney San Martín Modified:
4 years ago Reviewers:
tapted CC:
chromium-reviews, tfarina, mac-reviews_chromium.org 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.
This is an improved version of
https://crrev.com/799f1635ddedf7fd2f69f7dca26b0f2bf6399cba
BUG=666893
Committed: https://crrev.com/3a6a6c13fe021aa3124ecaadf4655aa76ce47a99
Cr-Commit-Position: refs/heads/master@{#435477}
Patch Set 1 #
Total comments: 3
Messages
Total messages: 16 (10 generated)
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...
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. This is an improved version of https://crrev.com/799f1635ddedf7fd2f69f7dca26b0f2bf6399cba BUG=666893 ==========
sdy@chromium.org changed reviewers: + tapted@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Tested this against our hermetic toolchain and the 10.12 SDK.
lgtm - Thanks! A couple of concerns but maybe we just need to start smoking out these older bots.. hopefully they are all happy with this https://codereview.chromium.org/2543703002/diff/1/ui/base/cocoa/touch_bar_for... File ui/base/cocoa/touch_bar_forward_declarations.h (right): https://codereview.chromium.org/2543703002/diff/1/ui/base/cocoa/touch_bar_for... ui/base/cocoa/touch_bar_forward_declarations.h:14: #pragma clang assume_nonnull begin I think this is OK since we're replicating a system header and want to reduce the likelihood that we let stuff through the CQ that will break when compiling against the authentic 10.12.1 SDK (but if you're curious there's some active discussion around whether we should use audited regions like this for new code at http://go/uzjld ) https://codereview.chromium.org/2543703002/diff/1/ui/base/cocoa/touch_bar_for... ui/base/cocoa/touch_bar_forward_declarations.h:29: - (instancetype)init NS_DESIGNATED_INITIALIZER; Internet suggests NS_DESIGNATED_INITIALIZER is XCode 6, but I'm not sure on the SDK. https://www.chromium.org/developers/design-documents/mac-xib-files suggests we still use XCode 5 in some places. But I _think_ this is OK now. NS_DESIGNATED_INITIALIZER is used a lot on a lot of ios code, but image_capture_device_manager_unittest.mm is the only occurence I could find for MacOS code. (and it looks like official builders are happy with that). My 10.10 SDK's NSObjCRuntime.h has this defined in any case. https://codereview.chromium.org/2543703002/diff/1/ui/base/cocoa/touch_bar_for... ui/base/cocoa/touch_bar_forward_declarations.h:43: - (nullable __kindof NSTouchBarItem*)itemForIdentifier: I think __kindof is Xcode 7 (10.11 SDK). But hopefully this is a compiler intrinsic so isn't dependent on the SDK we link against. (i.e. the 10.10 SDK won't have any generics and might get confused :/)
The CQ bit was checked by sdy@chromium.org
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": 1, "attempt_start_ts": 1480548670812560, "parent_rev":
"ef2739fb114a873e5291a266a66a71ebaea37c6e", "commit_rev":
"c993dccf1db82173280db7f3964b6772e39b4b97"}
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. This is an improved version of https://crrev.com/799f1635ddedf7fd2f69f7dca26b0f2bf6399cba 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. This is an improved version of https://crrev.com/799f1635ddedf7fd2f69f7dca26b0f2bf6399cba BUG=666893 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
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. This is an improved version of https://crrev.com/799f1635ddedf7fd2f69f7dca26b0f2bf6399cba 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. This is an improved version of https://crrev.com/799f1635ddedf7fd2f69f7dca26b0f2bf6399cba BUG=666893 Committed: https://crrev.com/3a6a6c13fe021aa3124ecaadf4655aa76ce47a99 Cr-Commit-Position: refs/heads/master@{#435477} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/3a6a6c13fe021aa3124ecaadf4655aa76ce47a99 Cr-Commit-Position: refs/heads/master@{#435477}
Message was sent while issue was closed.
On 2016/11/30 23:29:27, tapted wrote: > lgtm - Thanks! A couple of concerns but maybe we just need to start smoking out > these older bots.. hopefully they are all happy with this > > https://codereview.chromium.org/2543703002/diff/1/ui/base/cocoa/touch_bar_for... > File ui/base/cocoa/touch_bar_forward_declarations.h (right): > > https://codereview.chromium.org/2543703002/diff/1/ui/base/cocoa/touch_bar_for... > ui/base/cocoa/touch_bar_forward_declarations.h:14: #pragma clang assume_nonnull > begin > I think this is OK since we're replicating a system header and want to reduce > the likelihood that we let stuff through the CQ that will break when compiling > against the authentic 10.12.1 SDK (but if you're curious there's some active > discussion around whether we should use audited regions like this for new code > at http://go/uzjld ) Interesting discussion, thanks for the link! > https://codereview.chromium.org/2543703002/diff/1/ui/base/cocoa/touch_bar_for... > ui/base/cocoa/touch_bar_forward_declarations.h:29: - (instancetype)init > NS_DESIGNATED_INITIALIZER; > Internet suggests NS_DESIGNATED_INITIALIZER is XCode 6, but I'm not sure on the > SDK. https://www.chromium.org/developers/design-documents/mac-xib-files suggests > we still use XCode 5 in some places. But I _think_ this is OK now. > NS_DESIGNATED_INITIALIZER is used a lot on a lot of ios code, but > image_capture_device_manager_unittest.mm is the only occurence I could find for > MacOS code. (and it looks like official builders are happy with that). > > My 10.10 SDK's NSObjCRuntime.h has this defined in any case. > > https://codereview.chromium.org/2543703002/diff/1/ui/base/cocoa/touch_bar_for... > ui/base/cocoa/touch_bar_forward_declarations.h:43: - (nullable __kindof > NSTouchBarItem*)itemForIdentifier: > I think __kindof is Xcode 7 (10.11 SDK). But hopefully this is a compiler > intrinsic so isn't dependent on the SDK we link against. (i.e. the 10.10 SDK > won't have any generics and might get confused :/) Fingers crossed :/ ? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
