Bug 48781 – Add a resourceload delegate method to query if WebCore should paint the default broken image for failed images.

• Component: Images
• Status: RESOLVED FIXED

[Yongjun Zhang]

Sometimes, a WebKit base app might want WebCore not to paint the default broken image if an image is failed due to loading error or decoding error.   We need to add a delegate method to tell the app the image loading is failed (with image url and reason why failed), and let the app decide if WebCore can go ahead paint the default broken image.

[Yongjun Zhang]

<rdar://problem/8610908>

[Yongjun Zhang]

Created attachment 72577 [details]
Add new method shouldPaintBrokenImageWithError for FrameLoaderClient;

The patch implements shouldPaintBrokenImageWithError for mac, for other ports, it just makes a empty implementation.

I am not sure if it is the best way to add error code enums in CachedImage.h, I can move the error codes to a dedicated header file is that is preferred.

[Alexey Proskuryakov]

I guess the most interesting question here is whether we want a delegate method, or a preference.

> not to paint the default broken image if an image is failed due to loading error

I suspect that's doable with resource load delegates even today.

> or decoding error.

But probably not this one.

[WebKit Review Bot]

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

[WebKit Review Bot]

Attachment 72577 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/5007007

[Yongjun Zhang]

Created attachment 72676 [details]
Fix build break for GTK and Chromium.

[Yongjun Zhang]

(In reply to comment #3)
> I guess the most interesting question here is whether we want a delegate method, or a preference.

Adding a preference is my first thought, but the developer who requested this feature thinks a preference is not enough.

[Alexey Proskuryakov]

OK. This still looks somewhat arbitrary to me, but I'll just leave this for someone more familiar with API to review.

[Adam Barth]

Comment on attachment 72676 [details]
Fix build break for GTK and Chromium.

View in context: https://bugs.webkit.org/attachment.cgi?id=72676&action=review

Should we test what happens when you return false from this delegate call?

> WebCore/loader/CachedImage.cpp:233
> +    if (!frame || !frame->loader())

Frames always have loaders.  There's no need to null check it.

> WebCore/loader/CachedImage.cpp:236
> +    KURL imageURL(ParsedURLString, m_url);

Why do we define this local variable and then not use it?

> WebCore/loader/CachedImage.cpp:302
> +            int errorCode = ImageErrorUnknown;
> +            if (m_image->isNull())
> +                errorCode = ImageErrorUnableToDecode;
> +            else
> +                errorCode = ImageErrorImageTooBig;

This seems like it should be a member function that returns the errorCode()

> WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:938
> +}

missing blank line after this line.

[Yongjun Zhang]

Created attachment 73268 [details]
New patch addressing review comments.

Change in this version:

1. added m_shouldPaintBrokenImage data member and getter/setter to LayoutTestController, to turn on/off paint broken image for DRT.
2. added errorCode() member function in CachedImage as per review comments.
3. modified layout test to test if painting broken image is disabled.

[Yongjun Zhang]

Any updates?

[Darin Adler]

Comment on attachment 73268 [details]
New patch addressing review comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=73268&action=review

Looks OK. Needs some work.

> WebCore/loader/FrameLoaderClient.h:294
> +        virtual bool shouldPaintBrokenImageWithError(const KURL& url, int errorCode) const = 0;

The argument type should be ImageError, not int.

There’s no need to include “with error” in the name of this function. That’s fine for Objective-C naming style, but not appropriate here.

There’s no need for the argument name “url”. If you were going to give a name that clarified what it’s the URL of, that would be one thing, but currently it’s just repeating the type name, and so should be omitted.

This is an unusual feature that most platforms won’t implement, so I suggest having it default to returning true rather than being a pure virtual function. That way you won’t have to touch the frame loader clients for all the platforms that won’t implement it and for EmptyClients.h.

> WebCore/loader/cache/CachedImage.cpp:61
> +    , m_errorCode(ImageErrorNone)

I see no reason for m_errorCode to be a data member. The call sites should instead just pass the error code as an argument when calling the function that sets m_shouldPaintBrokenImage.

> WebCore/loader/cache/CachedImage.cpp:231
> +bool CachedImage::checkShouldPaintBrokenImage() const

There’s no reason to have the word “check” in the name of this function. Unless you want it to set m_shouldPaintBrokenImage directly rather than having a return value. Then the name might be OK.

