Bug 43702

Summary: <video> element does not resize correctly
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: MediaAssignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: ademar, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 51249    
Attachments:
Description Flags
proposed patch
none
revised patch none

Description Eric Carlson 2010-08-08 20:11:48 PDT
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.
Comment 1 Eric Carlson 2010-08-08 20:16:06 PDT
Created attachment 63857 [details]
proposed patch
Comment 2 WebKit Review Bot 2010-08-08 20:17:43 PDT
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.
Comment 3 Darin Adler 2010-08-08 23:39:27 PDT
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?
Comment 4 Eric Carlson 2010-08-09 10:30:54 PDT
(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!
Comment 5 Eric Carlson 2010-08-09 11:58:42 PDT
Created attachment 63913 [details]
revised patch

RenderVideo::videoSizeChanged removed completely, updateFromElement now resets the intrinsic size when necessary.
Comment 6 WebKit Review Bot 2010-08-09 11:59:48 PDT
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.
Comment 7 Darin Adler 2010-08-09 12:02:35 PDT
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
Comment 8 Eric Carlson 2010-08-09 13:31:40 PDT
http://trac.webkit.org/changeset/64997, with all of Darin's suggestions.
Comment 9 Ademar Reis 2011-01-24 12:19:10 PST
Revision r64997 cherry-picked into qtwebkit-2.2 with commit 68a63b1 <http://gitorious.org/webkit/qtwebkit/commit/68a63b1>