Implement mediaPlayerGetRawCookies
<rdar://problem/70313370>
Created attachment 411390 [details] Patch
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 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 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.
Created attachment 411591 [details] Patch for landing
Committed r268607: <https://trac.webkit.org/changeset/268607> All reviewed patches have been landed. Closing bug and clearing flags on attachment 411591 [details].
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