Bug 186245 – [Cocoa] Update more code to be more ARC-compatible to prepare for future ARC adoption

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Darin Adler]

[Cocoa] Update more code to be more ARC-compatible to prepare for future ARC adoption

[Darin Adler]

Created attachment 341871 [details]
Patch

[EWS Watchlist]

Attachment 341871 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:791:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:791:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebCore/bridge/objc/objc_class.mm:136:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/bridge/objc/objc_class.mm:223:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 4 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Created attachment 341876 [details]
Patch

[EWS Watchlist]

Attachment 341876 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:791:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:791:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebCore/bridge/objc/objc_class.mm:136:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/bridge/objc/objc_class.mm:223:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 4 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 341876 [details]
Patch

Attachment 341876 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7966497

New failing tests:
accessibility/mac/attributed-string/attributed-string-for-range-with-options.html
accessibility/content-editable-as-textarea.html
accessibility/mac/attributed-string/attributed-string-for-range.html

[EWS Watchlist]

Created attachment 341879 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 341876 [details]
Patch

Attachment 341876 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7966532

New failing tests:
accessibility/mac/attributed-string/attributed-string-for-range-with-options.html
accessibility/content-editable-as-textarea.html
accessibility/mac/attributed-string/attributed-string-for-range.html

[EWS Watchlist]

Created attachment 341880 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 341876 [details]
Patch

Attachment 341876 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7966582

New failing tests:
accessibility/mac/attributed-string/attributed-string-for-range-with-options.html
accessibility/content-editable-as-textarea.html
accessibility/mac/attributed-string/attributed-string-for-range.html

[EWS Watchlist]

Created attachment 341882 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Darin Adler]

Created attachment 341888 [details]
Patch

[Daniel Bates]

Comment on attachment 341888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341888&action=review

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:562
> +        NSDictionary *attributes = @{ (__bridge NSString *)kCVPixelBufferPixelFormatTypeKey: @(kCVPixelFormatType_32BGRA) };

This is ok as-is. Could use auto here.

> Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:222
> +        NSDictionary* videoDecoderSpecification = @{ (__bridge NSString *)kVTVideoDecoderSpecification_EnableHardwareAcceleratedVideoDecoder: @YES };

Ditto.

> Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:819
> +    CFURLRef result = CFURLCreateWithBytes(NULL, urlBytes.data(), fragRg.location - 1, kCFStringEncodingUTF8, NULL);

This is ok as-is. NULL => nullptr 

(There are two in this line).

> Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:821
> +        result = CFURLCreateWithBytes(NULL, urlBytes.data(), fragRg.location - 1, kCFStringEncodingISOLatin1, NULL);

Ditto.

> Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:1012
> +    CFURLRef result = CFURLCreateWithBytes(NULL, urlBytes, numBytes - range.length, kCFStringEncodingUTF8, NULL);

Ditto.

> Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:1014
> +        result = CFURLCreateWithBytes(NULL, urlBytes, numBytes - range.length, kCFStringEncodingISOLatin1, NULL);

Ditto.

> Source/WebCore/platform/network/mac/ResourceErrorMac.mm:121
> +    : ResourceError((__bridge NSError *)cfError)

This is ok as-is. Could use uniform initializer syntax.

[Daniel Bates]

Comment on attachment 341888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341888&action=review

It’s tempting to comment on how to clean up the surrounding code, but that is not the purpose of this patch :)

> Source/WebCore/bridge/objc/objc_class.mm:53
> +    ObjcClass* aClass = reinterpret_cast<ObjcClass*>(const_cast<void*>(CFDictionaryGetValue(classesByIsA, (__bridge CFTypeRef)isa)));

This is ok as-is. Could use auto here.

> Source/WebCore/platform/mac/PluginBlacklist.mm:172
> +    : m_bundleIDToMinimumSecureVersion(adoptNS([bundleIDToMinimumSecureVersion copy]))

This is ok as-is. Could use uniform initializer syntax in this line and the lines below that are part of the initializer list.

> Source/WebCore/platform/mac/URLMac.mm:57
> +    // Creating a toll-free bridged CFURL, because creation with NSURL methods would not preserve the original string.

Is the comma before “because” necessary? Since you’re touching this function, that is unrelated to this purpose of this patch, we could use a C++ style cast in the return statement.

[Darin Adler]

(In reply to Daniel Bates from comment #13)
> Since you’re touching this
> function, that is unrelated to this purpose of this patch, we could use a
> C++ style cast in the return statement.

I made the other changes you requested, but I will not be changing this return statement for two reasons:

1) I prefer not to use C++ style typecasts for conversions between CF types and NS types because of the need for __bridge and this is a conversion from a CF type to an NS type.

2) The reason I didn’t change this particular line to add a __bridge is that it’s tricky to get it right for ARC and I am waiting until later when I figure out how to do it 100% correctly.

It doesn’t seem helpful for me to change it in any way at this time since I don’t know how to make it right. I limited my changes to ones I am confident are correct for ARC, to make the actual conversion a bit easier so we can focus on the more difficult cases.

[Darin Adler]

Committed r232501: <https://trac.webkit.org/changeset/232501>

[Radar WebKit Bug Importer]

<rdar://problem/40793363>

[Daniel Bates]

(In reply to Darin Adler from comment #14)
> (In reply to Daniel Bates from comment #13)
> > Since you’re touching this
> > function, that is unrelated to this purpose of this patch, we could use a
> > C++ style cast in the return statement.
> 
> I made the other changes you requested, but I will not be changing this
> return statement for two reasons:
> 
> 1) I prefer not to use C++ style typecasts for conversions between CF types
> and NS types because of the need for __bridge and this is a conversion from
> a CF type to an NS type.
> 
> 2) The reason I didn’t change this particular line to add a __bridge is that
> it’s tricky to get it right for ARC and I am waiting until later when I
> figure out how to do it 100% correctly.
> 
> It doesn’t seem helpful for me to change it in any way at this time since I
> don’t know how to make it right. I limited my changes to ones I am confident
> are correct for ARC, to make the actual conversion a bit easier so we can
> focus on the more difficult cases.

Makes sense. Out of curiosity, are you using ARC conversion feature in Xcode to help you with the conversion?

[Darin Adler]

(In reply to Daniel Bates from comment #17)
> Out of curiosity, are you using ARC conversion feature in Xcode
> to help you with the conversion?

I tried. So far it has not been helpful.