Bug 58346

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();

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?

(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.

Created attachment 89898 [details] Patch

Created attachment 89900 [details] Patch

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

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

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.

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

Created attachment 89917 [details] Patch

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

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 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?

Just a stupid question but this is only for the logic part right? The styling will still happen is RenderThemeXYZ ?

Created attachment 90059 [details] Patch

(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.

(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?

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

Created attachment 90069 [details] Patch

(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...

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

(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?

Created attachment 90088 [details] Patch

(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.

Created attachment 90098 [details] Patch

(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.

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 on attachment 90098 [details] Patch Clearing flags on attachment: 90098 Committed r84222: <http://trac.webkit.org/changeset/84222>

All reviewed patches have been landed. Closing bug.