[webkit-reviews] review denied: [Bug 47099] Allow shrinking large images inside a frame : [Attachment 69654] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> 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 69654: Patch
https://bugs.webkit.org/attachment.cgi?id=69654&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=69654&action=review

I didn’t review the Chrome-only part of this that adds a new function just for
that platform, but I did review the platform-independent part.

> WebCore/html/ImageDocument.cpp:220
> +void ImageDocument::allowShrinking(bool allow)

This name is not right for a function that takes a boolean argument. If we had
a function named allowShrinking then it would not take a boolean.

The WebKit name for this would be setShouldShrinkToFit.

> WebCore/html/ImageDocument.cpp:225
> -	   RefPtr<EventListener> listener = ImageEventListener::create(this);
> +	   RefPtr<EventListener> listener;
> +	   listener = ImageEventListener::create(this);

This is not a good change. Why did you break this into two lines?

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

I suggest putting this function inline in the header.

> WebCore/html/ImageDocument.h:77
> +    EventListener *m_listener;

It may not be safe to hold the event listener in a raw pointer; in normal
circumstances the references to the listeners in the object should keep them
alive, but there may be unusual circumstances where it does not. Assuming we
need this code to dynamically add and remove the listeners, then I suggest
using a RefPtr.

Also, the formatting here is wrong.