Bug 193056 – Prefer RetainPtr<NSObject> to RetainPtr<NSObject *>

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

[David Kilzer (:ddkilzer)]

Prefer RetainPtr<NSObject> to RetainPtr<NSObject *> because:
- It was the original intent based on Macieij's comment from r16987.
- The vast majority of RetainPtr<> use in WebKit already omits the '*' operator for NS types.  (Yay consistency!)
- We can write a style checker rule to enforce it.
- We can make adoptNS() and retainPtr() create the correct types using C++ template metaprogramming.

Note that RetainPtr<NSObject *> still works as it did before (and tests have been added to verify that it works with RetainPtr<NSObject>).

This change simply formalizes the preference for RetainPtr<NSObject> over RetainPtr<NSObject *>.

[David Kilzer (:ddkilzer)]

Created attachment 358136 [details]
Patch v1

[David Kilzer (:ddkilzer)]

Comment on attachment 358136 [details]
Patch v1

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

> Source/WTF/wtf/RetainPtr.h:60
> +template<typename T> RetainPtr<typename std::conditional<std::is_convertible<T, id>::value && !std::is_same<T, id>::value, typename std::remove_pointer<T>::type, T>::type> adoptNS(T NS_RELEASES_ARGUMENT) WARN_UNUSED_RETURN;

After posting this patch, I just realized I might be able to do something simpler which matches the way that ValueType is defined:

-template<typename T> RetainPtr<typename std::conditional<std::is_convertible<T, id>::value && !std::is_same<T, id>::value, typename std::remove_pointer<T>::type, T>::type> adoptNS(T NS_RELEASES_ARGUMENT) WARN_UNUSED_RETURN;
+template<typename T> RetainPtr<typename std::remove_pointer<T>::type> adoptNS(T NS_RELEASES_ARGUMENT) WARN_UNUSED_RETURN;

Testing now.

[EWS Watchlist]

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


ERROR: Tools/Scripts/webkitpy/style/checkers/cpp.py:3321:  at least two spaces before inline comment  [pep8/E261] [5]
Total errors found: 1 in 42 files


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

[David Kilzer (:ddkilzer)]

Comment on attachment 358136 [details]
Patch v1

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

>> Source/WTF/wtf/RetainPtr.h:60
>> +template<typename T> RetainPtr<typename std::conditional<std::is_convertible<T, id>::value && !std::is_same<T, id>::value, typename std::remove_pointer<T>::type, T>::type> adoptNS(T NS_RELEASES_ARGUMENT) WARN_UNUSED_RETURN;
> 
> After posting this patch, I just realized I might be able to do something simpler which matches the way that ValueType is defined:
> 
> -template<typename T> RetainPtr<typename std::conditional<std::is_convertible<T, id>::value && !std::is_same<T, id>::value, typename std::remove_pointer<T>::type, T>::type> adoptNS(T NS_RELEASES_ARGUMENT) WARN_UNUSED_RETURN;
> +template<typename T> RetainPtr<typename std::remove_pointer<T>::type> adoptNS(T NS_RELEASES_ARGUMENT) WARN_UNUSED_RETURN;
> 
> Testing now.

Yes this can be simplified.  Making some changes now.

[David Kilzer (:ddkilzer)]

Comment on attachment 358136 [details]
Patch v1

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

> Tools/TestWebKitAPI/Tests/WTF/cf/RetainPtr.cpp:168
> +    EXPECT_FALSE(optionalObject2.value());

This caused a RELEASE_ASSERT() for `optionalObject2` not having a value.  Needs to be:

    EXPECT_FALSE(optionalObject2);

> Tools/TestWebKitAPI/Tests/WTF/ns/RetainPtr.mm:253
> +    EXPECT_FALSE(optionalObject2.value());

This caused a RELEASE_ASSERT() for `optionalObject2` not having a value.  Needs to be:

    EXPECT_FALSE(optionalObject2);

[David Kilzer (:ddkilzer)]

The way std::remove_pointer<T> works is that it only removes explicit pointer operators from type T, even if type T is a pointer beneath a typedef.

That means:

typename std::remove_pointer<NSArray *>::type == NSArray

But that:

typename std::remove_pointer<NSArray>::type == NSArray
typename std::remove_pointer<id>::type == id
typename std::remove_pointer<CFArrayRef>::type == CFArrayRef

This is why ValueType uses an unconditional std::remove_pointer<t> in the RetainPtr class definition.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #6)
> The way std::remove_pointer<T> works is that it only removes explicit
> pointer operators from type T, even if type T is a pointer beneath a typedef.
> 
> That means:
> 
> typename std::remove_pointer<NSArray *>::type == NSArray
> 
> But that:
> 
> typename std::remove_pointer<NSArray>::type == NSArray
> typename std::remove_pointer<id>::type == id
> typename std::remove_pointer<CFArrayRef>::type == CFArrayRef

And I'm wrong (yay tests!):

