Bug 145581

Summary: Add basic Media Session support to HTMLMediaElement
Product: WebKit Reporter: Matt Rajca <mrajca>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, conrad_shultz, eric.carlson, jer.noble, jonlee, mrajca, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 145411    
Attachments:
Description Flags
Patch
none
Patch (with build fixes)
none
Patch
none
Patch
none
Patch (speculative GTK build fix)
none
Patch (speculative Windows build fix)
darin: review-, darin: commit-queue-
Patch
none
Patch none

Description Matt Rajca 2015-06-02 17:43:06 PDT
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
Comment 1 Radar WebKit Bug Importer 2015-06-02 17:44:04 PDT
<rdar://problem/21213287>
Comment 2 Matt Rajca 2015-06-02 18:00:44 PDT
Created attachment 254130 [details]
Patch
Comment 3 Matt Rajca 2015-06-02 18:07:58 PDT
Created attachment 254131 [details]
Patch (with build fixes)
Comment 4 Matt Rajca 2015-06-03 09:54:19 PDT
Created attachment 254183 [details]
Patch
Comment 5 Eric Carlson 2015-06-03 10:07:56 PDT
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.
Comment 6 Matt Rajca 2015-06-03 12:20:32 PDT
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.
Comment 7 Matt Rajca 2015-06-03 12:22:37 PDT
Created attachment 254194 [details]
Patch
Comment 8 Conrad Shultz 2015-06-03 12:31:40 PDT
The EWS failures look like they might be legit, e.g.

DerivedSources/webkitdom/WebKitDOMHTMLMediaElement.h:654:40: error: 'WebKitDOMMediaSession' does not name a type
Comment 9 Matt Rajca 2015-06-03 12:33:51 PDT
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
Comment 10 Matt Rajca 2015-06-03 13:10:18 PDT
Created attachment 254200 [details]
Patch (speculative GTK build fix)
Comment 11 Matt Rajca 2015-06-03 13:57:26 PDT
Created attachment 254206 [details]
Patch (speculative Windows build fix)
Comment 12 Darin Adler 2015-06-03 14:20:54 PDT
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.
Comment 13 Matt Rajca 2015-06-03 14:35:48 PDT
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.
Comment 14 Matt Rajca 2015-06-03 14:36:12 PDT
Created attachment 254210 [details]
Patch
Comment 15 Darin Adler 2015-06-03 14:38:24 PDT
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*.
Comment 16 Matt Rajca 2015-06-03 15:10:37 PDT
(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).
Comment 17 Matt Rajca 2015-06-03 15:21:32 PDT
Created attachment 254216 [details]
Patch
Comment 18 WebKit Commit Bot 2015-06-03 16:42:18 PDT
Comment on attachment 254216 [details]
Patch

Clearing flags on attachment: 254216

Committed r185179: <http://trac.webkit.org/changeset/185179>
Comment 19 WebKit Commit Bot 2015-06-03 16:42:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Jon Lee 2015-06-03 18:10:48 PDT
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?
Comment 21 Jon Lee 2015-06-03 18:12:19 PDT
(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?
Comment 22 Eric Carlson 2015-06-03 20:14:23 PDT
(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