[webkit-reviews] review denied: [Bug 47099] Allow shrinking large images inside a frame : [Attachment 69672] Updated patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied sadrul at chromium.org's request for
review:
Bug 47099: Allow shrinking large images inside a frame
https://bugs.webkit.org/show_bug.cgi?id=47099

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=69672&action=review

R- for nits (just because this is an old patch).  Generally, this looks
reasonable, but I didn't look at the Chromium-specific parts too closely. 
We'll need fishd to give the final review because this changes the Chromium
WebKit API.

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

We won't be able to land this patch with this line in the ChangeLog.  We'll
either need to add some tests or explain why we can't test this patch.

> WebCore/html/ImageDocument.cpp:215
> +    m_imageElement = imageElement.get();

Is m_imageElement a raw pointer?  This line looks suspicious.  Usually we
should be able to just assign m_imageElement = imageElement directly.  I guess
this code was here before, but I'd like to understand.

> WebCore/html/ImageDocument.h:50
> +    bool imageIsShrunk() const { return m_didShrinkImage; }

The accessor's name should match the name of the member variable.  I'm not sure
which name is better, but they should match.  :)

> WebCore/html/ImageDocument.h:73
>      // Whether the image should be shrunk or not

This comment is useless and should be removed.	Actually all the comments on
these member variables are useless and should be removed.  ;)

> WebCore/html/ImageDocument.h:76
> +    // Click handler

This comment is useless and should be removed.	Perhaps m_listener should be
called m_clickListener?

> WebKit/chromium/src/ContextMenuClientImpl.cpp:184
> +		   ImageDocument* imgdoc =
static_cast<ImageDocument*>(r.innerNonSharedNode()->document());

imgdoc => imageDocument