[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 bugzilla-daemon at webkit.org
Mon Aug 11 17:20:10 PDT 2014

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.

Attachment 236415: Patch

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

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.

> +static NSHTTPCookie* toNSHTTPCookie(const Cookie& cookie)

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

> +	   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

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.

> +	   }];

Four too many spaces here, I think.

More information about the webkit-reviews mailing list