Bug 217739 - [GPU Process] Implement mediaPlayerGetRawCookies
Summary: [GPU Process] Implement mediaPlayerGetRawCookies
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-14 16:33 PDT by Eric Carlson
Modified: 2020-10-16 13:09 PDT (History)
13 users (show)

See Also:


Attachments
Patch (15.34 KB, patch)
2020-10-14 17:01 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch for landing (20.34 KB, patch)
2020-10-16 10:06 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2020-10-14 16:33:56 PDT
Implement mediaPlayerGetRawCookies
Comment 1 Radar WebKit Bug Importer 2020-10-14 16:37:15 PDT
<rdar://problem/70313370>
Comment 2 Eric Carlson 2020-10-14 17:01:16 PDT
Created attachment 411390 [details]
Patch
Comment 3 Peng Liu 2020-10-14 18:10:08 PDT
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"?
Comment 4 youenn fablet 2020-10-15 04:36:18 PDT
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?
Comment 5 Eric Carlson 2020-10-15 20:27:21 PDT
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.
Comment 6 Eric Carlson 2020-10-16 10:06:17 PDT
Created attachment 411591 [details]
Patch for landing
Comment 7 EWS 2020-10-16 13:09:35 PDT
Committed r268607: <https://trac.webkit.org/changeset/268607>

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