Bug 58346 – Convert media controls hooks to a client interface

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Dimitri Glazkov (Google)]

To facilitate cleaner separation of concerns and allow ports build their own DOM UI (rather than hacking it in using RenderTheme), we should create a MediaControlsClient interface, which is consumed by HTMLMediaElement. The implementors of the interface can then supply their own implementation of media controls.

[Dimitri Glazkov (Google)]

The hooks mentioned above are all the methods on MediaControlRootElement that HTMLMediaElement uses to signal a change of the state:


    void show();
    void hide();
    void makeOpaque();
    void makeTransparent();

    void reset();

    void playbackProgressed();
    void playbackStarted();
    void playbackStopped();

    void changedMute();
    void changedVolume();

    void enteredFullscreen();
    void exitedFullscreen();

    void reportedError();
    void changedNetworkState();
    void loadedMetadata();
    void changedClosedCaptionsVisibility();

    void showVolumeSlider();
    void updateTimeDisplay();

[Steve Lacey]

Am looking at taking this on.

From a quick look, it seems like we could (which is basically reiterating Dimitri's comment):

1) Create a MediaControlsClient interface.
2) Derive MediaControls from that and rename to MediaControlRootElement.
3) Have MediaElement use that.

This seems fairly simple and would keep everything working. Ports can then implement their own MediaControlClient instead of MediaControlRootElement as and when they wish.

Any gotchas?

[Dimitri Glazkov (Google)]

(In reply to comment #2)
> Am looking at taking this on.
> 
> From a quick look, it seems like we could (which is basically reiterating Dimitri's comment):
> 
> 1) Create a MediaControlsClient interface.
> 2) Derive MediaControls from that and rename to MediaControlRootElement.
> 3) Have MediaElement use that.
> 
> This seems fairly simple and would keep everything working. Ports can then implement their own MediaControlClient instead of MediaControlRootElement as and when they wish.
> 
> Any gotchas?

This is a reasonable first step.

[Steve Lacey]

Created attachment 89898 [details]
Patch

[Steve Lacey]

Created attachment 89900 [details]
Patch

[Steve Lacey]

First patch was missing project.pbxproj change. Fixed in second patch.

[Dimitri Glazkov (Google)]

Comment on attachment 89900 [details]
Patch

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

> Source/WebCore/html/shadow/MediaControlClient.h:38
> +class MediaControlClient : public HTMLDivElement {

This is interesting. If it's a subclass of an HTMLDivElement, it's probably not a client. I was imagining MediaControlClient to not be a subclass of anything, but maybe that would be too convoluted.

I think what you have is fine, but you need to get rid of the Client designation. This is a base class, and the convention in WebKit is to use a Base suffix. So this would be an HTMLMediaControlBase.h

[Steve Lacey]

Thanks for the quick review!

That was what I thought too. But the issue became one that ideally:

1) The HTMLMediaElement would maintain a reference to the controls interface on the object.
2) The object is added to the shadow root, which maintains a reference.

Who owns what then becomes tricky and you'd end up splitting into various objects and lifetimes become an issue.

It became simpler just to think of the interface as a base class.

Anyhow - yup! I should have changed the name :-)  Will fix on Monday.

[Steve Lacey]

Clarification: MediaControlBase or HTMLMediaControlBase? I expect the former as it's not a core html element.

[Steve Lacey]

Created attachment 89917 [details]
Patch

[Steve Lacey]

New patch is a pure name change MediaControlClient -> MediaControlBase.

[Dimitri Glazkov (Google)]

Comment on attachment 89917 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:22965
> +				1F0D6E0E13590DE700236592 /* MediaControlBase.h in Headers */,

The XCode changes look like they haven't been sorted. Can you run sort-XCode-project-files on this?

> Source/WebCore/html/shadow/MediaControlBase.h:34
> +#include <HTMLDivElement.h>
> +#include <HTMLMediaElement.h>
> +#include <HTMLNames.h>

"", not <>

