Summary: | Add basic Media Session support to HTMLMediaElement | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Rajca <mrajca> | ||||||||||||||||||
Component: | Media | Assignee: | 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
Matt Rajca
2015-06-02 17:43:06 PDT
Created attachment 254130 [details]
Patch
Created attachment 254131 [details]
Patch (with build fixes)
Created attachment 254183 [details]
Patch
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 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. Created attachment 254194 [details]
Patch
The EWS failures look like they might be legit, e.g. DerivedSources/webkitdom/WebKitDOMHTMLMediaElement.h:654:40: error: 'WebKitDOMMediaSession' does not name a type 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 Created attachment 254200 [details]
Patch (speculative GTK build fix)
Created attachment 254206 [details]
Patch (speculative Windows build fix)
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 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. Created attachment 254210 [details]
Patch
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*. (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). Created attachment 254216 [details]
Patch
Comment on attachment 254216 [details] Patch Clearing flags on attachment: 254216 Committed r185179: <http://trac.webkit.org/changeset/185179> All reviewed patches have been landed. Closing bug. 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? (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? (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 |