Bug 43702 – <video> element does not resize correctly

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

According to the HTML5 spec, a <video> element's intrinsic size is:

  + the intrinsic width/height of a video element's playback area is the intrinsic width/height of the video resource, if that is available
  + otherwise it is the intrinsic width/height of the poster frame, if that is available; 
  + otherwise it is 300/155 CSS pixels

In WebKit a newly opened <video> element follows this rule, but if the 'poster' or 'src' attribute is changed, and the url fails to load the element will not be sized correctly.

[Eric Carlson]

Created attachment 63857 [details]
proposed patch

[WebKit Review Bot]

Attachment 63857 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/html/HTMLVideoElement.cpp:111:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 63857 [details]
proposed patch

> +#if !ENABLE(PLUGIN_PROXY_FOR_VIDEO)
> +    // Reset the intrinsic size in case we are loading a new movie.
> +    if (renderer() && renderer()->isVideo())
> +        toRenderVideo(renderer())->videoSizeChanged();
> +#endif
> +    
>      if (renderer())
>          renderer()->updateFromElement();

Why do we need a new function that's video-element specific? Why can't RenderVideo do this in an override of updateFromElement?

[Eric Carlson]

(In reply to comment #3)
> (From update of attachment 63857 [details])
> > +#if !ENABLE(PLUGIN_PROXY_FOR_VIDEO)
> > +    // Reset the intrinsic size in case we are loading a new movie.
> > +    if (renderer() && renderer()->isVideo())
> > +        toRenderVideo(renderer())->videoSizeChanged();
> > +#endif
> > +    
> >      if (renderer())
> >          renderer()->updateFromElement();
> 
> Why do we need a new function that's video-element specific? Why can't RenderVideo do this 
> in an override of updateFromElement?

Excellent point, thanks!

[Eric Carlson]

Created attachment 63913 [details]
revised patch

RenderVideo::videoSizeChanged removed completely, updateFromElement now resets the intrinsic size when necessary.

[WebKit Review Bot]

Attachment 63913 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/html/HTMLVideoElement.cpp:111:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 63913 [details]
revised patch

>          }
> +        else {

Else should go on the same line as the closing brace to match the usual WebKit style.

> +void RenderVideo::updateIntrinsicSize()
>  {
> +    IntSize size = calculateIntrinsicSize();
> +
> +    // Never set the element size to zero when in a media document.
> +    if (size.isEmpty() && node()->ownerDocument() && node()->ownerDocument()->isMediaDocument())
>          return;
> +
>      if (size != intrinsicSize()) {
>          setIntrinsicSize(size);
>          setPrefWidthsDirty(true);
>          setNeedsLayout(true);
>      }
>  }

Looks good. I would have used an early return style instead of nesting the work inside an if statement.

> +        if (errorOccurred())
> +            setIntrinsicSize(defaultSize());

Could you just call updateIntrinsicSize here instead of explicitly setting the size to default?

r=me

[Eric Carlson]

http://trac.webkit.org/changeset/64997, with all of Darin's suggestions.

[Ademar Reis]

Revision r64997 cherry-picked into qtwebkit-2.2 with commit 68a63b1 <http://gitorious.org/webkit/qtwebkit/commit/68a63b1>