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

Eric Carlson
Reported 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.
Attachments
proposed patch (23.33 KB, patch)
2010-08-08 20:16 PDT, Eric Carlson 		no flags
DetailsFormatted DiffDiff
revised patch (25.38 KB, patch)
2010-08-09 11:58 PDT, Eric Carlson 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Eric Carlson
Comment 1 2010-08-08 20:16:06 PDT
Created attachment 63857 [details] proposed patch
WebKit Review Bot
Comment 2 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.
Darin Adler
Comment 3 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?
Eric Carlson
Comment 4 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!
Eric Carlson
Comment 5 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.
WebKit Review Bot
Comment 6 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.
Darin Adler
Comment 7 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
Eric Carlson
Comment 8 2010-08-09 13:31:40 PDT
http://trac.webkit.org/changeset/64997, with all of Darin's suggestions.
Ademar Reis
Comment 9 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>
Note You need to log in before you can comment on or make changes to this bug.