> Source/WebCore/html/shadow/MediaControlBase.h:38
> +class MediaControlBase : public HTMLDivElement {

I am sorry about flip-flopping here, but looking at callsites, it's clear that they are working with an interface, not a base class. So we should call this new thing MediaControls, not MediaControlBase.

> Source/WebCore/html/shadow/MediaControlBase.h:42
> +    // This function is to be implemented in your platform-specific media

port-specific.

> Source/WebCore/html/shadow/MediaControlBase.h:73
> +    MediaControlBase(HTMLMediaElement* mediaElement)
> +        : HTMLDivElement(HTMLNames::divTag, mediaElement->document()) { }

I know you want to keep this change to only have a header file, but you could save a couple of header includes if you just have a source file with it.

> Source/WebCore/html/shadow/MediaControlBase.h:81
> +#endif

extra break after first endif would be helpful.

> Source/WebCore/html/shadow/MediaControlRootElement.h:28
> +#ifndef MediaControlRootElement_h
> +#define MediaControlRootElement_h

Ha! Thanks for noticing this one.

[Alexis Menard (darktears)]

Comment on attachment 89917 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.h:338
> +

Extra spaces?

[Alexis Menard (darktears)]

Just a stupid question but this is only for the logic part right?

The styling will still happen is RenderThemeXYZ ?

[Steve Lacey]

Created attachment 90059 [details]
Patch

[Steve Lacey]

(In reply to comment #12)
> (From update of attachment 89917 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89917&action=review
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:22965
> > +				1F0D6E0E13590DE700236592 /* MediaControlBase.h in Headers */,
> 
> The XCode changes look like they haven't been sorted. Can you run sort-XCode-project-files on this?

Done. Though this resulted in a lot of changes! Am presuming this hasn't been run for a while and that the script did the right thing...

> 
> > Source/WebCore/html/shadow/MediaControlBase.h:34
> > +#include <HTMLDivElement.h>
> > +#include <HTMLMediaElement.h>
> > +#include <HTMLNames.h>
> 
> "", not <>

Done.

> 
> > Source/WebCore/html/shadow/MediaControlBase.h:38
> > +class MediaControlBase : public HTMLDivElement {
> 
> I am sorry about flip-flopping here, but looking at callsites, it's clear that they are working with an interface, not a base class. So we should call this new thing MediaControls, not MediaControlBase.

Done.

> 
> > Source/WebCore/html/shadow/MediaControlBase.h:42
> > +    // This function is to be implemented in your platform-specific media
> 
> port-specific.

Done.

> 
> > Source/WebCore/html/shadow/MediaControlBase.h:73
> > +    MediaControlBase(HTMLMediaElement* mediaElement)
> > +        : HTMLDivElement(HTMLNames::divTag, mediaElement->document()) { }
> 
> I know you want to keep this change to only have a header file, but you could save a couple of header includes if you just have a source file with it.

Done.

> 
> > Source/WebCore/html/shadow/MediaControlBase.h:81
> > +#endif
> 
> extra break after first endif would be helpful.

Done. Was just following the style in other includes...

> 
> > Source/WebCore/html/shadow/MediaControlRootElement.h:28
> > +#ifndef MediaControlRootElement_h
> > +#define MediaControlRootElement_h
> 
> Ha! Thanks for noticing this one.

[Steve Lacey]

(In reply to comment #13)
> (From update of attachment 89917 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89917&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.h:338
> > +
> 
> Extra spaces?

My change removed some spurious spaces due to me adding an new member and then taking it away. Are the spaces needed?

[Early Warning System Bot]

Attachment 90059 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8457976

[Steve Lacey]

Created attachment 90069 [details]
Patch

[Steve Lacey]

(In reply to comment #18)
> Attachment 90059 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/8457976

Should be fixed in the followup patch. Messed up the pbxproj. Nice catch by the bots...

[Early Warning System Bot]

Attachment 90069 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8473016

[Steve Lacey]

(In reply to comment #21)
> Attachment 90069 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/8473016


Looks like MediaControls.cpp is not being pulled into the qt build. I can't see any missing makefile. Is there another file where this should be listed? A grep over webkit for another doesn't show any missing change.

Any clues?

[Steve Lacey]

Created attachment 90088 [details]
Patch

[Steve Lacey]

(In reply to comment #22)
> (In reply to comment #21)
> > Attachment 90069 [details] [details] did not build on qt:
> > Build output: http://queues.webkit.org/results/8473016
> 
> 
> Looks like MediaControls.cpp is not being pulled into the qt build. I can't see any missing makefile. Is there another file where this should be listed? A grep over webkit for another doesn't show any missing change.
> 
> Any clues?

Answering my own question: missed WebCore.pro.

[Steve Lacey]

Created attachment 90098 [details]
Patch

[Steve Lacey]

(In reply to comment #25)
> Created an attachment (id=90098) [details]
> Patch

No change other than a merge with head to try and get the qt bot to apply the patch successfully.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 90098 [details]:

http/tests/misc/favicon-loads-with-icon-loading-override.html bug 58412 (author: [e-mail])
http/tests/media/video-load-twice.html bug 51138 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 90098 [details]
Patch

Clearing flags on attachment: 90098

Committed r84222: <http://trac.webkit.org/changeset/84222>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.