Bug 88818

Created attachment 147000 [details] Full patch for layout tests

Created attachment 147003 [details] Use this for review Fullscreen related changes only

Comment on attachment 147003 [details] Use this for review it looks like this was created largely by copying and pasting chunks from MediaControlRootElement.cpp. This makes sense in as much as the patch adds functionality that is already in the media controls root used by all of the other ports, but it also means that the two classes are more and more similar without actually sharing any code. We now have two classes that perform exactly the same function and that are called from cross platform code, but that share by copy and paste instead of by using a common base class. I don't know why Steve decided to create this file by cloning MediaControlRootElement instead of factoring the common code into a base class, and clearly I should not have r+d the original change, but I think it is time to reevaluate. Instead of making this mess worse, how hard would it be to add these fullscreen changes by factoring the code from MediaControlRootElement into a base class?

(In reply to comment #3) > (From update of attachment 147003 [details]) > I don't know why Steve decided to create this file by cloning MediaControlRootElement instead of > factoring the common code into a base class, and clearly I should not have r+d the original change, but >I think it is time to reevaluate. Instead of making this mess worse, how hard would it be to add these > fullscreen changes by factoring the code from MediaControlRootElement into a base class? Actually, today, a patch landed that added fullscreen support for Android, so half this patch has already landed. Also, if you look at the individual functions, some of them are actually quite different between the two files. I understand where you're coming from and I am sympathetic, but I'd prefer doing it after this patch. I am supposed to land this this week so it can make it into Chrome 21. So, I'd prefer to do the analysis and clean-up afterwards.

Created a new bug for the code restructing issue, see bug 88871 .

Created attachment 147096 [details] adapted to work with recent fullscreen commits

Created attachment 147098 [details] Use this for review Substantial change with recent comment, see bug 88266.

Comment on attachment 147098 [details] Use this for review View in context: https://bugs.webkit.org/attachment.cgi?id=147098&action=review This generally looks OK, but I would like to see a version with corrected ChangeLogs. > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:365 > + // When we get a mouse move in fullscreen mode, show the media controls, and start a timer > + // that will hide the media controls after a 3 seconds without a mouse move. Copy-pasta: you redefined timeWithoutMouseMovementBeforeHidingControls above to 2 seconds. > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:277 > - DEFINE_STATIC_LOCAL(Image*, mediaFullscreen, (platformResource("mediaFullscreen"))); > - return paintMediaButton(paintInfo.context, rect, mediaFullscreen); > + static Image* mediaFullscreenButton = platformResource("mediaplayerFullscreen"); Why the change from "DEFINE_STATIC_LOCAL" to just "static"? > Source/WebCore/rendering/RenderThemeChromiumSkia.cpp:462 > -String formatMediaControlsTime(float time) const > +String formatMediaControlsTime(float time) Is it not possible for RenderMediaControlsChromium::formatMediaControlsTime to be const?

(In reply to comment #8) > (From update of attachment 147098 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147098&action=review > > This generally looks OK, but I would like to see a version with corrected ChangeLogs. Actually, I was hasty with this diff, since it includes more than it should, in particular the weird changes to the ChangeLogs and the Skia changes (the latter of which need to go into bug 88724 where the skia builds failed. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:365 > > + // When we get a mouse move in fullscreen mode, show the media controls, and start a timer > > + // that will hide the media controls after a 3 seconds without a mouse move. > > Copy-pasta: you redefined timeWithoutMouseMovementBeforeHidingControls above to 2 seconds. Thanks for noticing! > > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:277 > > - DEFINE_STATIC_LOCAL(Image*, mediaFullscreen, (platformResource("mediaFullscreen"))); > > - return paintMediaButton(paintInfo.context, rect, mediaFullscreen); > > + static Image* mediaFullscreenButton = platformResource("mediaplayerFullscreen"); > > Why the change from "DEFINE_STATIC_LOCAL" to just "static"? All images are now pulled in in this way - the previous way doesn't work any more. > > Source/WebCore/rendering/RenderThemeChromiumSkia.cpp:462 > > -String formatMediaControlsTime(float time) const > > +String formatMediaControlsTime(float time) > > Is it not possible for RenderMediaControlsChromium::formatMediaControlsTime to be const? It's not a class member function, just a static helper so can't declare it in this way. Hmm... I missed the "static". Will fix.

Comment on attachment 147096 [details] adapted to work with recent fullscreen commits Attachment 147096 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12938788

Created attachment 147568 [details] added a fullscreen transparent background for audio (full patch)

Created attachment 147569 [details] Use this for review This patch plus the changelog will be what I'm planning to land.

Comment on attachment 147569 [details] Use this for review View in context: https://bugs.webkit.org/attachment.cgi?id=147569&action=review > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:365 > + // When we get a mouse move in fullscreen mode, show the media controls, and start a timer > + // that will hide the media controls after a 3 seconds without a mouse move. Nit: you hide the controls after 2 seconds, not 3. > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:369 > + if (m_isFullscreen) { > + // When we get a mouse move in fullscreen mode, show the media controls, and start a timer > + // that will hide the media controls after a 3 seconds without a mouse move. > + makeOpaque(); > + if (shouldHideControls()) > + startHideFullscreenControlsTimer(); > + } Does it matter that you may call startHideFullscreenControlsTimer() multiple times?

(In reply to comment #13) > (From update of attachment 147569 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147569&action=review > > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:365 > > + // When we get a mouse move in fullscreen mode, show the media controls, and start a timer > > + // that will hide the media controls after a 3 seconds without a mouse move. > > Nit: you hide the controls after 2 seconds, not 3. DONE. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:369 > > + if (m_isFullscreen) { > > + // When we get a mouse move in fullscreen mode, show the media controls, and start a timer > > + // that will hide the media controls after a 3 seconds without a mouse move. > > + makeOpaque(); > > + if (shouldHideControls()) > > + startHideFullscreenControlsTimer(); > > + } > > Does it matter that you may call startHideFullscreenControlsTimer() multiple times? Every time that the mouse is moved, the timer has to be reset. So, no, it's intended. (Also note that MediaControlRootElement.cpp does the same thing.)

Created attachment 147687 [details] patch for cq

Comment on attachment 147687 [details] patch for cq Clearing flags on attachment: 147687 Committed r120414: <http://trac.webkit.org/changeset/120414>

All reviewed patches have been landed. Closing bug.