Summary: | <video> element does not resize correctly | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||
Component: | Media | Assignee: | 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
Eric Carlson
2010-08-08 20:11:48 PDT
Created attachment 63857 [details]
proposed patch
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 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? (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! Created attachment 63913 [details]
revised patch
RenderVideo::videoSizeChanged removed completely, updateFromElement now resets the intrinsic size when necessary.
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 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 http://trac.webkit.org/changeset/64997, with all of Darin's suggestions. Revision r64997 cherry-picked into qtwebkit-2.2 with commit 68a63b1 <http://gitorious.org/webkit/qtwebkit/commit/68a63b1> |