[webkit-reviews] review granted: [Bug 88871] Analyze duplication of code for video controls shadow dom & rendering : [Attachment 172710] Refactoring MediaControls - will fail chromebot, since it also needs a chromium patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Silvia Pfeiffer
<silviapf at chromium.org>'s request for review:
Bug 88871: Analyze duplication of code for video controls shadow dom &
rendering
https://bugs.webkit.org/show_bug.cgi?id=88871

Attachment 172710: Refactoring MediaControls - will fail chromebot, since it
also needs a chromium patch
https://bugs.webkit.org/attachment.cgi?id=172710&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=172710&action=review


Thanks for taking this on Silvia!

> Source/WebCore/ChangeLog:78
> +	   (WebCore::MediaControls::show):
> +	   (WebCore::MediaControls::hide):
> +	   (WebCore::MediaControls::makeOpaque):
> +	   (WebCore::MediaControls::makeTransparent):
> +	   (WebCore::MediaControls::shouldHideControls):
> +	   (WebCore::MediaControls::bufferingProgressed):
> +	   (WebCore::MediaControls::playbackStarted):
> +	   (WebCore::MediaControls::playbackProgressed):
> +	   (WebCore::MediaControls::playbackStopped):
> +	   (WebCore::MediaControls::updateCurrentTimeDisplay):
> +	   (WebCore::MediaControls::showVolumeSlider):
> +	   (WebCore::MediaControls::changedMute):
> +	   (WebCore::MediaControls::changedVolume):
> +	   (WebCore::MediaControls::changedClosedCaptionsVisibility):
> +	   (WebCore::MediaControls::enteredFullscreen):
> +	   (WebCore::MediaControls::exitedFullscreen):
> +	   (WebCore::MediaControls::defaultEventHandler):
> +	   (WebCore::MediaControls::hideFullscreenControlsTimerFired):
> +	   (WebCore::MediaControls::startHideFullscreenControlsTimer):
> +	   (WebCore::MediaControls::stopHideFullscreenControlsTimer):
> +	   (WebCore::MediaControls::shadowPseudoId):
> +	   (WebCore::MediaControls::containsRelatedTarget):
> +	   (WebCore::MediaControls::createTextTrackDisplay):
> +	   (WebCore::MediaControls::showTextTrackDisplay):
> +	   (WebCore::MediaControls::hideTextTrackDisplay):
> +	   (WebCore::MediaControls::updateTextTrackDisplay):

Nit: I don't think it is necessary to include these in the ChangeLog.

> Source/WebCore/ChangeLog:105
> +	   (WebCore::MediaControlsApple::MediaControlsApple):
> +	   (WebCore::MediaControls::create):
> +	   (WebCore::MediaControlsApple::createControls):
> +	   (WebCore::MediaControlsApple::setMediaController):
> +	   (WebCore::MediaControlsApple::hide):
> +	   (WebCore::MediaControlsApple::makeTransparent):
> +	   (WebCore::MediaControlsApple::reset):
> +	   (WebCore::MediaControlsApple::updateCurrentTimeDisplay):
> +	   (WebCore::MediaControlsApple::reportedError):
> +	   (WebCore::MediaControlsApple::updateStatusDisplay):
> +	   (WebCore::MediaControlsApple::loadedMetadata):
> +	   (WebCore::MediaControlsApple::changedMute):
> +	   (WebCore::MediaControlsApple::changedVolume):
> +	   (WebCore::MediaControlsApple::enteredFullscreen):
> +	   (WebCore::MediaControlsApple::exitedFullscreen):
> +	   (WebCore::MediaControlsApple::showVolumeSlider):

Ditto.

> Source/WebCore/ChangeLog:127
> +	  
(WebCore::MediaControlChromiumEnclosureElement::MediaControlChromiumEnclosureEl
ement):
> +	   (WebCore::MediaControlChromiumEnclosureElement::displayType):
> +	  
(WebCore::MediaControlPanelEnclosureElement::MediaControlPanelEnclosureElement)
:
> +	   (WebCore::MediaControlPanelEnclosureElement::create):
> +	   (WebCore::MediaControlPanelEnclosureElement::shadowPseudoId):
> +	   (WebCore::MediaControlsChromium::MediaControlsChromium):
> +	   (WebCore::MediaControls::create):
> +	   (WebCore::MediaControlsChromium::createControls):
> +	   (WebCore::MediaControlsChromium::initializeControls):
> +	   (WebCore::MediaControlsChromium::setMediaController):
> +	   (WebCore::MediaControlsChromium::reset):
> +	   (WebCore::MediaControlsChromium::playbackStarted):
> +	   (WebCore::MediaControlsChromium::updateCurrentTimeDisplay):
> +	   (WebCore::MediaControlsChromium::changedMute):
> +	   (WebCore::MediaControlsChromium::createTextTrackDisplay):

Ditto.

> Source/WebCore/ChangeLog:145
> +	   (WebCore):
> +	  
(WebCore::MediaControlOverlayEnclosureElement::MediaControlOverlayEnclosureElem
ent):
> +	   (WebCore::MediaControlOverlayEnclosureElement::create):
> +	   (WebCore::MediaControlOverlayEnclosureElement::shadowPseudoId):
> +	  
(WebCore::MediaControlsChromiumAndroid::MediaControlsChromiumAndroid):
> +	   (WebCore::MediaControls::create):
> +	   (WebCore::MediaControlsChromiumAndroid::createControls):
> +	   (WebCore::MediaControlsChromiumAndroid::setMediaController):
> +	   (WebCore::MediaControlsChromiumAndroid::playbackStarted):
> +	   (WebCore::MediaControlsChromiumAndroid::playbackStopped):

Ditto.

> Source/WebCore/html/shadow/MediaControls.cpp:224
> +    // Allow the theme to format the time.

Nit: I know you didn't add this comment, but it is unnecessary as is only
states what is obvious from examining the code.

> Source/WebCore/html/shadow/MediaControls.cpp:299
> +    if (event->type() == eventNames().mouseoverEvent) {
> +	   if (!containsRelatedTarget(event)) {
> +	       m_isMouseOverControls = true;
> +	       if (!m_mediaController->canPlay()) {
> +		   makeOpaque();
> +		   if (shouldHideControls())
> +		       startHideFullscreenControlsTimer();
> +	       }
> +	   }
> +    } else if (event->type() == eventNames().mouseoutEvent) {
> +	   if (!containsRelatedTarget(event)) {
> +	       m_isMouseOverControls = false;
> +	       stopHideFullscreenControlsTimer();
> +	   }
> +    } else if (event->type() == eventNames().mousemoveEvent) {
> +	   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();
> +	   }
> +    }

Nit: you didn't create this either, but early returns would make this much
easier to follow.