[webkit-reviews] review denied: [Bug 48781] Add a resourceload delegate method to query if WebCore should paint the default broken image for failed images. : [Attachment 72676] Fix build break for GTK and Chromium.

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Yongjun Zhang
<yongjun_zhang at apple.com>'s request for review:
Bug 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

Attachment 72676: Fix build break for GTK and Chromium.
https://bugs.webkit.org/attachment.cgi?id=72676&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.