[Webkit-unassigned] [Bug 88871] Analyze duplication of code for video controls shadow dom & rendering

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=88871





--- Comment #2 from Silvia Pfeiffer <silviapf at chromium.org>  2012-09-10 22:34:31 PST ---
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.