[webkit-reviews] review granted: [Bug 43702] <video> element does not resize correctly : [Attachment 63913] revised patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Aug 9 12:02:35 PDT 2010


Darin Adler <darin at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 43702: <video> element does not resize correctly
https://bugs.webkit.org/show_bug.cgi?id=43702

Attachment 63913: revised patch
https://bugs.webkit.org/attachment.cgi?id=63913&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
>	   }
> +	   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


More information about the webkit-reviews mailing list