Bug 96339

Created attachment 188978 [details] Patch

Comment on attachment 188978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188978&action=review > Source/WebCore/html/shadow/MediaControlElements.cpp:319 > +void MediaControlPanelEnclosureElement::startTimer() > +{ > + stopTimer(); > + > + // The timer is required to set the property display:'none' on the panel, > + // such that captions are correctly displayed at the bottom of the video > + // at the end of the fadeout transition. > + double duration = document()->page() ? document()->page()->theme()->mediaControlsFadeOutDuration() : 0; > + m_transitionTimer.startOneShot(duration); > +} > + > +void MediaControlPanelEnclosureElement::stopTimer() > +{ > + if (m_transitionTimer.isActive()) > + m_transitionTimer.stop(); > +} > + > +void MediaControlPanelEnclosureElement::transitionTimerFired(Timer<MediaControlPanelEnclosureElement>*) > +{ > + if (!m_opaque) > + hide(); > + > + stopTimer(); > +} It looks like these were copied verbatim from MediaControlPanelElement. Both classes both derive from MediaControlDivElement, can these be moved there instead so we have only one implementation?

Yes they were. :-) MediaControlDivElement is the parent for almost all controls. Only MediaControlPanelElement and MediaControlPanelEnclosureElement need the timer before hiding. I could create another common parent between the two but it seems the extra class hierarchy is less efficient than this small amount of copied code.

(In reply to comment #3) > Yes they were. :-) > > MediaControlDivElement is the parent for almost all controls. Only MediaControlPanelElement and MediaControlPanelEnclosureElement need the timer before hiding. I could create another common parent between the two but it seems the extra class hierarchy is less efficient than this small amount of copied code. I don't see any reason it couldn't go into the base class. We don't want objects that don't need a timer to have one, but you can use an OwnPtr<Timer<..> > and have startTimer() allocate it the first time it is called. See RenderButton::styleDidChange for an example of how this might work.

(In reply to comment #4) > I don't see any reason it couldn't go into the base class. We don't want objects that don't need a timer to have one, but you can use an OwnPtr<Timer<..> > and have startTimer() allocate it the first time it is called. See RenderButton::styleDidChange for an example of how this might work. Good idea, thanks!

Just noticed it on MediaControlEmbeddedPanelElement of the Blackberry controls, too. This will remove some duplicated code from there, too.

Created attachment 189709 [details] move timer into MediaControlDivElement

Comment on attachment 189709 [details] move timer into MediaControlDivElement Attachment 189709 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16698688 New failing tests: media/video-controls-toggling.html

Comment on attachment 189709 [details] move timer into MediaControlDivElement View in context: https://bugs.webkit.org/attachment.cgi?id=189709&action=review Nice refactoring, thanks for taking the extra time to do it! > Source/WebCore/html/shadow/MediaControlElementTypes.cpp:154 > + double duration = document()->page() ? document()->page()->theme()->mediaControlsFadeInDuration() : 0; > + > + setInlineStyleProperty(CSSPropertyWebkitTransitionProperty, CSSPropertyOpacity); > + setInlineStyleProperty(CSSPropertyWebkitTransitionDuration, duration, CSSPrimitiveValue::CSS_S); > + setInlineStyleProperty(CSSPropertyOpacity, 1.0, CSSPrimitiveValue::CSS_NUMBER); > + > + setOpaque(true); Nit: if this was in a separate method it could be shared by makeOpaque and makeTransparent.