Bug 101877

Summary: Refactor Media Control Elements (second half of bug 88871)
Product: WebKit Reporter: Silvia Pfeiffer <silviapf>
Component: MediaAssignee: Silvia Pfeiffer <silviapf>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric.carlson, eric, feature-media-reviews, gyuyoung.kim, ojan, peter+ews, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 88871    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Fix for Android
none
Inc Eric's Feedback none

Silvia Pfeiffer
Reported 2012-11-11 18:23:07 PST
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.
Attachments
Patch (105.04 KB, patch)
2012-11-27 22:35 PST, Silvia Pfeiffer 		no flags
DetailsFormatted DiffDiff
Fix for Android (106.39 KB, patch)
2012-11-29 20:36 PST, Silvia Pfeiffer 		no flags
DetailsFormatted DiffDiff
Inc Eric's Feedback (110.89 KB, patch)
2012-12-03 18:51 PST, Silvia Pfeiffer 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Silvia Pfeiffer
Comment 1 2012-11-27 22:35:49 PST
Created attachment 176400 [details] Patch
Peter Beverloo (cr-android ews)
Comment 2 2012-11-27 22:54:26 PST
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
Comment 3 2012-11-29 20:36:39 PST
Created attachment 176896 [details] Fix for Android
Eric Carlson
Comment 4 2012-11-30 07:54:07 PST
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
Comment 5 2012-12-03 15:43:56 PST
(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
Comment 6 2012-12-03 18:51:49 PST
Created attachment 177392 [details] Inc Eric's Feedback
WebKit Review Bot
Comment 7 2012-12-04 20:02:09 PST
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
Comment 8 2012-12-04 20:02:14 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.