WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Fix build break for GTK and Chromium.
(32.09 KB, patch)
2010-11-02 09:28 PDT
,
Yongjun Zhang
abarth
: review-
Details
Formatted Diff
Diff
New patch addressing review comments.
(36.65 KB, patch)
2010-11-08 12:42 PST
,
Yongjun Zhang
darin
: review-
Details
Formatted Diff
Diff
New patch addressing comments.
(19.73 KB, patch)
2011-01-03 14:10 PST
,
Yongjun Zhang
no flags
Details
Formatted Diff
Diff
change the delegate method to shouldPaintBrokenImageForURL:
(19.78 KB, patch)
2011-01-03 14:51 PST
,
Yongjun Zhang
no flags
Details
Formatted Diff
Diff
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
Details
Formatted Diff
Diff
fix style violation.
(20.01 KB, patch)
2011-01-13 19:20 PST
,
Yongjun Zhang
no flags
Details
Formatted Diff
Diff
Addressing David's comments in the modified patch.
(19.80 KB, patch)
2011-02-24 13:03 PST
,
Yongjun Zhang
no flags
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Yongjun Zhang
Comment 1
2010-11-01 14:26:01 PDT
<
rdar://problem/8610908
>
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
Attachment 72577
[details]
did not build on chromium: Build output:
http://queues.webkit.org/results/4960011
WebKit Review Bot
Comment 5
2010-11-02 03:37:36 PDT
Attachment 72577
[details]
did not build on gtk: Build output:
http://queues.webkit.org/results/5007007
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.
Top of Page
Format For Printing
XML
Clone This Bug