Bug 34302 - RenderVideo should prefer provided width/height properties to hard coded defaults when natural size is unavailable
Summary: RenderVideo should prefer provided width/height properties to hard coded defa...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-28 20:25 PST by Nick Young
Modified: 2010-01-28 23:42 PST (History)
2 users (show)

See Also:


Attachments
Initial Patch. (2.48 KB, patch)
2010-01-28 20:28 PST, Nick Young
eric.carlson: review+
eric.carlson: commit-queue-
Details | Formatted Diff | Diff
Updated Patch (3.24 KB, patch)
2010-01-28 21:52 PST, Nick Young
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Young 2010-01-28 20:25:32 PST
The constructor of RenderVideo in WebCore/rendering/RenderVideo.cpp attempts to determine the intrinsic size of the video by calling the naturalSize() method of the media player.
However, when RenderVideo is constructed the media player may not yet have been constructed.

In these situations, the renderer currently falls back to a hard coded default video size as the intrinsic size - (300, 150).
This patch contends that in these situations, the width and height properties of the video element, if available, should be used as the intrinsic size until better values are available.

If the width and height properties are unavailable, then the hard coded default is used once again.
Comment 1 Nick Young 2010-01-28 20:28:38 PST
Created attachment 47669 [details]
Initial Patch.
Comment 2 Eric Carlson 2010-01-28 21:42:47 PST
Comment on attachment 47669 [details]
Initial Patch.

>          // size since they also have audio thrown at them. By setting the intrinsic
>          // size to 300x1 the video will resize itself in these cases, and audio will
>          // have the correct height (it needs to be > 0 for controls to render properly).
> -        if (video->ownerDocument() && video->ownerDocument()->isMediaDocument())
> +        if (video->hasAttribute(widthAttr) && video->hasAttribute(heightAttr))
> +            setIntrinsicSize(IntSize(video->width(), video->height()));
> +        else if (video->ownerDocument() && video->ownerDocument()->isMediaDocument())

This comment doesn't make sense now, it is about the second test. Please add a comment about the logic in the first test ("Prefer provided video element width/height properties...") and move this comment down to be with the second test.

r=me with this change
Comment 3 Nick Young 2010-01-28 21:52:03 PST
Created attachment 47678 [details]
Updated Patch

Comments reorganized and updated as requested.
Comment 4 Eric Carlson 2010-01-28 22:14:26 PST
Comment on attachment 47678 [details]
Updated Patch

Nice change, thanks!

r=me
Comment 5 WebKit Commit Bot 2010-01-28 23:42:11 PST
Comment on attachment 47678 [details]
Updated Patch

Clearing flags on attachment: 47678

Committed r54048: <http://trac.webkit.org/changeset/54048>
Comment 6 WebKit Commit Bot 2010-01-28 23:42:17 PST
All reviewed patches have been landed.  Closing bug.