Bug 47099 – Allow shrinking large images inside a frame

• Component: Images
• Status: UNCONFIRMED

[Sadrul Habib Chowdhury]

Allow shrinking large images inside a frame

[Sadrul Habib Chowdhury]

Created attachment 69654 [details]
Patch

When a frame contains a large image, it doesn't autoresize it, and doesn't allow manually resizing it to fit inside the frame. This patch adds a way to toggle between the original size or the shrunk size to fit in the window for large images.

[WebKit Review Bot]

Attachment 69654 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/ContextMenuClientImpl.cpp:177:  Declaration has space between type name and * in Image *image  [whitespace/declaration] [3]
WebKit/chromium/src/ContextMenuClientImpl.cpp:181:  Declaration has space between type name and * in FrameView *view  [whitespace/declaration] [3]
WebKit/chromium/src/ContextMenuClientImpl.cpp:182:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebKit/chromium/src/ContextMenuClientImpl.cpp:185:  Declaration has space between type name and * in ImageDocument *imgdoc  [whitespace/declaration] [3]
WebKit/chromium/src/WebViewImpl.cpp:1648:  Declaration has space between type name and * in Node *node  [whitespace/declaration] [3]
WebKit/chromium/src/WebViewImpl.cpp:1650:  Declaration has space between type name and * in ImageDocument *imgdoc  [whitespace/declaration] [3]
WebCore/html/ImageDocument.cpp:382:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 7 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 69654 [details]
Patch

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.

[Sadrul Habib Chowdhury]

Created attachment 69661 [details]
Revised patch to pass the tests

Oops. Sorry! Still getting used to checking styles before submission.

[WebKit Review Bot]

Attachment 69654 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4163081

[Sadrul Habib Chowdhury]

The patch to use this in chromium is at: http://codereview.chromium.org/3570007/show

I will address the raised issues and resubmit the patch.

[Sadrul Habib Chowdhury]

Created attachment 69672 [details]
Updated patch

Addressed Darin's comments to update the patch.

[WebKit Review Bot]

Attachment 69672 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4176083

[Adam Barth]

Comment on attachment 69672 [details]
Updated patch

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

[Alexey Proskuryakov]

See also: bug 30766, which is pretty much a duplicate. It seems that the difference is only about the default state.