Bug 88871

Note in particular it may be better to inherit MediaControlRootElementChromium from MediaControlRootElement rather than from MediaControls .

I've analyzed the duplication of code. There is much duplication not only between the MediaControlRootElementXXX.[cc,h] files, but also with the definition of the individual controls in MediaControlElement.[cc,h]. ANALYSIS ======== Essentially, the current class hierarchy looks like this: 1. Controls root: HTMLDivElement | - MediaControls | - MediaControlRootElement - MediaControlRootElementChromium | - MediaControlRootElementChromiumAndroid 2. Control elements: HTMLDivElement | - MediaControlElement | - MediaControlPanelElement - MediaControlTimelineContainerElement - MediaControlVolumeSliderContainerElement - MediaControlStatusDisplayElement - MediaControlTextTrackContainerElement - MediaControlTimeDisplayElement | - MediaControlTimeRemainingDisplayElement - MediaControlCurrentTimeDisplayElement - MediaControlChromiumEnclosureElement | - MediaControlPanelEnclosureElement - MediaControlOverlayEnclosureElement HTMLInputElement | - MediaControlInputElement | - MediaControlPlayButtonElement - MediaControlOverlayPlayButtonElement - MediaControlRewindButtonElement - MediaControlReturnToRealtimeButtonElement - MediaControlToggleClosedCaptionsButtonElement - MediaControlTimelineElement - MediaControlFullscreenVolumeMinButtonElement - MediaControlFullscreenVolumeMaxButtonElement - MediaControlFullscreenButtonElement - MediaControlVolumeSliderElement | - MediaControlFullscreenVolumeSliderElement - MediaControlSeekButtonElement | - MediaControlSeekForwardButtonElement - MediaControlSeekBackButtonElement - MediaControlMuteButtonElement | - MediaControlPanelMuteButtonElement - MediaControlVolumeSliderMuteButtonElement That's three different inheritance trees for classes that all share these functions: * create() * show() * hide() * setMediaController() Further, the individual controls all have these functions in common: * displayType() * isMediaControlElement() * shadowPseudoId() * mediaController() * defaultElementHandler() [this one is inconsistent and causing trouble in places] PROPOSAL ======== There are three stages of optimization that could be undertaken. 1. A first one would clean up the inheritance tree under the MediaControls Class. There are many function implementations that are shared between - MediaControlRootElement - MediaControlRootElementChromium - MediaControlRootElementChromiumAndroid These implementations should be pulled into the parent MediaControls class virtual functions. The inherited functions can then extend the base class virtual functions if needed. I think this cleanup is fairly obvious. 2. A second clean up would remove the duplication within the different control elements. I would want to create a stand-alone virtual class MediaControlElement that does not inherit from anywhere, but provides the common functions. Then the individual elements would inherit from MediaControlElement and either HTMLInputElement or HTMLDivElement. They would extend the functions as appropriate. I think this cleanup is also necessary. 3. A third cleanup could make a common virtual parent class between the MediaControls and the MediaControlElement classes for the create(), show(), hide() and setMediaController() functions. This would remove some further code duplication. It is, however, minimal and probably not worth the effort. Question ======== Would you agree to start with 1 and do 2, but probably not approach 3?

(In reply to comment #2) > I've analyzed the duplication of code. There is much duplication not only between the MediaControlRootElementXXX.[cc,h] files, but also with the definition of the individual controls in MediaControlElement.[cc,h]. > ... Nice analysis! > > Question > ======== > > Would you agree to start with 1 and do 2, but probably not approach 3? Yes, I agree that it makes sense to start with #1, then #2, and that #3 is probably not worth the effort at this time. Thanks!

Note to self for code reorg: Code in MediaControlElements.cpp interferes with the video controls display in MediaControlRootElementChromium.cpp (RenderMediaControlPanelEnclosure::layout) of bug 89344: * RenderMediaControlTimeDisplay::layout : removes time display at 190px * MediaControlVolumeSliderContainerElement::defaultEventHandler : hides volume slider * MediaControlPanelMuteButtonElement::defaultEventHandler : shows volume slider Chromium needs overrides for individual controls.

There is further reorg necessary in the Rendering classes for these elements. There are RenderXXX classes mixed in between the media controls in MediaControlElements.cpp . Can I suggest we move those into rendering/RenderMediaControls.h/.cpp ?

Created attachment 172710 [details] Refactoring MediaControls - will fail chromebot, since it also needs a chromium patch

Adding scherkus for review and also for the chromium patch - see http://code.google.com/p/chromium/issues/detail?id=159781

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Adding quinmin for a review of the android controls - I ran it through the build bots, but am not sure I broke anything visually, since we don't have layout tests for android buildbots FAIK

I am happy with the overall direction. Chromium WebKit API changes LGTM.

Android part LGTM

Comment on attachment 172710 [details] Refactoring MediaControls - will fail chromebot, since it also needs a chromium patch 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::MediaControlChromiumEnclosureElement): > + (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::MediaControlOverlayEnclosureElement): > + (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.

(In reply to comment #12) > > 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. OK, will remove these and similar ones. They were auto-created by prepare-Changelog. > > 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. Fair enough. :-) > > 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. OK, will add. Thanks everyone for taking the time to review!

Created attachment 173529 [details] Refactored MediaControls incl feedback from Eric

Note for Chromium commit queue: https://codereview.chromium.org/11364197 needs to land after this patch

Comment on attachment 173529 [details] Refactored MediaControls incl feedback from Eric Will add a patch to make sure not to break Chromium builds

Created attachment 173758 [details] Refactoring MediaControls incl a FIXME for after Chrome updates

Comment on attachment 173758 [details] Refactoring MediaControls incl a FIXME for after Chrome updates Rejecting attachment 173758 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: NFLICT (content): Merge conflict in LayoutTests/ChangeLog Failed to merge in the changes. Patch failed at 0001 Remove a few old no-longer-failing tests and update one baseline. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 154. Full output: http://queues.webkit.org/results/14809972

Landed manually as http://trac.webkit.org/changeset/134488