[webkit-reviews] review granted: [Bug 217739] [GPU Process] Implement mediaPlayerGetRawCookies : [Attachment 411390] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 15 04:36:18 PDT 2020


youenn fablet <youennf at gmail.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 217739: [GPU Process] Implement mediaPlayerGetRawCookies
https://bugs.webkit.org/show_bug.cgi?id=217739

Attachment 411390: Patch

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




--- Comment #4 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 411390
  --> https://bugs.webkit.org/attachment.cgi?id=411390
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:6966
> +	   completionHandler(WTFMove(cookies));

I would return early if getRawCookies returns false.

> Source/WebCore/platform/graphics/MediaPlayer.h:54
> +#include <wtf/Unexpected.h>

Not clear why we now need Expected.h and Unexpected.h.

> Source/WebCore/platform/graphics/MediaPlayer.h:256
> +    using GetRawCookiesCallback =
CompletionHandler<void(Optional<Vector<Cookie>>&&)>;

Do we make any difference between a WTF::nullopt and an empty Vector<>?
Should we simplify to a VecCompletionHandler<void(Vector<Cookie>&&)>;?

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.h:191
> +    void createAVAssetForURL(const URL&, RetainPtr<NSMutableDictionary>);

RetainPtr<>&&
I would have been tempted to do createAVAssetForURL(const URL&, const
Vector<Cookie>&) instead of passing a mutable dictionary.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:762
> +	   createAVAssetForURL(url, options.leakRef());

Shouldn't we write it as:
createAVAssetForURL(url, WTFMove(options));
It seems like there could be a leak if createAVAssetForURL was not taking a
RetainPtr.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:766
> +    player()->getRawCookies(url, [this, weakThis = makeWeakPtr(*this),
options = WTFMove(options), url] (Optional<Vector<Cookie>>&& cookies) mutable {

could write it makeWeakPtr(this)
could use auto instead of Optional<>...

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:778
> +	   createAVAssetForURL(url, options.leakRef());

Ditto

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:781
> +    createAVAssetForURL(url, options.leakRef());

Ditto.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:787
> +{

Should we add back the if (m_avAsset) check here?
Now that we set m_avAsset asynchronously, is there a possibility for
createAVAssetForURL(url) to be called twice, the second one being hopefully a
no-op?

Are there other potential consequences in the code for m_avAsset to be null for
some time after createAVAssetForURL(url) is called?


More information about the webkit-reviews mailing list