Bug 47099 - Allow shrinking large images inside a frame
Summary: Allow shrinking large images inside a frame
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2010-10-04 11:32 PDT by Sadrul Habib Chowdhury
Modified: 2010-12-23 10:12 PST (History)
3 users (show)

See Also:

Patch (9.66 KB, patch)
2010-10-04 11:45 PDT, Sadrul Habib Chowdhury
darin: review-
Details | Formatted Diff | Diff
Revised patch to pass the tests (9.64 KB, patch)
2010-10-04 12:04 PDT, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
Updated patch (9.48 KB, patch)
2010-10-04 13:03 PDT, Sadrul Habib Chowdhury
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sadrul Habib Chowdhury 2010-10-04 11:32:49 PDT
Allow shrinking large images inside a frame
Comment 1 Sadrul Habib Chowdhury 2010-10-04 11:45:17 PDT
Created attachment 69654 [details]

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.
Comment 2 WebKit Review Bot 2010-10-04 11:50:01 PDT
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.
Comment 3 Darin Adler 2010-10-04 11:56:27 PDT
Comment on attachment 69654 [details]

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.
Comment 4 Sadrul Habib Chowdhury 2010-10-04 12:04:36 PDT
Created attachment 69661 [details]
Revised patch to pass the tests

Oops. Sorry! Still getting used to checking styles before submission.
Comment 5 WebKit Review Bot 2010-10-04 12:06:25 PDT
Attachment 69654 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4163081
Comment 6 Sadrul Habib Chowdhury 2010-10-04 12:10:03 PDT
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.
Comment 7 Sadrul Habib Chowdhury 2010-10-04 13:03:16 PDT
Created attachment 69672 [details]
Updated patch

Addressed Darin's comments to update the patch.
Comment 8 WebKit Review Bot 2010-10-04 16:02:00 PDT
Attachment 69672 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4176083
Comment 9 Adam Barth 2010-12-23 00:55:32 PST
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
Comment 10 Alexey Proskuryakov 2010-12-23 10:12:07 PST
See also: bug 30766, which is pretty much a duplicate. It seems that the difference is only about the default state.