Summary: | [Cocoa] Make some RetainPtr refinements to get more ready for ARC | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||||||||
Component: | Web Template Framework | Assignee: | Darin Adler <darin> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | andersca, benjamin, cdumez, cmarcelo, dbates, ews-watchlist, mitz, mmaxfield, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Darin Adler
2018-06-11 10:00:57 PDT
Created attachment 342440 [details]
Patch
As part of review, I’d like feedback on whether I should be adding these new RetainPtr members or not. Created attachment 342548 [details]
Patch
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. Created attachment 342550 [details]
Patch
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 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
Created attachment 342632 [details]
Patch
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? (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. Created attachment 342720 [details]
Patch
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.
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. (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. 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. 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. Committed r232911: <https://trac.webkit.org/changeset/232911> 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. |