Summary: | Use RetainPtr<> / OSObjectPtr<> more in WebKit | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||||||||||
Component: | WebCore Misc. | Assignee: | Chris Dumez <cdumez> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, benjamin, changseok, cmarcelo, darin, eric.carlson, esprehn+autocc, ews-watchlist, ggaren, glenn, hi, hta, jer.noble, joepeck, keith_miller, kondapallykalyan, mark.lam, mifenton, mmaxfield, msaboff, pdr, philipj, saam, sam, sergio, tommyw, tzagallo, webkit-bug-importer | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||
Bug Blocks: | 223049 | ||||||||||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2021-03-10 10:18:12 PST
Created attachment 422843 [details]
Patch
Created attachment 422844 [details]
Patch
Created attachment 422845 [details]
Patch
Created attachment 422846 [details]
Patch
Created attachment 422849 [details]
Patch
Created attachment 422851 [details]
Patch
Created attachment 422858 [details]
Patch
Created attachment 422863 [details]
Patch
Comment on attachment 422863 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422863&action=review > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:161 > + inline dispatch_queue_t dispatchQueue() const { return m_notificationQueue.get(); } Unneeded use of "inline" here and throughout this class. > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1569 > + m_avPlayer = playerRef; Seems a slight shame that we hold one reference in the object and another in a local variable. Could we just use m_avPlayer directly throughout? > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1609 > + auto itemRef = adoptF(AVCFPlayerItemCreateWithAsset(kCFAllocatorDefault, avAsset(), m_notificationQueue.get())); > + m_avPlayerItem = itemRef; Ditto. > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1759 > + static const CFStringRef keyNames[] = { > AVCFAssetPropertyPlayable > }; > - propertyKeyName = CFArrayCreate(0, (const void**)keyNames, sizeof(keyNames) / sizeof(keyNames[0]), &kCFTypeArrayCallBacks); > - } > + return adoptCF(CFArrayCreate(0, (const void**)keyNames, sizeof(keyNames) / sizeof(keyNames[0]), &kCFTypeArrayCallBacks)); Since this code is only called once, we can write it more simply and not try to optimize so much and write such strange code. If this was Objective-C I would just write @[ AVCFAssetPropertyPlayable ] with typecasts. Here we could: 1) Type the local variable to match what the function takes, const void* elements instead of CFStringRef elements. 2) Don't use a static because this is one time code so it’s fine to dynamically allocate the array. 3) Because of (1) and (2) don’t need to typecast when calling CFArrayCreate. 4) Use std::size instead of sizeof(x) / sizeof(x[0]). > Source/WebCore/platform/graphics/cg/PathCG.cpp:232 > bool ret = CGPathContainsPoint(path.get(), 0, point, rule == WindRule::EvenOdd ? true : false); > return ret; This code is funny. Lets write it more the way we would if it was new today: return CGPathContainsPoint(path.get(), nullptr, point, rule == WindRule::EvenOdd); > Source/WebCore/platform/network/cf/CredentialStorageCFNet.cpp:49 > + auto protectionSpaceCF = createCF(protectionSpace); > + auto credentialCF = copyCredentialFromProtectionSpace(protectionSpaceCF.get()); > return core(credentialCF.get()); Would work fine in a single line: return core(copyCredentialFromProtectionSpace(createCF(protectionSpace).get()).get()); > Source/WebCore/platform/text/mac/TextBoundaries.mm:75 > const char* temp = currentTextBreakLocaleID(); "temp" > Source/WebCore/platform/text/mac/TextBoundaries.mm:80 > + if (!locale.get()) Do we need .get() here? > Source/WebCore/platform/text/mac/TextBoundaries.mm:86 > + if (!tokenizer.get()) Do we need .get() here? Created attachment 422878 [details]
Patch
(In reply to Darin Adler from comment #9) > Comment on attachment 422863 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=422863&action=review > > > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:161 > > + inline dispatch_queue_t dispatchQueue() const { return m_notificationQueue.get(); } > > Unneeded use of "inline" here and throughout this class. > > > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1569 > > + m_avPlayer = playerRef; > > Seems a slight shame that we hold one reference in the object and another in > a local variable. Could we just use m_avPlayer directly throughout? > > > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1609 > > + auto itemRef = adoptF(AVCFPlayerItemCreateWithAsset(kCFAllocatorDefault, avAsset(), m_notificationQueue.get())); > > + m_avPlayerItem = itemRef; > > Ditto. > > > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1759 > > + static const CFStringRef keyNames[] = { > > AVCFAssetPropertyPlayable > > }; > > - propertyKeyName = CFArrayCreate(0, (const void**)keyNames, sizeof(keyNames) / sizeof(keyNames[0]), &kCFTypeArrayCallBacks); > > - } > > + return adoptCF(CFArrayCreate(0, (const void**)keyNames, sizeof(keyNames) / sizeof(keyNames[0]), &kCFTypeArrayCallBacks)); > > Since this code is only called once, we can write it more simply and not try > to optimize so much and write such strange code. If this was Objective-C I > would just write @[ AVCFAssetPropertyPlayable ] with typecasts. Here we > could: > > 1) Type the local variable to match what the function takes, const void* > elements instead of CFStringRef elements. > 2) Don't use a static because this is one time code so it’s fine to > dynamically allocate the array. > 3) Because of (1) and (2) don’t need to typecast when calling CFArrayCreate. > 4) Use std::size instead of sizeof(x) / sizeof(x[0]). > > > Source/WebCore/platform/graphics/cg/PathCG.cpp:232 > > bool ret = CGPathContainsPoint(path.get(), 0, point, rule == WindRule::EvenOdd ? true : false); > > return ret; > > This code is funny. Lets write it more the way we would if it was new today: > > return CGPathContainsPoint(path.get(), nullptr, point, rule == > WindRule::EvenOdd); > > > Source/WebCore/platform/network/cf/CredentialStorageCFNet.cpp:49 > > + auto protectionSpaceCF = createCF(protectionSpace); > > + auto credentialCF = copyCredentialFromProtectionSpace(protectionSpaceCF.get()); > > return core(credentialCF.get()); > > Would work fine in a single line: > > return > core(copyCredentialFromProtectionSpace(createCF(protectionSpace).get()). > get()); > > > Source/WebCore/platform/text/mac/TextBoundaries.mm:75 > > const char* temp = currentTextBreakLocaleID(); > > "temp" > > > Source/WebCore/platform/text/mac/TextBoundaries.mm:80 > > + if (!locale.get()) > > Do we need .get() here? > > > Source/WebCore/platform/text/mac/TextBoundaries.mm:86 > > + if (!tokenizer.get()) > > Do we need .get() here? Yes, they were needed: ./platform/text/mac/TextBoundaries.mm:80:9: error: invalid argument type 'WTF::NeverDestroyed<WTF::RetainPtr<const __CFLocale *>, WTF::AnyThreadsAccessTraits>' to unary expression if (!locale) ^~~~~~~ ./platform/text/mac/TextBoundaries.mm:86:9: error: invalid argument type 'NeverDestroyed<RetainPtr<CFStringTokenizerRef> >' (aka 'NeverDestroyed<RetainPtr<__CFStringTokenizer *> >') to unary expression if (!tokenizer) ^~~~~~~~~~ Created attachment 422881 [details]
Patch
Comment on attachment 422881 [details] Patch Clearing flags on attachment: 422881 Committed r274252 (235156@main): <https://commits.webkit.org/235156@main> All reviewed patches have been landed. Closing bug. |