Bug 145581 – Add basic Media Session support to HTMLMediaElement

• Component: Media
• Status: RESOLVED FIXED

[Matt Rajca]

The Media Session spec adds to HTMLMediaElement:

- a 'kind' attribute representing the intrinsic media category
- a 'session' attribute representing the current media session, if any

[Radar WebKit Bug Importer]

<rdar://problem/21213287>

[Matt Rajca]

Created attachment 254130 [details]
Patch

[Matt Rajca]

Created attachment 254131 [details]
Patch (with build fixes)

[Matt Rajca]

Created attachment 254183 [details]
Patch

[Eric Carlson]

Comment on attachment 254183 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254183&action=review

> Source/WebCore/html/HTMLMediaElement.h:53
> +#if ENABLE(MEDIA_SESSION)
> +#include "MediaSession.h"
> +#endif

I think you can pre-declare "class MediaSession" here and put the include in the .cpp file.

[Matt Rajca]

Comment on attachment 254183 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254183&action=review

>> Source/WebCore/html/HTMLMediaElement.h:53
>> +#endif
> 
> I think you can pre-declare "class MediaSession" here and put the include in the .cpp file.

Done.

[Matt Rajca]

Created attachment 254194 [details]
Patch

[Conrad Shultz]

The EWS failures look like they might be legit, e.g.

DerivedSources/webkitdom/WebKitDOMHTMLMediaElement.h:654:40: error: 'WebKitDOMMediaSession' does not name a type

[Matt Rajca]

The latest patch should fix a few more build errors. I'm going to wait until the EWS finishes before putting this in the commit queue.

