Bug 101877 – Refactor Media Control Elements (second half of bug 88871)

• Component: Media
• Status: RESOLVED FIXED

[Silvia Pfeiffer]

Remove code duplication within the different control elements by refactoring the media control elements as described in bug 88871.

Instead of having two different types of inheritance trees for MediaControlElement and MediaControlInputElement, create a stand-alone virtual class MediaControlElement that does not inherit from anywhere, but provides the common functions:
* create()
* show()
* hide()
* setMediaController()
* displayType()
* isMediaControlElement()
* shadowPseudoId()
* mediaController()
* defaultElementHandler()

Then the individual elements inherit from MediaControlElement and either HTMLInputElement or HTMLDivElement. They extend/override the base functions as appropriate.

Also, as part of this, the RenderXXX classes mixed in between the media controls in MediaControlElements.cpp should be moved to rendering/RenderMediaControls.h/.cpp to follow common code separation.

[Silvia Pfeiffer]

Created attachment 176400 [details]
Patch

[Peter Beverloo (cr-android ews)]

Comment on attachment 176400 [details]
Patch

Attachment 176400 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15025251

[Silvia Pfeiffer]

Created attachment 176896 [details]
Fix for Android

[Eric Carlson]

Comment on attachment 176896 [details]
Fix for Android

View in context: https://bugs.webkit.org/attachment.cgi?id=176896&action=review

> Source/WebCore/html/shadow/MediaControlElementTypes.cpp:56
> +#if ENABLE(VIDEO_TRACK)
> +static const int textTracksIndexNotFound = -2;
> +#endif

Please don't define this in two files in the same directory, add a static method to a class accessible to all places it is used.

> Source/WebCore/html/shadow/MediaControlElementTypes.h:120
> +

Nit: blank line isn't necessary.

> Source/WebCore/html/shadow/MediaControlElementTypes.h:139
> +    virtual void setCurrentValue(float);
> +    virtual float currentValue() const { return m_currentValue; }

Dot these need to be virtual?

> Source/WebCore/html/shadow/MediaControlElementTypes.h:180
> +    virtual void setActive(bool /*flag*/ = true, bool /*pause*/ = false);

OVERRIDE

> Source/WebCore/html/shadow/MediaControlElementTypes.h:185
> +    virtual void startTimer();
> +    virtual void stopTimer();
> +    virtual float nextRate() const;
> +    virtual void seekTimerFired(Timer<MediaControlSeekButtonElement>*);

Do these need to be virtual?

> Source/WebCore/html/shadow/MediaControlElementTypes.h:202
> +    virtual void setVolume(float);
> +    virtual void setClearMutedOnUserInteraction(bool);

Ditto.

[Silvia Pfeiffer]

(In reply to comment #4)
> (From update of attachment 176896 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176896&action=review
> 
> > Source/WebCore/html/shadow/MediaControlElementTypes.cpp:56
> > +#if ENABLE(VIDEO_TRACK)
> > +static const int textTracksIndexNotFound = -2;
> > +#endif
> 
> Please don't define this in two files in the same directory, add a static method to a class accessible to all places it is used.

DONE.

> > Source/WebCore/html/shadow/MediaControlElementTypes.h:120
> > +
> 
> Nit: blank line isn't necessary.

Removed. Also removed a few more from MediaControlElements.h.
 
> > Source/WebCore/html/shadow/MediaControlElementTypes.h:139
> > +    virtual void setCurrentValue(float);
> > +    virtual float currentValue() const { return m_currentValue; }
> 
> Dot these need to be virtual?

Good catch. Removed.

> > Source/WebCore/html/shadow/MediaControlElementTypes.h:180
> > +    virtual void setActive(bool /*flag*/ = true, bool /*pause*/ = false);
> 
> OVERRIDE

Good catch. I went through this file and MediaControlElements.h and added the necessary OVERRIDEs.

> > Source/WebCore/html/shadow/MediaControlElementTypes.h:185
> > +    virtual void startTimer();
> > +    virtual void stopTimer();
> > +    virtual float nextRate() const;
> > +    virtual void seekTimerFired(Timer<MediaControlSeekButtonElement>*);
> 
> Do these need to be virtual?

Removed.

> > Source/WebCore/html/shadow/MediaControlElementTypes.h:202
> > +    virtual void setVolume(float);
> > +    virtual void setClearMutedOnUserInteraction(bool);
> 
> Ditto.

Removed.

[Silvia Pfeiffer]

Created attachment 177392 [details]
Inc Eric's Feedback

[WebKit Review Bot]

Comment on attachment 177392 [details]
Inc Eric's Feedback

Clearing flags on attachment: 177392

Committed r136613: <http://trac.webkit.org/changeset/136613>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.