Bug 193056

Created attachment 358136 [details] Patch v1

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.

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.

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.

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);

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.

(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.

Created attachment 358140 [details] Patch v2

Comment on attachment 358140 [details] Patch v2 - Fix crashing TestWTF.RetainPtr tests. - Defined shared typedef to clean up RetainPtr.h changes.

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.

Comment on attachment 358140 [details] Patch v2 Was the motivation for doing all this work just style?

(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.

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

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 commit-queue@webkit.org... 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

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

<rdar://problem/47115086>