I suggest keeping the name, and changing how the function works.

> WebCore/loader/cache/CachedImage.cpp:300
> +            m_errorCode = ImageErrorUnknown;

This line is dead code and should be removed.

> WebCore/loader/cache/CachedImage.h:38
> +    ImageErrorNone = 0,

No need for the = 0 here.

> WebCore/loader/cache/CachedImage.h:39
> +    ImageErrorUnknown,

This error code isn’t needed. Lets just omit it.

> WebCore/loader/cache/CachedImage.h:42
> +    ImageErrorImageOutOfMemory,

This error code is unused. Please omit it unless we have a future use for it planned.

> WebCore/loader/cache/CachedImage.h:99
> +    ImageError errorCode() const { return m_errorCode; }

This is not needed. Please do not add it.

> WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1133
> +    notImplemented();

I don’t think you need notImplemented() here. It’s OK to not have the feature for now so please just leave it as-is.

> WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:447
> +    if (implementations->shouldPaintBrokenImageWithErrorFunc) {

We normally use early exit, so you could just reverse this so you won’t have to nest everything.

> WebKit/mac/WebView/WebResourceLoadDelegate.h:169
> ++     @result return an BOOL to tell if WebKit should paint the default broken image.

“an bool”?

The resource load delegate is public API. If you want to make this change we’ll need to do API review. An alternative would be to make this a private extension to the resource load delegate API.

[Yongjun Zhang]

Created attachment 77846 [details]
New patch addressing comments.

Thanks, for the review.  Here is the new patch.

Changes:

1. added default implementation for FrameLoaderClient::shouldPaintBrokenImage, so that I don't need to touch other WebKit ports.
2. set m_shouldPaintBrokenImage in CachedImage::checkShouldPaintBrokenImage instead of the call site.
3. moved webView:shouldPaintBrokenImage:(NSURL*)imageURL to WebResourceLoadDelegatePrivate.
4. removed image errorcode argument from FrameLoaderClient::shouldPaintBrokenImage and the delegate method, since the developer asked for this feature mainly cared about the URL of the failed image, no the error code.  I will open another bug if they really need it.

[Darin Adler]

Comment on attachment 77846 [details]
New patch addressing comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=77846&action=review

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:245
> +- (BOOL)webView: (WebView*)wv shouldPaintBrokenImage:(NSURL*)imageURL

Extras space here. Also, I would use a longer variable name rather than "wv".

> WebKit/mac/WebView/WebResourceLoadDelegatePrivate.h:63
> +/*!
> + @method webView:shouldPaintBrokenImage:(NSURL*)imageURL
> + @abstract This message is sent when an image cannot be decoded or displayed.
> + @param imageURL The url of the offending image.
> + @result return YES if WebKit should paint the default broken image.
> + */
> +- (BOOL)webView:(WebView*)sender shouldPaintBrokenImage:(NSURL*)imageURL;
>  @end

I would think this should be named shouldPaintBrokenImageForURL: given the usual Objective-C naming scheme. In non-ObjC code, there is no need to include the argument name in the function name, but in ObjC method names we normally do.

[Yongjun Zhang]

Created attachment 77849 [details]
change the delegate method to shouldPaintBrokenImageForURL:

Yes, shouldPaintBrokenImageForURL is better.  Please see the revised patch.

[Darin Adler]

Comment on attachment 77849 [details]
change the delegate method to shouldPaintBrokenImageForURL:

View in context: https://bugs.webkit.org/attachment.cgi?id=77849&action=review

> Tools/DumpRenderTree/LayoutTestController.h:306
> +    bool shouldPaintBrokenImage() const	{ return m_shouldPaintBrokenImage; }

Extra spaces here after “const”.

[WebKit Commit Bot]

Comment on attachment 77849 [details]
change the delegate method to shouldPaintBrokenImageForURL:

Rejecting attachment 77849 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
tional ..
platform/mac/plugins ............
platform/mac/scrollbars .
plugins ...................................................
plugins/npruntime ...............
printing ...........................
printing/css2.1 .........
scrollbars ..................
security ..
security/block-test-no-port.html -> failed

Exiting early after 1 failures. 18604 tests run.
420.97s total testing time

18603 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
11 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7217418

[Yongjun Zhang]

Created attachment 78148 [details]
Only log the message when shouldPaintBrokenImage() returns NO; this avoids changing results of existing layout tests with failed images.

Fix the layout test break in security/block-test-no-port.html.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/images/resources/broken-image-with-invalid-format.png', u'LayoutTests/fast/images/support-broken-image-delegate.html', u'LayoutTests/platform/mac/fast/images/support-broken-image-delegate-expected.txt', u'Tools/ChangeLog', u'Tools/DumpRenderTree/LayoutTestController.cpp', u'Tools/DumpRenderTree/LayoutTestController.h', u'Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm', u'WebCore/ChangeLog', u'WebCore/loader/FrameLoaderClient.h', u'WebCore/loader/cache/CachedImage.cpp', u'WebCore/loader/cache/CachedImage.h', u'WebKit/mac/ChangeLog', u'WebKit/mac/WebCoreSupport/WebFrameLoaderClient.h', u'WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm', u'WebKit/mac/WebView/WebDelegateImplementationCaching.h', u'WebKit/mac/WebView/WebDelegateImplementationCaching.mm', u'WebKit/mac/WebView/WebResourceLoadDelegatePrivate.h', u'WebKit/mac/WebView/WebView.mm']" exit_code: 1
Tools/DumpRenderTree/LayoutTestController.h:305:  The parameter name "shouldPaintBrokenImage" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 19 files


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

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/images/resources/broken-image-with-invalid-format.png', u'LayoutTests/fast/images/support-broken-image-delegate.html', u'LayoutTests/platform/mac/fast/images/support-broken-image-delegate-expected.txt', u'Tools/ChangeLog', u'Tools/DumpRenderTree/LayoutTestController.cpp', u'Tools/DumpRenderTree/LayoutTestController.h', u'Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm', u'WebCore/ChangeLog', u'WebCore/loader/FrameLoaderClient.h', u'WebCore/loader/cache/CachedImage.cpp', u'WebCore/loader/cache/CachedImage.h', u'WebKit/mac/ChangeLog', u'WebKit/mac/WebCoreSupport/WebFrameLoaderClient.h', u'WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm', u'WebKit/mac/WebView/WebDelegateImplementationCaching.h', u'WebKit/mac/WebView/WebDelegateImplementationCaching.mm', u'WebKit/mac/WebView/WebResourceLoadDelegatePrivate.h', u'WebKit/mac/WebView/WebView.mm']" exit_code: 1
Tools/DumpRenderTree/LayoutTestController.h:305:  The parameter name "shouldPaintBrokenImage" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 19 files


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

[Eric Seidel (no email)]

(Fixing a bug in the style-queue, please pardon the spam.)

[David Levin]

(In reply to comment #20)
> Attachment 78148 [details] did not pass style-queue:
> 
> Tools/DumpRenderTree/LayoutTestController.h:305:  The parameter name "shouldPaintBrokenImage" adds no information, so it should be removed.  [readability/parameter_name] [5]

Note that the stylebot was flagging a valid issue in the patch.

[Yongjun Zhang]

Created attachment 78883 [details]
fix style violation.

only log the message when shouldPaintBrokenImage() returns NO; this avoids changing results of existing layout tests with failed images.

[Eric Seidel (no email)]

Comment on attachment 77846 [details]
New patch addressing comments.

Cleared Darin Adler's review+ from obsolete attachment 77846 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 77849 [details]
change the delegate method to shouldPaintBrokenImageForURL:

Cleared Darin Adler's review+ from obsolete attachment 77849 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[David Kilzer (:ddkilzer)]

Comment on attachment 78883 [details]
fix style violation.

View in context: https://bugs.webkit.org/attachment.cgi?id=78883&action=review

r=me, but consider the comments.

> WebKit/mac/WebView/WebResourceLoadDelegatePrivate.h:59
> + @param imageURL The url of the offending image.

Maybe use "broken" instead of "offending" here.

> LayoutTests/fast/images/support-broken-image-delegate.html:26
> +<img id="broken" onError="loaded()" onLoad="loaded()" src="resources/broken-image-with-invalid-format.png">

Instead of calling loaded() in the onload="" attribute, can you call a different function that always sets result.innerHTML to "FAIL" since this code path is unexpected?  (Or is there a reason for setting onload and onerror to the same function?)

> LayoutTests/fast/images/resources/broken-image-with-invalid-format.png:1
> +fsadljf903q2asdfic-=29q24;lasdf

Is the MIME type for this file correct?  Strange that it shows up as text in the diff.  May need to fix this up in svn after the commit.

[Yongjun Zhang]

(In reply to comment #26)
> (From update of attachment 78883 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78883&action=review
> 
> r=me, but consider the comments.
> 
> > WebKit/mac/WebView/WebResourceLoadDelegatePrivate.h:59
> > + @param imageURL The url of the offending image.
> 
> Maybe use "broken" instead of "offending" here.
> 
Done.

> > LayoutTests/fast/images/support-broken-image-delegate.html:26
> > +<img id="broken" onError="loaded()" onLoad="loaded()" src="resources/broken-image-with-invalid-format.png">
> 
> Instead of calling loaded() in the onload="" attribute, can you call a different function that always sets result.innerHTML to "FAIL" since this code path is unexpected?  (Or is there a reason for setting onload and onerror to the same function?)
>

I wanted to reset shouldPaintBorkenImage flag and call layoutTestController.notifyDone() when the test fails, to avoid time out.  Agree it is better to move that to another function, please see the next patch. 
 
> > LayoutTests/fast/images/resources/broken-image-with-invalid-format.png:1
> > +fsadljf903q2asdfic-=29q24;lasdf
> 
> Is the MIME type for this file correct?  Strange that it shows up as text in the diff.  May need to fix this up in svn after the commit.

Yes, I manually added text in this file to make sure it is a broken png file :-)

[Yongjun Zhang]

Created attachment 83707 [details]
Addressing David's comments in the modified patch.

Added a function failed() in LayoutTests/fast/images/support-broken-image-delegate.html to handle failed case.

[David Kilzer (:ddkilzer)]

Comment on attachment 78883 [details]
fix style violation.

Clearing flags on the old patch and marking as obsolete.

[David Kilzer (:ddkilzer)]

Comment on attachment 83707 [details]
Addressing David's comments in the modified patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=83707&action=review

r=me

> LayoutTests/fast/images/support-broken-image-delegate.html:26
> +    result.innerHTML = "FAIL";

Nit: Would be nice to differentiate this FAIL message from the one in loaded():

    result.innerHTML = "FAIL: onLoad event fired. Expected onError event.";

[WebKit Review Bot]

Comment on attachment 83707 [details]
Addressing David's comments in the modified patch.

Rejecting attachment 83707 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 83707 [details]
Addressing David's comments in the modified patch.

Clearing flags on attachment: 83707

Committed r79771: <http://trac.webkit.org/changeset/79771>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This test is failing on Windows port:

http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r79771%20(9787)/results.html

--- /home/buildbot/slave/WebKit-BuildSlave/win-release-tests/build/layout-test-results/fast/images/support-broken-image-delegate-expected.txt	2011-02-26 02:13:43.615329500 -0800
+++ /home/buildbot/slave/WebKit-BuildSlave/win-release-tests/build/layout-test-results/fast/images/support-broken-image-delegate-actual.txt	2011-02-26 02:13:43.613329400 -0800
@@ -1,7 +1,6 @@
-resources/broken-image-with-invalid-format.png - willSendRequest <NSURLRequest URL resources/broken-image-with-invalid-format.png, main document URL support-broken-image-delegate.html, http method GET> redirectResponse (null)
+broken-image-with-invalid-format.png - willSendRequest <NSURLRequest URL broken-image-with-invalid-format.png, main document URL support-broken-image-delegate.html, http method GET> redirectResponse (null)
 <unknown> - didFinishLoading
-resources/broken-image-with-invalid-format.png - didReceiveResponse <NSURLResponse resources/broken-image-with-invalid-format.png, http status code 0>
-resources/broken-image-with-invalid-format.png - shouldPaintBrokenImage: NO
+broken-image-with-invalid-format.png - didReceiveResponse <NSURLResponse broken-image-with-invalid-format.png, http status code 0>
 Radar 8610908 - Add a resource delegate method to query if WebCore should paint the broken image.
 
-PASS
+FAIL

[Adam Roben (:aroben)]

(In reply to comment #34)
> This test is failing on Windows port:
> 
> http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r79771%20(9787)/results.html

I filed bug 55324 to cover the failure.