Bug 217739 – [GPU Process] Implement mediaPlayerGetRawCookies

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

Implement mediaPlayerGetRawCookies

[Radar WebKit Bug Importer]

<rdar://problem/70313370>

[Eric Carlson]

Created attachment 411390 [details]
Patch

[Peng Liu]

Comment on attachment 411390 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.h:726
> +    void mediaPlayerGetRawCookies(const URL&, MediaPlayerClient::GetRawCookiesCallback&&) const override;

Nit. This function can be "final"?

[youenn fablet]

Comment on attachment 411390 [details]
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/MediaPlayerPrivateAVFoundationObjC.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/MediaPlayerPrivateAVFoundationObjC.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/MediaPlayerPrivateAVFoundationObjC.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/MediaPlayerPrivateAVFoundationObjC.mm:778
> +        createAVAssetForURL(url, options.leakRef());

Ditto

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

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.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?

[Eric Carlson]

Comment on attachment 411390 [details]
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.

Fixed.

>> Source/WebCore/html/HTMLMediaElement.h:726
>> +    void mediaPlayerGetRawCookies(const URL&, MediaPlayerClient::GetRawCookiesCallback&&) const override;
> 
> Nit. This function can be "final"?

Good point, fixed.

>> Source/WebCore/platform/graphics/MediaPlayer.h:54
>> +#include <wtf/Unexpected.h>
> 
> Not clear why we now need Expected.h and Unexpected.h.

Indeed, unneeded cruft from an earlier version.

>> 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>&&)>;?

Good idea!

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.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.

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.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<>...

Fixed

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.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?

True, I added the check back.

 createAVAssetForURL could be called more than once, I added a guard.

We wait for AVAsset to load metadata before the state machine progresses, so it shouldn't matter if it is NULL for a while.

[Eric Carlson]

Created attachment 411591 [details]
Patch for landing

[EWS]

Committed r268607: <https://trac.webkit.org/changeset/268607>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411591 [details].

[Chris Dumez]

Comment on attachment 411591 [details]
Patch for landing

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

> Source/WebCore/html/HTMLMediaElement.cpp:6962
> +        completionHandler({ });

Cause of rdar://problem/80778666