Bug 88871 – Analyze duplication of code for video controls shadow dom & rendering

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Silvia Pfeiffer]

We found this during the creation of patches for bug 88818 .

Right now, the MediaControlRootElement.[cc,h] and MediaControlRootElementChromium.[cc,h] files duplicate a lot of code without inheritance. Code streamlining is likely possible. We should analyze the duplication of code for the video controls shadow dom & rendering and remove code duplication where possible.

[Silvia Pfeiffer]

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

[Silvia Pfeiffer]

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?

[Eric Carlson]

(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!

[Silvia Pfeiffer]

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.

[Silvia Pfeiffer]

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 ?

[Silvia Pfeiffer]

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

[Silvia Pfeiffer]

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

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Silvia Pfeiffer]

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

[Dimitri Glazkov (Google)]

I am happy with the overall direction.

Chromium WebKit API changes LGTM.

[Min Qin]

Android part LGTM

[Eric Carlson]

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.

[Silvia Pfeiffer]

(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!

[Silvia Pfeiffer]

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

[Silvia Pfeiffer]

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

[Silvia Pfeiffer]

Comment on attachment 173529 [details]
Refactored MediaControls incl feedback from Eric

Will add a patch to make sure not to break Chromium builds

[Silvia Pfeiffer]

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

[WebKit Review Bot]

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

[Dean Jackson]

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