Bug 67898 - REGRESSION(r94900): fast/images/support-broken-image-delegate.html fails on Mac
Summary: REGRESSION(r94900): fast/images/support-broken-image-delegate.html fails on Mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords:
Depends on: 67819
Blocks:
  Show dependency tree / graph
 
Reported: 2011-09-10 17:07 PDT by Ryosuke Niwa
Modified: 2011-09-12 14:51 PDT (History)
8 users (show)

See Also:

Attachments
Patch (3.62 KB, patch)
2011-09-10 18:01 PDT, Beth Dakin 		simon.fraser: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Ryosuke Niwa 2011-09-10 17:16:58 PDT 
Temporarily skipped the test in http://trac.webkit.org/changeset/94915.
Comment 2 Beth Dakin 2011-09-10 17:36:09 PDT 
Thanks so much Ryosuke. 

I looked at this a bit, and I think I understand the problem. I changed RenderImage::imageSizeForError() so that instead of accessing CachedImage::image() and then measuring the size, it goes directly to CachedImage::brokenImage(). I think this test shows that that is wrong because in this case m_shouldPaintBrokenImage would be false and rather than returning the brokenImage,  CachedImage::image() would have returned the null image. 

I think I have time to post a patch for this before I have to go.
Comment 3 Beth Dakin 2011-09-10 18:01:43 PDT 
Created attachment 106993 [details]
Patch

Here's a patch to fix the test failure. Maybe check check for willPaintBrokenImage() should be inside brokenImage() itself…not sure…for the time being, I made it an independent function.
Comment 4 Ryosuke Niwa 2011-09-10 18:20:32 PDT 
cc more people in the hope they can review this patch.
Comment 5 WebKit Review Bot 2011-09-11 14:07:20 PDT 
Comment on attachment 106993 [details]
Patch

Attachment 106993 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9641169

New failing tests:
svg/css/getComputedStyle-basic.xhtml
svg/custom/svg-fonts-word-spacing.html
media/video-zoom-controls.html
Comment 6 Simon Fraser (smfr) 2011-09-11 22:35:56 PDT 
Comment on attachment 106993 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        REGRESSION(r94900): fast/images/support-broken-image-delegate.html fails on Mac
> +
> +        No new tests. (OOPS!)

Some verbage here describing the change would be good.
Comment 7 Beth Dakin 2011-09-12 14:38:17 PDT 
(In reply to comment #6)
> (From update of attachment 106993 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106993&action=review
> 
> > Source/WebCore/ChangeLog:6
> > +        REGRESSION(r94900): fast/images/support-broken-image-delegate.html fails on Mac
> > +
> > +        No new tests. (OOPS!)
> 
> Some verbage here describing the change would be good.

Thanks, Simon! I will add some verbiage.

I cannot reproduce those test failures that the Bot claims are failures. It would also be very surprising if this change caused failures in those tests anyway. I plan to commit the change and monitor the build bots.
Comment 8 Beth Dakin 2011-09-12 14:51:22 PDT 
Committed change with revision 94980.