Bug 27123 – Full page zoom breaks remaining and elapsed time display in the <video> controller.

• Component: New Bugs
• Status: RESOLVED FIXED

[Pierre d'Herbemont]

Full page zoom breaks remaining and elapsed time display in the <video> controller.

<rdar://problem/7045074>

[Pierre d'Herbemont]

Created attachment 32524 [details]
patch v1.

[Simon Fraser (smfr)]

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.

[Pierre d'Herbemont]

Created attachment 32531 [details]
patch v2.

[Pierre d'Herbemont]

(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.

[Pierre d'Herbemont]

Closed for no reason - reopening.

[Simon Fraser (smfr)]

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());

[Pierre d'Herbemont]

http://trac.webkit.org/changeset/45687