Add an overlay play button to media controls on android
Created attachment 154087 [details] Patch
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?
Created attachment 154452 [details] Patch
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)
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.
Created attachment 154668 [details] Patch
(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.
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) ?
Created attachment 154858 [details] Patch
(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.
Looks like your latest patch doesn't apply cleanly to top-of-tree.
Created attachment 154998 [details] Patch
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.
Comment on attachment 154998 [details] Patch Thanks. R+ based on my (limited) understanding of this code and Eric's Comment #5.
Comment on attachment 154998 [details] Patch Clearing flags on attachment: 154998 Committed r123908: <http://trac.webkit.org/changeset/123908>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by 92572
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.
Created attachment 155118 [details] Patch
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.
Created attachment 155119 [details] Patch
@dpranke: This comments above explain the problems you were having with the component build too.
Comment on attachment 155119 [details] Patch Clearing flags on attachment: 155119 Committed r123969: <http://trac.webkit.org/changeset/123969>