Bug 27123 - Full page zoom breaks remaining and elapsed time display in the <video> controller.
Summary: Full page zoom breaks remaining and elapsed time display in the <video> contr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Pierre d'Herbemont
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-07-09 10:26 PDT by Pierre d'Herbemont
Modified: 2009-07-09 18:23 PDT (History)
0 users

See Also:


Attachments
patch v1. (19.39 KB, patch)
2009-07-09 11:20 PDT, Pierre d'Herbemont
simon.fraser: review-
Details | Formatted Diff | Diff
patch v2. (19.47 KB, patch)
2009-07-09 14:27 PDT, Pierre d'Herbemont
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pierre d'Herbemont 2009-07-09 10:26:35 PDT
Full page zoom breaks remaining and elapsed time display in the <video> controller.

<rdar://problem/7045074>
Comment 1 Pierre d'Herbemont 2009-07-09 11:20:01 PDT
Created attachment 32524 [details]
patch v1.
Comment 2 Simon Fraser (smfr) 2009-07-09 12:34:39 PDT
Comment on attachment 32524 [details]
patch v1.


>             Full page zoom breaks remaining and elapsed time display in the &lt;video&gt; 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.
Comment 3 Pierre d'Herbemont 2009-07-09 14:27:14 PDT
Created attachment 32531 [details]
patch v2.
Comment 4 Pierre d'Herbemont 2009-07-09 14:29:31 PDT
(In reply to comment #2)
> (From update of attachment 32524 [details])
> 
> >             Full page zoom breaks remaining and elapsed time display in the &lt;video&gt; 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.
Comment 5 Pierre d'Herbemont 2009-07-09 14:30:05 PDT
Closed for no reason - reopening.
Comment 6 Simon Fraser (smfr) 2009-07-09 14:45:13 PDT
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());
Comment 7 Pierre d'Herbemont 2009-07-09 18:23:17 PDT
http://trac.webkit.org/changeset/45687