[webkit-reviews] review granted: [Bug 223030] Use RetainPtr<> / OSObjectPtr<> more in WebKit : [Attachment 422863] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 223030: Use RetainPtr<> / OSObjectPtr<> more in WebKit
https://bugs.webkit.org/show_bug.cgi?id=223030

Attachment 422863: Patch

https://bugs.webkit.org/attachment.cgi?id=422863&action=review




--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 422863
  --> https://bugs.webkit.org/attachment.cgi?id=422863
Patch

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

>
Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundation
CF.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/MediaPlayerPrivateAVFoundation
CF.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/MediaPlayerPrivateAVFoundation
CF.cpp:1609
> +    auto itemRef = adoptF(AVCFPlayerItemCreateWithAsset(kCFAllocatorDefault,
avAsset(), m_notificationQueue.get()));
> +    m_avPlayerItem = itemRef;

Ditto.

>
Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundation
CF.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?