Add an overlay play button to media controls on android

• Component: New Bugs
• Status: RESOLVED FIXED

[Min Qin]

Add an overlay play button to media controls on android

[Min Qin]

Created attachment 154087 [details]
Patch

[Eric Carlson]

Comment on attachment 154087 [details]
Patch

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

> Source/WebCore/css/mediaControls.css:86
> +    display: -webkit-box;

Shouldn't this be display:none, and then each port that wants to display it can override?

> Source/WebCore/html/shadow/MediaControlElements.h:283
> +    MediaControlOverlayPlayButtonElement(Document*);

All single-argument constructors should be marked "explicit" so the compiler doesn't try to implicitly create a MediaControlCromiumEnclosureElement from a Document*.

> Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:78
> +#if ENABLE(MEDIA_OVERLAY_PLAY_BUTTON)
> +MediaControlOverlayEnclosureElement::MediaControlOverlayEnclosureElement(Document* document)
> +    : MediaControlChromiumEnclosureElement(document)
> +{
> +}
> +

Is there any reason to not do this by subclassing MediaControlRootElementChromium instead of adding compile flags?

> Source/WebCore/rendering/RenderMediaControlsChromium.cpp:116
> +#if ENABLE(MEDIA_OVERLAY_PLAY_BUTTON)
> +static bool paintMediaOverlayPlayButton(RenderObject* object, const PaintInfo& paintInfo, const IntRect& rect)
> +{

Again, is there any reason to not do this by subclassing?

[Min Qin]

Created attachment 154452 [details]
Patch

[Min Qin]

Removed the ENABLE(MEDIA_OVERLAY_PLAY_BUTTON) flag, PTAL.

(In reply to comment #2)
> (From update of attachment 154087 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154087&action=review
> 
> > Source/WebCore/css/mediaControls.css:86
> > +    display: -webkit-box;
> 
> Shouldn't this be display:none, and then each port that wants to display it can override?

Done. 

> 
> > Source/WebCore/html/shadow/MediaControlElements.h:283
> > +    MediaControlOverlayPlayButtonElement(Document*);
> 
> All single-argument constructors should be marked "explicit" so the compiler doesn't try to implicitly create a MediaControlCromiumEnclosureElement from a Document*.
> 
> > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:78
> > +#if ENABLE(MEDIA_OVERLAY_PLAY_BUTTON)
> > +MediaControlOverlayEnclosureElement::MediaControlOverlayEnclosureElement(Document* document)
> > +    : MediaControlChromiumEnclosureElement(document)
> > +{
> > +}
> > +
> 
> Is there any reason to not do this by subclassing MediaControlRootElementChromium instead of adding compile flags?

Refactored MediaControlRootElementChromium and added a subclass MediaControlRootElementChromiumAndroid to implement the functionality of  the overlay play button.

> 
> > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:116
> > +#if ENABLE(MEDIA_OVERLAY_PLAY_BUTTON)
> > +static bool paintMediaOverlayPlayButton(RenderObject* object, const PaintInfo& paintInfo, const IntRect& rect)
> > +{
> 
> Again, is there any reason to not do this by subclassing?

This class is full of static functions. So subclassing it does not help too much. I removed the "#if ENABLE(MEDIA_OVERLAY_PLAY_BUTTON)" in the file, the code will only be hit in the android port (called from RenderThemeChromiumAndroid)

[Eric Carlson]

Comment on attachment 154452 [details]
Patch

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

This looks great with the minor nit noted. Thank you for taking the time to change your approach and then refactor, this is a huge improvement!

> Source/WebCore/css/mediaControlsChromiumAndroid.css:113
> +audio::-webkit-media-controls-overlay-play-button {
> +    display: none;
> +}

This should be unnecessary.

[Min Qin]

Created attachment 154668 [details]
Patch

[Min Qin]

(In reply to comment #5)
> (From update of attachment 154452 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154452&action=review
> 
> This looks great with the minor nit noted. Thank you for taking the time to change your approach and then refactor, this is a huge improvement!
> 
> > Source/WebCore/css/mediaControlsChromiumAndroid.css:113
> > +audio::-webkit-media-controls-overlay-play-button {
> > +    display: none;
> > +}
> 
> This should be unnecessary.

Done, removed.

[Adam Barth]

Comment on attachment 154668 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeChromiumAndroid.cpp:92
> +#if ENABLE(VIDEO) && ENABLE(MEDIA_OVERLAY_PLAY_BUTTON)

I thought you removed ENABLE(MEDIA_OVERLAY_PLAY_BUTTON) ?

[Min Qin]

Created attachment 154858 [details]
Patch

[Min Qin]

(In reply to comment #8)
> (From update of attachment 154668 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154668&action=review
> 
> > Source/WebCore/rendering/RenderThemeChromiumAndroid.cpp:92
> > +#if ENABLE(VIDEO) && ENABLE(MEDIA_OVERLAY_PLAY_BUTTON)
> 
> I thought you removed ENABLE(MEDIA_OVERLAY_PLAY_BUTTON) ?

Sorry, missed it. 
Yes, ENABLE(MEDIA_OVERLAY_PLAY_BUTTON) was removed.

[Adam Barth]

Looks like your latest patch doesn't apply cleanly to top-of-tree.

[Min Qin]

Created attachment 154998 [details]
Patch

[Min Qin]

Rebased the patch to the top of the tree, resolved some of the conflicts there.

(In reply to comment #11)
> Looks like your latest patch doesn't apply cleanly to top-of-tree.

[Adam Barth]

Comment on attachment 154998 [details]
Patch

Thanks.  R+ based on my (limited) understanding of this code and Eric's Comment #5.

[WebKit Review Bot]

Comment on attachment 154998 [details]
Patch

Clearing flags on attachment: 154998

Committed r123908: <http://trac.webkit.org/changeset/123908>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 92572

[Peter Kasting]

This broke the Chromium-Linux canary compile.  Sample build log available at http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux%20%28dbg%29/builds/9510/steps/compile/logs/stdio .

The relevant info is:

out/Debug/../../third_party/gold/gold64: error: out/Debug/obj.target/third_party/WebKit/Source/WebCore/WebCore.gyp/libwebcore_html.a(out/Debug/obj.target/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../../../webcore_html/third_party/WebKit/Source/WebCore/html/shadow/MediaControlRootElementChromiumAndroid.o): multiple definition of 'WebCore::MediaControls::create(WebCore::Document*)'
out/Debug/../../third_party/gold/gold64: out/Debug/obj.target/third_party/WebKit/Source/WebCore/WebCore.gyp/libwebcore_html.a(out/Debug/obj.target/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../../../webcore_html/third_party/WebKit/Source/WebCore/html/shadow/MediaControlRootElementChromium.o): previous definition here

Looks to me like the newly-added MediaControlRootElementChromiumAndroid.cpp file is not getting properly excluded from the Linux build.

Rolling this out on bug 92572.

[Min Qin]

Created attachment 155118 [details]
Patch

[Min Qin]

html/ happens to be one of the directory that *Android.cpp files are not excluded.  So that's why my android build works fine while the linux build is broken.

Updated the webcore.gyp file to exclude all the *Android.cpp files from html/ directory for non-android build.


(In reply to comment #18)
> This broke the Chromium-Linux canary compile.  Sample build log available at http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux%20%28dbg%29/builds/9510/steps/compile/logs/stdio .
> 
> The relevant info is:
> 
> out/Debug/../../third_party/gold/gold64: error: out/Debug/obj.target/third_party/WebKit/Source/WebCore/WebCore.gyp/libwebcore_html.a(out/Debug/obj.target/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../../../webcore_html/third_party/WebKit/Source/WebCore/html/shadow/MediaControlRootElementChromiumAndroid.o): multiple definition of 'WebCore::MediaControls::create(WebCore::Document*)'
> out/Debug/../../third_party/gold/gold64: out/Debug/obj.target/third_party/WebKit/Source/WebCore/WebCore.gyp/libwebcore_html.a(out/Debug/obj.target/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../../../webcore_html/third_party/WebKit/Source/WebCore/html/shadow/MediaControlRootElementChromium.o): previous definition here
> 
> Looks to me like the newly-added MediaControlRootElementChromiumAndroid.cpp file is not getting properly excluded from the Linux build.
> 
> Rolling this out on bug 92572.

[Min Qin]

Created attachment 155119 [details]
Patch

[Adam Barth]

@dpranke: This comments above explain the problems you were having with the component build too.

[WebKit Review Bot]

Comment on attachment 155119 [details]
Patch

Clearing flags on attachment: 155119

Committed r123969: <http://trac.webkit.org/changeset/123969>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.