[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


Adam Barth <abarth at webkit.org> changed:

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




--- Comment #9 from Adam Barth <abarth at webkit.org>  2010-12-23 00:55:32 PST ---
(From update of attachment 69672)
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

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