Full page zoom breaks remaining and elapsed time display in the <video> controller. <rdar://problem/7045074>
Created attachment 32524 [details] patch v1.
Comment on attachment 32524 [details] patch v1. > Full page zoom breaks remaining and elapsed time display in the <video> controller. No need to escape the <> > + PassRefPtr<RenderStyle> style = styleForElement(); > if (!style) > return; This should be a RefPtr. > > @@ -258,7 +258,7 @@ void MediaControlInputElement::update() > updateStyle(); > } > > -RenderStyle* MediaControlInputElement::styleForElement() > +PassRefPtr<RenderStyle> MediaControlInputElement::styleForElement() > { > return m_mediaElement->renderer()->getCachedPseudoStyle(m_pseudoStyleId); > } No need to change return type> > @@ -270,17 +270,17 @@ bool MediaControlInputElement::rendererIsNeeded(RenderStyle* style) > > void MediaControlInputElement::attach() > { > - RenderStyle* style = styleForElement(); > + PassRefPtr<RenderStyle> style = styleForElement(); This should be a RefPtr > void MediaControlTimeDisplayElement::setVisible(bool visible) > { > + // This function is used during the RenderMedia::layout() > + // call, where we cannot change the renderer at this time. > + if (visible == m_isVisible) > + return; > + m_isVisible = visible; > + RefPtr<RenderStyle> style = styleForElement(); > + renderer()->setStyle(style.get()); But you are changing style here, which is not really a good thing to do.
Created attachment 32531 [details] patch v2.
(In reply to comment #2) > (From update of attachment 32524 [details]) > > > Full page zoom breaks remaining and elapsed time display in the <video> controller. > > No need to escape the <> For some reason prepare-ChangeLog does it. > > > > + PassRefPtr<RenderStyle> style = styleForElement(); > > if (!style) > > return; > > This should be a RefPtr. Bad search in replace, should be fixed. > > > > > @@ -258,7 +258,7 @@ void MediaControlInputElement::update() > > updateStyle(); > > } > > > > -RenderStyle* MediaControlInputElement::styleForElement() > > +PassRefPtr<RenderStyle> MediaControlInputElement::styleForElement() > > { > > return m_mediaElement->renderer()->getCachedPseudoStyle(m_pseudoStyleId); > > } > > No need to change return type> It's needed for the subclass override. (Which may clone a style) > > > void MediaControlTimeDisplayElement::setVisible(bool visible) > > { > > + // This function is used during the RenderMedia::layout() > > + // call, where we cannot change the renderer at this time. > > > + if (visible == m_isVisible) > > + return; > > + m_isVisible = visible; > > + RefPtr<RenderStyle> style = styleForElement(); > > + renderer()->setStyle(style.get()); > > But you are changing style here, which is not really a good thing to do. We agreed that this was OK even though not the proper solution in fact, given that previous implementation is doing in the end the same thing.
Closed for no reason - reopening.
Comment on attachment 32531 [details] patch v2. > + We are changing the size of the time remaining and time ellapsed field, to Typo: "ellapsed" > + The following patch fixes that problem by using a cloned style on which we "This change", not "The following patch", since the changelog needs to read well after you commit :) > + RefPtr<RenderStyle> style = styleForElement(); > + renderer()->setStyle(style.get()); To reduce ref churn, you could just do renderer()->setStyle(styleForElement());
http://trac.webkit.org/changeset/45687