Tools/TestWebKitAPI/Tests/WTF/cf/RetainPtr.cpp:141:38: error: no viable conversion from 'RetainPtr<typename std::remove_pointer<const __CFNumber *>::type>' (aka 'RetainPtr<const __CFNumber>') to 'Optional<RetainPtr<CFNumberRef> >' (aka 'Optional<RetainPtr<const __CFNumber *> >')
    Optional<RetainPtr<CFNumberRef>> optionalObject2 = retainPtr(CFNumberCreate(kCFAllocatorDefault, kCFNumberFloatType,
 &value));
                                     ^                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~

I think it depends on how the type is defined by the compiler.

[David Kilzer (:ddkilzer)]

Created attachment 358140 [details]
Patch v2

[David Kilzer (:ddkilzer)]

Comment on attachment 358140 [details]
Patch v2

- Fix crashing TestWTF.RetainPtr tests.
- Defined shared typedef to clean up RetainPtr.h changes.

[EWS Watchlist]

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


ERROR: Tools/Scripts/webkitpy/style/checkers/cpp.py:3321:  at least two spaces before inline comment  [pep8/E261] [5]
Total errors found: 1 in 42 files


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

[Alex Christensen]

Comment on attachment 358140 [details]
Patch v2

Was the motivation for doing all this work just style?

[David Kilzer (:ddkilzer)]

(In reply to Alex Christensen from comment #11)
> Comment on attachment 358140 [details]
> Patch v2
> 
> Was the motivation for doing all this work just style?

Yes, for consistency.  It's one of the primary reasons we have WebKit style guidelines.

[David Kilzer (:ddkilzer)]

Thanks for the review, Alex!  Waiting to land this until after the next branch.

[WebKit Commit Bot]

Comment on attachment 358140 [details]
Patch v2

Rejecting attachment 358140 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 358140, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=358140&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=193056&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 358140 from bug 193056.
Fetching: https://bugs.webkit.org/attachment.cgi?id=358140
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Alex Christensen']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 42 diffs from patch file(s).
patching file Source/WTF/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKitLegacy/mac/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WTF/wtf/RetainPtr.h
patching file Source/WTF/wtf/SchedulePair.h
patching file Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm
patching file Source/WebCore/platform/network/cf/AuthenticationChallenge.h
patching file Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm
patching file Source/WebKit/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.mm
patching file Source/WebKit/UIProcess/API/Cocoa/_WKThumbnailView.mm
patching file Source/WebKit/UIProcess/ApplePay/WebPaymentCoordinatorProxy.h
patching file Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.h
patching file Source/WebKit/UIProcess/Plugins/PluginProcessProxy.h
patching file Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeHostIOS.mm
patching file Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemoteCustom.mm
patching file Source/WebKitLegacy/mac/Plugins/Hosted/NetscapePluginHostProxy.h
patching file Source/WebKitLegacy/mac/Plugins/Hosted/NetscapePluginInstanceProxy.h
patching file Source/WebKitLegacy/mac/Plugins/Hosted/NetscapePluginInstanceProxy.mm
patching file Source/WebKitLegacy/mac/Plugins/Hosted/ProxyInstance.mm
patching file Source/WebKitLegacy/mac/WebCoreSupport/WebGeolocationClient.mm
patching file Source/WebKitLegacy/mac/WebView/WebDataSource.mm
patching file Source/WebKitLegacy/mac/WebView/WebView.mm
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/Scripts/webkitpy/style/checker.py
patching file Tools/Scripts/webkitpy/style/checkers/cpp.py
patching file Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py
patching file Tools/TestRunnerShared/EventSerialization/mac/EventSerializerMac.h
patching file Tools/TestWebKitAPI/EditingTestHarness.h
patching file Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
Hunk #1 succeeded at 172 (offset 1 line).
Hunk #2 succeeded at 3806 (offset 24 lines).
patching file Tools/TestWebKitAPI/Tests/WTF/cf/RetainPtr.cpp
patching file Tools/TestWebKitAPI/Tests/WTF/ns/RetainPtr.mm
patching file Tools/TestWebKitAPI/Tests/WebKitCocoa/DownloadProgress.mm
patching file Tools/TestWebKitAPI/Tests/WebKitCocoa/IconLoadingDelegate.mm
patching file Tools/TestWebKitAPI/Tests/WebKitCocoa/JITEnabled.mm
patching file Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
patching file Tools/TestWebKitAPI/Tests/WebKitCocoa/VideoControlsManager.mm
patching file Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm
Hunk #1 FAILED at 865.
1 out of 1 hunk FAILED -- saving rejects to file Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm.rej
patching file Tools/TestWebKitAPI/ios/DragAndDropSimulatorIOS.mm
patching file Tools/WebKitTestRunner/InjectedBundle/cocoa/ActivateFontsCocoa.mm
patching file Tools/WebKitTestRunner/InjectedBundle/ios/InjectedBundleIOS.mm
patching file Tools/WebKitTestRunner/cocoa/TestRunnerWKWebView.mm

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Alex Christensen']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/10661738

[David Kilzer (:ddkilzer)]

Committed r239709: <https://trac.webkit.org/changeset/239709>

[David Kilzer (:ddkilzer)]

<rdar://problem/47115086>