Bug 58346 - Convert media controls hooks to a client interface
Summary: Convert media controls hooks to a client interface
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 58345
  Show dependency treegraph
 
Reported: 2011-04-12 10:31 PDT by Dimitri Glazkov (Google)
Modified: 2011-04-18 21:28 PDT (History)
4 users (show)

See Also:


Attachments
Patch (18.03 KB, patch)
2011-04-15 18:19 PDT, Steve Lacey
no flags Details | Formatted Diff | Diff
Patch (21.65 KB, patch)
2011-04-15 18:34 PDT, Steve Lacey
no flags Details | Formatted Diff | Diff
Patch (21.48 KB, patch)
2011-04-15 22:58 PDT, Steve Lacey
no flags Details | Formatted Diff | Diff
Patch (50.35 KB, patch)
2011-04-18 11:19 PDT, Steve Lacey
no flags Details | Formatted Diff | Diff
Patch (28.94 KB, patch)
2011-04-18 12:26 PDT, Steve Lacey
no flags Details | Formatted Diff | Diff
Patch (31.33 KB, patch)
2011-04-18 13:58 PDT, Steve Lacey
no flags Details | Formatted Diff | Diff
Patch (29.51 KB, patch)
2011-04-18 14:48 PDT, Steve Lacey
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2011-04-12 10:31:17 PDT
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.
Comment 1 Dimitri Glazkov (Google) 2011-04-12 10:33:26 PDT
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();
Comment 2 Steve Lacey 2011-04-13 13:46:39 PDT
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?
Comment 3 Dimitri Glazkov (Google) 2011-04-13 13:49:38 PDT
(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.
Comment 4 Steve Lacey 2011-04-15 18:19:29 PDT
Created attachment 89898 [details]
Patch
Comment 5 Steve Lacey 2011-04-15 18:34:02 PDT
Created attachment 89900 [details]
Patch
Comment 6 Steve Lacey 2011-04-15 18:34:44 PDT
First patch was missing project.pbxproj change. Fixed in second patch.
Comment 7 Dimitri Glazkov (Google) 2011-04-15 19:48:29 PDT
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
Comment 8 Steve Lacey 2011-04-15 20:49:43 PDT
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.
Comment 9 Steve Lacey 2011-04-15 22:14:48 PDT
Clarification: MediaControlBase or HTMLMediaControlBase? I expect the former as it's not a core html element.
Comment 10 Steve Lacey 2011-04-15 22:58:08 PDT
Created attachment 89917 [details]
Patch
Comment 11 Steve Lacey 2011-04-15 22:58:43 PDT
New patch is a pure name change MediaControlClient -> MediaControlBase.
Comment 12 Dimitri Glazkov (Google) 2011-04-16 09:52:59 PDT
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.
Comment 13 Alexis Menard (darktears) 2011-04-18 07:27:43 PDT
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?
Comment 14 Alexis Menard (darktears) 2011-04-18 07:28:27 PDT
Just a stupid question but this is only for the logic part right?

The styling will still happen is RenderThemeXYZ ?
Comment 15 Steve Lacey 2011-04-18 11:19:08 PDT
Created attachment 90059 [details]
Patch
Comment 16 Steve Lacey 2011-04-18 11:22:05 PDT
(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.
Comment 17 Steve Lacey 2011-04-18 11:22:57 PDT
(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?
Comment 18 Early Warning System Bot 2011-04-18 11:31:28 PDT
Attachment 90059 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8457976
Comment 19 Steve Lacey 2011-04-18 12:26:16 PDT
Created attachment 90069 [details]
Patch
Comment 20 Steve Lacey 2011-04-18 12:27:54 PDT
(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...
Comment 21 Early Warning System Bot 2011-04-18 13:02:36 PDT
Attachment 90069 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8473016
Comment 22 Steve Lacey 2011-04-18 13:17:05 PDT
(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?
Comment 23 Steve Lacey 2011-04-18 13:58:42 PDT
Created attachment 90088 [details]
Patch
Comment 24 Steve Lacey 2011-04-18 14:00:13 PDT
(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.
Comment 25 Steve Lacey 2011-04-18 14:48:32 PDT
Created attachment 90098 [details]
Patch
Comment 26 Steve Lacey 2011-04-18 14:49:14 PDT
(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.
Comment 27 WebKit Commit Bot 2011-04-18 21:26:13 PDT
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: alice.liu@apple.com)
http/tests/media/video-load-twice.html bug 51138 (authors: eric.carlson@apple.com and jamesr@chromium.org)
The commit-queue is continuing to process your patch.
Comment 28 WebKit Commit Bot 2011-04-18 21:28:51 PDT
Comment on attachment 90098 [details]
Patch

Clearing flags on attachment: 90098

Committed r84222: <http://trac.webkit.org/changeset/84222>
Comment 29 WebKit Commit Bot 2011-04-18 21:28:58 PDT
All reviewed patches have been landed.  Closing bug.