[webkit-reviews] review granted: [Bug 135816] [iOS] <video> element requests are missing session cookies; sometimes persistent cookies. : [Attachment 236415] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 135816: [iOS] <video> element requests are missing session cookies;
sometimes persistent cookies.
https://bugs.webkit.org/show_bug.cgi?id=135816

Attachment 236415: Patch
https://bugs.webkit.org/attachment.cgi?id=236415&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=236415&action=review


> Source/WebCore/html/HTMLMediaElement.h:575
> +    virtual bool mediaPlayerGetRawCookies(const URL&, Vector<Cookie>&)
const;

Please add "override" to this declaration.

Curious that "Cookie" worked without a forward declaration. Or will it break
!ENABLE(VIDEO) builds?

> Source/WebCore/platform/graphics/MediaPlayer.h:268
> +    virtual bool mediaPlayerGetRawCookies(const URL&, Vector<Cookie>&) const
{ return false; }

It would be slightly more idiomatic to just return an empty vector as a value,
and to have a notImplemented call in the base class version. Clients don't need
to check at runtime whether their platform implements this.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:718
> +static NSHTTPCookie* toNSHTTPCookie(const Cookie& cookie)

I think that Cookie class in WebCore is a better place for this code.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:725
> +	   NSHTTPCookieExpires: [NSDate
dateWithTimeIntervalSince1970:(cookie.expires / 1000)],

It may be a good idea to pass NSHTTPCookieDiscard, assuming that it doesn't
break anything. But I don't know abut how it interacts with
NSHTTPCookieExpires.

Maybe the best thing to do is to have a FIXME about (1) the need to clarify
this, and (2) move this code to Cookie class once it is generic enough.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:726
> +	   }];

Four too many spaces here, I think.