[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