Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2253863002:
Fix accessibility constant clash in OSX 10.12. (Closed)
|
Created:
4 years, 4 months ago by pwnall Modified:
4 years, 4 months ago CC:
aboxhall+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, jam, je_julie, nektar+watch_chromium.org, yuzo+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix accessibility constant clash in OSX 10.12.
BUG=638529
Committed: https://crrev.com/913febd51b3506b59e9dd06ffd2869837f4bfc14
Cr-Commit-Position: refs/heads/master@{#413218}
Patch Set 1 #Patch Set 2 : Applied feedback. #
Total comments: 2
Patch Set 3 : Addressed nit. #
Total comments: 1
Patch Set 4 : Re-fixed build against 10.12 SDK. #Patch Set 5 : Applied feedback. #Messages
Total messages: 41 (24 generated)
pwnall@chromium.org changed reviewers: + aboxhall@chromium.org
I couldn't figure out a better way to fix the constant clash in 5 minutes. If there is one, I'd be happy to implement it. In an ideal world, we'd be able to make the definition in the 10.12 SDK go away, but I don't know how to do that. WDYT?
The CQ bit was checked by pwnall@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.
dmazzoni@chromium.org changed reviewers: + dmazzoni@chromium.org, nektar@chromium.org, tapted@chromium.org
+tapted, nektar I'm pretty sure there's a standard way to do this now - Trent or Nektarios, where's the file where we define Mac constants that are defined in only some API versions, or what's the best practice here?
On 2016/08/17 19:57:14, dmazzoni wrote:
> +tapted, nektar
>
> I'm pretty sure there's a standard way to do this now - Trent or Nektarios,
> where's the file where we define Mac constants that are defined in only
> some API versions, or what's the best practice here?
There's a file base/mac/sdk_forward_declarations.h, but if this is only used in
a single translation unit, it's easier to just add the following to this file:
#if !defined(MAC_OS_X_VERSION_10_12) || \
MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_12
NSString* const NSAccessibilityRequiredAttribute = @"AXRequired";
#endif
Description was changed from ========== Fix accessibility constant clash in OSX 10.12. BUG=638529 ========== to ========== Fix accessibility constant clash in OSX 10.12. BUG=638529 ==========
erikchen@chromium.org changed reviewers: + erikchen@chromium.org
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by pwnall@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...
On 2016/08/19 00:23:19, erikchen wrote: > On 2016/08/17 19:57:14, dmazzoni wrote: > > +tapted, nektar > > > > I'm pretty sure there's a standard way to do this now - Trent or Nektarios, > > where's the file where we define Mac constants that are defined in only > > some API versions, or what's the best practice here? > > There's a file base/mac/sdk_forward_declarations.h, but if this is only used in > a single translation unit, it's easier to just add the following to this file: > > #if !defined(MAC_OS_X_VERSION_10_12) || \ > MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_12 > NSString* const NSAccessibilityRequiredAttribute = @"AXRequired"; > #endif I used your suggestion exactly as typed here. I'll use base/mac/sdk_forward_declarations.h for the other CL I sent your way. Thank you very much! Can you please take another look to this CL?
erikchen@chromium.org changed reviewers: + avi@chromium.org
small nit, otherwise lgtm. +avi for content/ OWNER. https://codereview.chromium.org/2253863002/diff/40001/content/browser/accessi... File content/browser/accessibility/browser_accessibility_cocoa.mm (right): https://codereview.chromium.org/2253863002/diff/40001/content/browser/accessi... content/browser/accessibility/browser_accessibility_cocoa.mm:58: NSString* const NSAccessibilityRequiredAttribute = @"AXRequired"; whoops, shouldn't have had 2 spaces at the beginning of the line. My bad. You can use "git cl format" to auto-format your CL to deal with these pesky nits.
The CQ bit was checked by pwnall@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...
Thank you very much for the quick feedback! https://codereview.chromium.org/2253863002/diff/40001/content/browser/accessi... File content/browser/accessibility/browser_accessibility_cocoa.mm (right): https://codereview.chromium.org/2253863002/diff/40001/content/browser/accessi... content/browser/accessibility/browser_accessibility_cocoa.mm:58: NSString* const NSAccessibilityRequiredAttribute = @"AXRequired"; On 2016/08/19 00:59:00, erikchen wrote: > whoops, shouldn't have had 2 spaces at the beginning of the line. My bad. You > can use "git cl format" to auto-format your CL to deal with these pesky nits. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
It turns out I didn't compile the latest revision against the 10.12 SDK. Unfortunately, the approach we tried doesn't work, so I tried something different. Can you please take another look at this CL? https://codereview.chromium.org/2253863002/diff/60001/content/browser/accessi... File content/browser/accessibility/browser_accessibility_cocoa.mm (right): https://codereview.chromium.org/2253863002/diff/60001/content/browser/accessi... content/browser/accessibility/browser_accessibility_cocoa.mm:57: MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_12 Using MAC_OS_X_VERSION_MIN_REQUIRED doesn't actually work, because we retain the same minimum version when compiling against the 10.12 SDK as before. MAC_OS_X_VERSION_MAX_ALLOWED (also used in base/mac/sdk_forward_declarations.h) matches the desired intent to get the block excluded when compiling against SDKs >= 10.12. Unfortunately, the SDK definition is marked NS_AVAILABLE_MAC(10_12), meaning that AppKit does not actually export the symbol before OSX 10.12. This triggers the following warning, which breaks the build. ../../content/browser/accessibility/browser_accessibility_cocoa.mm:556:8: error: 'NSAccessibilityRequiredAttribute' is partial: introduced in macOS 10.12 [-Werror,-Wpartial-availability] {NSAccessibilityRequiredAttribute, @"required"}, ^ /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSAccessibilityConstants.h:89:31: note: 'NSAccessibilityRequiredAttribute' has been explicitly marked partial here APPKIT_EXTERN NSString *const NSAccessibilityRequiredAttribute NS_AVAILABLE_MAC(10_12); //(NSNumber *) - (boolValue) whether a form field is required to have content for successful submission of the form ^ ../../content/browser/accessibility/browser_accessibility_cocoa.mm:556:8: note: explicitly redeclare 'NSAccessibilityRequiredAttribute' to silence this warning {NSAccessibilityRequiredAttribute, @"required"},
On 2016/08/19 08:15:03, pwnall wrote: > It turns out I didn't compile the latest revision against the 10.12 SDK. > Unfortunately, the approach we tried doesn't work, so I tried something > different. Can you please take another look at this CL? > > https://codereview.chromium.org/2253863002/diff/60001/content/browser/accessi... > File content/browser/accessibility/browser_accessibility_cocoa.mm (right): > > https://codereview.chromium.org/2253863002/diff/60001/content/browser/accessi... > content/browser/accessibility/browser_accessibility_cocoa.mm:57: > MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_12 > Using MAC_OS_X_VERSION_MIN_REQUIRED doesn't actually work, because we retain the > same minimum version when compiling against the 10.12 SDK as before. > MAC_OS_X_VERSION_MAX_ALLOWED (also used in base/mac/sdk_forward_declarations.h) > matches the desired intent to get the block excluded when compiling against SDKs > >= 10.12. > > Unfortunately, the SDK definition is marked NS_AVAILABLE_MAC(10_12), meaning > that AppKit does not actually export the symbol before OSX 10.12. This triggers > the following warning, which breaks the build. > > ../../content/browser/accessibility/browser_accessibility_cocoa.mm:556:8: error: > 'NSAccessibilityRequiredAttribute' is partial: introduced in macOS 10.12 > [-Werror,-Wpartial-availability] > {NSAccessibilityRequiredAttribute, @"required"}, > ^ > /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSAccessibilityConstants.h:89:31: > note: 'NSAccessibilityRequiredAttribute' has been explicitly marked partial here > APPKIT_EXTERN NSString *const NSAccessibilityRequiredAttribute > NS_AVAILABLE_MAC(10_12); //(NSNumber *) - (boolValue) whether a form field is > required to have content for successful submission of the form > ^ > ../../content/browser/accessibility/browser_accessibility_cocoa.mm:556:8: note: > explicitly redeclare 'NSAccessibilityRequiredAttribute' to silence this warning > {NSAccessibilityRequiredAttribute, @"required"}, pwnall: Something odd is happening. If we look at NSUserActivityTypeBrowsingWeb, we see that this type of declaration should work: https://cs-staging.chromium.org/search/?q=NSUserActivityTypeBrowsingWeb+packa... Looking at http://clang.llvm.org/docs/AttributeReference.html#availability, redeclarations of statements with availability attributes shouldn't be an issue. Looking back at the error message in the original bug: """ ../../content/browser/accessibility/browser_accessibility_cocoa.mm:553:8: error: reference to 'NSAccessibilityRequiredAttribute' is ambiguous {NSAccessibilityRequiredAttribute, @"required"}, ^ /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSAccessibilityConstants.h:89:31: note: candidate found by name lookup is 'NSAccessibilityRequiredAttribute' APPKIT_EXTERN NSString *const NSAccessibilityRequiredAttribute NS_AVAILABLE_MAC(10_12); //(NSNumber *) - (boolValue) whether a form field is required to have content for successful submission of the form ^ ../../content/browser/accessibility/browser_accessibility_cocoa.mm:56:17: note: candidate found by name lookup is '(anonymous namespace)::NSAccessibilityRequiredAttribute' NSString* const NSAccessibilityRequiredAttribute = @"AXRequired"; """ It looks like the actual problem might be that when we redeclare NSAccessibilityRequiredAttribute, we do so in an anonymous C++ namespace, rather than an extern "C" block. Can you try moving my original suggestion into an extern "C" block and see if that fixes the problem?
> pwnall: Something odd is happening. If we look at NSUserActivityTypeBrowsingWeb, > we see that this type of declaration should work: > > https://cs-staging.chromium.org/search/?q=NSUserActivityTypeBrowsingWeb+packa... > > Looking at http://clang.llvm.org/docs/AttributeReference.html#availability, > redeclarations of statements with availability attributes shouldn't be an issue. Thank you very much for the suggestion! I'll give it a try. The paragraph below got me worried that we'd produce a binary that can only run on OSX 10.12 and above. "A declaration can typically be used even when deploying back to a platform version prior to when the declaration was introduced. When this happens, the declaration is weakly linked, as if the weak_import attribute were added to the declaration. A weakly-linked declaration may or may not be present a run-time, and a program can determine whether the declaration is present by checking whether the address of that declaration is non-NULL."
On 2016/08/19 18:05:53, pwnall wrote: > > pwnall: Something odd is happening. If we look at > NSUserActivityTypeBrowsingWeb, > > we see that this type of declaration should work: > > > > > https://cs-staging.chromium.org/search/?q=NSUserActivityTypeBrowsingWeb+packa... > > > > Looking at http://clang.llvm.org/docs/AttributeReference.html#availability, > > redeclarations of statements with availability attributes shouldn't be an > issue. > > Thank you very much for the suggestion! I'll give it a try. > > The paragraph below got me worried that we'd produce a binary that can only run > on OSX 10.12 and above. > > "A declaration can typically be used even when deploying back to a platform > version prior to when the declaration was introduced. When this happens, the > declaration is weakly linked, as if the weak_import attribute were added to the > declaration. A weakly-linked declaration may or may not be present a run-time, > and a program can determine whether the declaration is present by checking > whether the address of that declaration is non-NULL." That's why we wrap in the conditional: """ #if !defined(MAC_OS_X_VERSION_10_12) || \ MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_12 """ This basically translates to "only use the SDK's definition of NSAccessibilityRequiredAttribute if we're using a 10.12+ SDK, and the lowest OS version we support is 10.12. Obviously this evaluates for now, and we use our redeclaration of NSAccessibilityRequiredAttribute. The reason we add this conditional to begin with is so that eventually, when we only support 10.12+, we can find this statement and get rid of it.
The CQ bit was checked by pwnall@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...
I just verified that a build produced by the 10.12 SDK runs on 10.11. Thank you very much for figuring this out, erikchen!
On 2016/08/19 19:09:05, pwnall wrote: > I just verified that a build produced by the 10.12 SDK runs on 10.11. Thank you > very much for figuring this out, erikchen! Also, does this still LGTY?
yup, still lgtm.
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 pwnall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2253863002/#ps90001 (title: "Applied feedback.")
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 ========== Fix accessibility constant clash in OSX 10.12. BUG=638529 ========== to ========== Fix accessibility constant clash in OSX 10.12. BUG=638529 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:90001)
Message was sent while issue was closed.
Description was changed from ========== Fix accessibility constant clash in OSX 10.12. BUG=638529 ========== to ========== Fix accessibility constant clash in OSX 10.12. BUG=638529 Committed: https://crrev.com/913febd51b3506b59e9dd06ffd2869837f4bfc14 Cr-Commit-Position: refs/heads/master@{#413218} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/913febd51b3506b59e9dd06ffd2869837f4bfc14 Cr-Commit-Position: refs/heads/master@{#413218} |
