RESOLVED FIXED 48781
Add a resourceload delegate method to query if WebCore should paint the default broken image for failed images.
https://bugs.webkit.org/show_bug.cgi?id=48781
Summary Add a resourceload delegate method to query if WebCore should paint the defau...
Yongjun Zhang
Reported 2010-11-01 14:23:49 PDT
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.
Attachments
Add new method shouldPaintBrokenImageWithError for FrameLoaderClient; (30.23 KB, patch)
2010-11-01 15:46 PDT, Yongjun Zhang
no flags
Fix build break for GTK and Chromium. (32.09 KB, patch)
2010-11-02 09:28 PDT, Yongjun Zhang
abarth: review-
New patch addressing review comments. (36.65 KB, patch)
2010-11-08 12:42 PST, Yongjun Zhang
darin: review-
New patch addressing comments. (19.73 KB, patch)
2011-01-03 14:10 PST, Yongjun Zhang
no flags
change the delegate method to shouldPaintBrokenImageForURL: (19.78 KB, patch)
2011-01-03 14:51 PST, Yongjun Zhang
no flags
Only log the message when shouldPaintBrokenImage() returns NO; this avoids changing results of existing layout tests with failed images. (19.93 KB, patch)
2011-01-06 12:21 PST, Yongjun Zhang
no flags
fix style violation. (20.01 KB, patch)
2011-01-13 19:20 PST, Yongjun Zhang
no flags
Addressing David's comments in the modified patch. (19.80 KB, patch)
2011-02-24 13:03 PST, Yongjun Zhang
no flags
Yongjun Zhang
Comment 1 2010-11-01 14:26:01 PDT
Yongjun Zhang
Comment 2 2010-11-01 15:46:38 PDT
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
Comment 3 2010-11-01 16:56:45 PDT
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
Comment 4 2010-11-02 00:26:05 PDT
WebKit Review Bot
Comment 5 2010-11-02 03:37:36 PDT
Yongjun Zhang
Comment 6 2010-11-02 09:28:50 PDT
Created attachment 72676 [details] Fix build break for GTK and Chromium.
Yongjun Zhang
Comment 7 2010-11-02 09:34:34 PDT
(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
Comment 8 2010-11-02 11:13:52 PDT
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 9 2010-11-04 15:51:42 PDT
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
Comment 10 2010-11-08 12:42:58 PST
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
Comment 11 2010-12-07 16:55:36 PST
Any updates?
Darin Adler
Comment 12 2010-12-29 17:46:11 PST
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
Comment 13 2011-01-03 14:10:40 PST
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 14 2011-01-03 14:18:28 PST
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
Comment 15 2011-01-03 14:51:47 PST
Created attachment 77849 [details] change the delegate method to shouldPaintBrokenImageForURL: Yes, shouldPaintBrokenImageForURL is better. Please see the revised patch.
Darin Adler
Comment 16 2011-01-03 15:15:02 PST
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 17 2011-01-05 17:04:21 PST
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
Comment 18 2011-01-06 12:21:56 PST
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
Comment 19 2011-01-06 19:58:17 PST
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
Comment 20 2011-01-06 20:00:38 PST
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)
Comment 21 2011-01-06 20:01:38 PST
(Fixing a bug in the style-queue, please pardon the spam.)
David Levin
Comment 22 2011-01-11 19:46:37 PST
(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
Comment 23 2011-01-13 19:20:33 PST
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 24 2011-01-31 16:06:47 PST
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 25 2011-01-31 16:06:51 PST
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 26 2011-02-22 11:17:02 PST
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
Comment 27 2011-02-24 13:00:13 PST
(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
Comment 28 2011-02-24 13:03:35 PST
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 29 2011-02-24 14:06:31 PST
Comment on attachment 78883 [details] fix style violation. Clearing flags on the old patch and marking as obsolete.
David Kilzer (:ddkilzer)
Comment 30 2011-02-24 14:11:43 PST
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 31 2011-02-24 19:06:33 PST
Comment on attachment 83707 [details] Addressing David's comments in the modified patch. Rejecting attachment 83707 [details] from commit-queue. yongjun_zhang@apple.com 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 32 2011-02-26 01:32:35 PST
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
Comment 33 2011-02-26 01:32:43 PST
All reviewed patches have been landed. Closing bug.
Ryosuke Niwa
Comment 34 2011-02-26 06:09:54 PST
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)
Comment 35 2011-02-27 10:32:40 PST
(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.
Note You need to log in before you can comment on or make changes to this bug.