Bug 135816 – [iOS] <video> element requests are missing session cookies; sometimes persistent cookies.

• Component: Media
• Status: RESOLVED FIXED

[Jer Noble]

[iOS] <video> element requests are missing session cookies; sometimes persistant cookies.

[Jer Noble]

Created attachment 236415 [details]
Patch

[Jer Noble]

<rdar://problem/17912361>

[Alexey Proskuryakov]

Comment on attachment 236415 [details]
Patch

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/MediaPlayerPrivateAVFoundationObjC.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/MediaPlayerPrivateAVFoundationObjC.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/MediaPlayerPrivateAVFoundationObjC.mm:726
> +        }];

Four too many spaces here, I think.

[Jer Noble]

(In reply to comment #3)
> (From update of attachment 236415 [details])
> 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.

Added.

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

MediaPlayer.h forward declares it, and HTMLMediaElement.h includes MediaPlayer.h.

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

I tried that initially, but forward-declaring Cookie won't work in that case, as this pulls in the constructor and destructor of Vector<Cookie>.  I suspect that's why it was done with this kind of function signature for CookieJar.h as well.

> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:718
> > +static NSHTTPCookie* toNSHTTPCookie(const Cookie& cookie)
> 
> I think that Cookie class in WebCore is a better place for this code.

I agree; I'd like to move it into a platform-specific file in a follow up.

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

I've figured this out, and i'll address it in the final patch. Turns out, it's the /presence/ of that key in the properties dictionary, not it's value, that determines the final result.

> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:726
> > +        }];
> 
> Four too many spaces here, I think.

Ok.

[Jer Noble]

Created attachment 236416 [details]
Patch for landing

[Jer Noble]

Committed r172422: <http://trac.webkit.org/changeset/172422>

[Alexey Proskuryakov]

> MediaPlayer.h forward declares it, and HTMLMediaElement.h includes MediaPlayer.h.

I think that the include of MediaPlayer.h is conditional, which is why I said that it will break no-video builds.

> I tried that initially, but forward-declaring Cookie won't work in that case, as this pulls in the constructor and destructor of Vector<Cookie>.  I suspect that's why it was done with this kind of function signature for CookieJar.h as well.

The right thing to do it simply to move the implementation to .cpp file. Inline virtual functions are frequently harmful (they increase binary code bloat, and can't be inlined anyway).

[Jer Noble]

(In reply to comment #7)
> > MediaPlayer.h forward declares it, and HTMLMediaElement.h includes MediaPlayer.h.
> 
> I think that the include of MediaPlayer.h is conditional, which is why I said that it will break no-video builds.

Ah, I see.  But the conditional wraps the entire header, so !VIDEO builds will be fine.

> > I tried that initially, but forward-declaring Cookie won't work in that case, as this pulls in the constructor and destructor of Vector<Cookie>.  I suspect that's why it was done with this kind of function signature for CookieJar.h as well.
> 
> The right thing to do it simply to move the implementation to .cpp file. Inline virtual functions are frequently harmful (they increase binary code bloat, and can't be inlined anyway).

Ok. All the MediaPlayerClient are inline-virtual, so moving them all into the .cpp file (or making them pure virtual, since there's only one subclass) we can do in a follow up.

[Alexey Proskuryakov]

> But the conditional wraps the entire header, so !VIDEO builds will be fine.

Indeed, I misread it.