[webkit-reviews] review denied: [Bug 27123] Full page zoom breaks remaining and elapsed time display in the <video> controller. : [Attachment 32524] patch v1.

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Pierre d'Herbemont
<pdherbemont at apple.com>'s request for review:
Bug 27123: Full page zoom breaks remaining and elapsed time display in the
<video> controller.
https://bugs.webkit.org/show_bug.cgi?id=27123

Attachment 32524: patch v1.
https://bugs.webkit.org/attachment.cgi?id=32524&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>

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