(In reply to comment #8)
> The EWS failures look like they might be legit, e.g.
> 
> DerivedSources/webkitdom/WebKitDOMHTMLMediaElement.h:654:40: error:
> 'WebKitDOMMediaSession' does not name a type

[Matt Rajca]

Created attachment 254200 [details]
Patch (speculative GTK build fix)

[Matt Rajca]

Created attachment 254206 [details]
Patch (speculative Windows build fix)

[Darin Adler]

Comment on attachment 254206 [details]
Patch (speculative Windows build fix)

View in context: https://bugs.webkit.org/attachment.cgi?id=254206&action=review

> Source/WebCore/Modules/mediasession/HTMLMediaElementMediaSession.h:32
> +#include <wtf/text/WTFString.h>

Don’t need this.

> Source/WebCore/Modules/mediasession/HTMLMediaElementMediaSession.h:36
> +class HTMLMediaElementMediaSession {

What determines the interface and name of this class? It seems quite strange and I am not sure exactly what it’s for.

I might omit the second occurrence of the word “Media” but I’m not sure enough of context to know if that suggestion is OK.

> Source/WebCore/Modules/mediasession/HTMLMediaElementMediaSession.h:42
> +    static const String& kind(HTMLMediaElement* element) { return element->kind(); }
> +    static void setKind(HTMLMediaElement* element, const String& kind) { return element->setKind(kind); }
> +
> +    static MediaSession* session(HTMLMediaElement*, bool&);
> +    static void setSession(HTMLMediaElement* element, MediaSession* session) { element->setSession(session); }

These should all take HTMLMediaElement&, not HTMLMediaElement*.

> Source/WebCore/html/HTMLMediaElement.h:422
> +    void setSession(MediaSession*);

Might want this to take RefPtr<MediaSession>&& instead, if callers are handing over ownership. If ownership is usually shared, then raw pointer is OK/better.

[Matt Rajca]

Comment on attachment 254206 [details]
Patch (speculative Windows build fix)

View in context: https://bugs.webkit.org/attachment.cgi?id=254206&action=review

>> Source/WebCore/Modules/mediasession/HTMLMediaElementMediaSession.h:32
>> +#include <wtf/text/WTFString.h>
> 
> Don’t need this.

Removed.

>> Source/WebCore/Modules/mediasession/HTMLMediaElementMediaSession.h:36
>> +class HTMLMediaElementMediaSession {
> 
> What determines the interface and name of this class? It seems quite strange and I am not sure exactly what it’s for.
> 
> I might omit the second occurrence of the word “Media” but I’m not sure enough of context to know if that suggestion is OK.

I see it as MediaSession "extensions" on HTMLMediaElement. We do something similar for MediaStream in HTMLMediaElementMediaStream.

I want to keep the second occurrence of the word "Media" in there so it's clear this is referring to the Media Session API rather than a different session.

>> Source/WebCore/Modules/mediasession/HTMLMediaElementMediaSession.h:42
>> +    static void setSession(HTMLMediaElement* element, MediaSession* session) { element->setSession(session); }
> 
> These should all take HTMLMediaElement&, not HTMLMediaElement*.

Per the Media Session spec, we should be able to set the session to null, which we won't be able to do with references.

>> Source/WebCore/html/HTMLMediaElement.h:422
>> +    void setSession(MediaSession*);
> 
> Might want this to take RefPtr<MediaSession>&& instead, if callers are handing over ownership. If ownership is usually shared, then raw pointer is OK/better.

A session can (and often will) be shared between multiple media elements. I'll keep this as is.

[Matt Rajca]

Created attachment 254210 [details]
Patch

[Darin Adler]

Comment on attachment 254206 [details]
Patch (speculative Windows build fix)

View in context: https://bugs.webkit.org/attachment.cgi?id=254206&action=review

>>> Source/WebCore/Modules/mediasession/HTMLMediaElementMediaSession.h:42
>>> +    static void setSession(HTMLMediaElement* element, MediaSession* session) { element->setSession(session); }
>> 
>> These should all take HTMLMediaElement&, not HTMLMediaElement*.
> 
> Per the Media Session spec, we should be able to set the session to null, which we won't be able to do with references.

Then all the function bodies in this class, all of which unconditionally dereference the element pointer, are incorrect.

Setting a session to null would depend on the type MediaSession*, not HTMLMediaElement*.

[Matt Rajca]

(In reply to comment #15)
> Comment on attachment 254206 [details]
> Patch (speculative Windows build fix)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254206&action=review
> 
> >>> Source/WebCore/Modules/mediasession/HTMLMediaElementMediaSession.h:42
> >>> +    static void setSession(HTMLMediaElement* element, MediaSession* session) { element->setSession(session); }
> >> 
> >> These should all take HTMLMediaElement&, not HTMLMediaElement*.
> > 
> > Per the Media Session spec, we should be able to set the session to null, which we won't be able to do with references.
> 
> Then all the function bodies in this class, all of which unconditionally
> dereference the element pointer, are incorrect.
> 
> Setting a session to null would depend on the type MediaSession*, not
> HTMLMediaElement*.

I misread the original feedback. MediaSessions might be null, but HTMLMediaElements will never be null (when jsHTMLMediaElementSession calls these functions it will always pass in a HTMLMediaElement). We could use references, but those aren't compatible with the bindings generated. So I will leave pointers to HTMLMediaElements but add assertions (which is what we do in HTMLMediaElementMediaSource).

[Matt Rajca]

Created attachment 254216 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 254216 [details]
Patch

Clearing flags on attachment: 254216

Committed r185179: <http://trac.webkit.org/changeset/185179>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jon Lee]

Comment on attachment 254216 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254216&action=review

> Source/WebCore/Modules/mediasession/HTMLMediaElementMediaSession.cpp:49
> +    isNull = !session;

Why is this necessary, when the client can simply check the returned variable?

[Jon Lee]

(In reply to comment #16)
> I misread the original feedback. MediaSessions might be null, but
> HTMLMediaElements will never be null (when jsHTMLMediaElementSession calls
> these functions it will always pass in a HTMLMediaElement). We could use
> references, but those aren't compatible with the bindings generated. So I
> will leave pointers to HTMLMediaElements but add assertions (which is what
> we do in HTMLMediaElementMediaSource).

It sounds like we should a new bug filed to improve the bindings generation code to use references instead.

Also, I see other examples of partial interfaces that make use of Supplement<>. Are we avoiding that here for a reason? Or are we abandoning that pattern?

[Eric Carlson]

(In reply to comment #20)
> Comment on attachment 254216 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254216&action=review
> 
> > Source/WebCore/Modules/mediasession/HTMLMediaElementMediaSession.cpp:49
> > +    isNull = !session;
> 
> Why is this necessary, when the client can simply check the returned
> variable?

That is the way our binding generator handles nullable types [1].

[1] http://www.w3.org/TR/WebIDL/#idl-nullable-type