[Webkit-unassigned] [Bug 47099] Allow shrinking large images inside a frame

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=47099


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #69654|review?                     |review-
               Flag|                            |




--- Comment #3 from Darin Adler <darin at apple.com>  2010-10-04 11:56:27 PST ---
(From update of attachment 69654)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.