Bug 186526 – [Cocoa] Make some RetainPtr refinements to get more ready for ARC

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Darin Adler]

[Cocoa] Make some RetainPtr refinements to get more ready for ARC

[Darin Adler]

Created attachment 342440 [details]
Patch

[Darin Adler]

As part of review, I’d like feedback on whether I should be adding these new RetainPtr members or not.

[Darin Adler]

Created attachment 342548 [details]
Patch

[Darin Adler]

I think Anders is probably the one who should review this.

New patch fixes const-correctness of cast inside RetainPtr::autoreleaseCF as well as making it work correctly with nullptr, and uses autoreleaseCF in one place in iOS-specific code that I had missed in WebKitLegacy.

[Darin Adler]

Created attachment 342550 [details]
Patch

[EWS Watchlist]

Comment on attachment 342550 [details]
Patch

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

New failing tests:
media/video-buffering-allowed.html

[EWS Watchlist]

Created attachment 342579 [details]
Archive of layout-test-results from ews116 for mac-sierra

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

[Darin Adler]

Created attachment 342632 [details]
Patch

[Anders Carlsson]

Comment on attachment 342632 [details]
Patch

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

> Source/WTF/ChangeLog:13
> +        * wtf/RetainPtr.h: Added autoreleaseCF and bridgingAutorelease.
> +        The existing autorelease is for autorelease of an Objective-C type.
> +        The two new ones are for autorelease of CF types: We can use
> +        autoreleaseCF when we want an autoreleased CF type, and we can
> +        use bridgingAutorelease when we want to autorelease as part of
> +        converting from a CF type to an Objective-C type.

I think these (autoreleaseCF anyway) adds some confusion. Would it be possible to use std::enable_if to just pick the right autorelease based on the type?

[Darin Adler]

(In reply to Anders Carlsson from comment #9)
> I think these (autoreleaseCF anyway) adds some confusion. Would it be
> possible to use std::enable_if to just pick the right autorelease based on
> the type?

I don’t know; I’ll try.

[Darin Adler]

Created attachment 342720 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/RetainPtr.h:194:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/RetainPtr.h:199:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 21 files


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

[Darin Adler]

Comment on attachment 342720 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:147
> +#if 0 && (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED == 101400) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED == 120000)

Oops, did not mean to post patch with this in it.

[Darin Adler]

(In reply to Anders Carlsson from comment #9)
> Would it be
> possible to use std::enable_if to just pick the right autorelease based on
> the type?

Yes, I did this in the latest patch, which no longer adds autoreleaseCF.

[Anders Carlsson]

Comment on attachment 342720 [details]
Patch

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

> Source/WTF/wtf/RetainPtr.h:150
> +#ifdef __OBJC__
> +    template<typename U> typename std::enable_if<std::is_convertible<U, id>::value, PtrType>::type autoreleaseHelper();
> +    template<typename U> typename std::enable_if<!std::is_convertible<U, id>::value, PtrType>::type autoreleaseHelper();
> +#endif

You can use std::enable_if_t here, and if WebKit requires C++17 you should be able to use std::is_convertible_v as well.

[Darin Adler]

Comment on attachment 342720 [details]
Patch

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

>> Source/WTF/wtf/RetainPtr.h:150
>> +#endif
> 
> You can use std::enable_if_t here, and if WebKit requires C++17 you should be able to use std::is_convertible_v as well.

Great, thanks for the suggestion. I will switch to those and retest before landing.

Roughly speaking, WebKit does require C++17 now, but there are many exceptions to that general rule. However, since this is ObjC-only code, all that really matters is the level of support we have in clang with the option we specify (which is now C++17) and we don’t need to worry about gcc or MSVC for this code.

[Darin Adler]

Committed r232911: <https://trac.webkit.org/changeset/232911>

[Radar WebKit Bug Importer]

<rdar://problem/41193181>

[Darin Adler]

I was unable to use std::is_convertible_v at this time because we are not yet passing in C++17 as our dialect for clang.