[webkit-reviews] review denied: [Bug 5793] HTML OBJECT without width/height attributes doesn't honor the size of the image : [Attachment 72031] Proposed patch without source code.

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Julie Jeongeun Kim
<jiyuluna at gmail.com>'s request for review:
Bug 5793: HTML OBJECT without width/height attributes doesn't  honor the size
of the image
https://bugs.webkit.org/show_bug.cgi?id=5793

Attachment 72031: Proposed patch without source code.
https://bugs.webkit.org/attachment.cgi?id=72031&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=72031&action=review

Overall, this patch looks good, but r- to run layout tests and/or write a new
layout test, remove debugging code, and fix the ChangeLog patch.

> WebCore/ChangeLog:1
> -2010-10-27  Adam Roben  <aroben at apple.com>
> +2010-10-27  Julie-Jeongeun-Kim  <jiyuluna at gmail.com>

Please don't delete existing ChangeLog entries.  Your new changelog entry
should simply appear before the other ones.

For the most part, your new changelog entry looks good.  Just a couple nits
below.

> WebCore/ChangeLog:10
> +	   It should let its owner know the real size so that OBJECT is
re-layouted.

Let's use "is laid out again." instead of "is re-layouted."

> WebCore/ChangeLog:12
> +	   No new tests. (OOPS!)

Did you try to write a test case for this change?

Did you run layout tests to see if any existing layout test results changed?

> WebCore/html/ImageDocument.cpp:46
> +#include <stdio.h>

This is probably left over from debugging.  Please remove it.

> WebCore/html/ImageDocument.cpp:301
> +    IntSize windowSize = IntSize(view->width(), view->height());

Is there a FrameView::size() method that returns an IntSize?

> WebCore/html/ImageDocument.cpp:310
> +		   RenderEmbeddedObject* rendererEmbedded =
toRenderEmbeddedObject(renderer);
> +		       
> +		   if (rendererEmbedded) 

These lines can be combined:

    if (RenderEmbeddedObject* rendererEmbedded =
toRenderEmbeddedObject(renderer))