[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 73268] New patch addressing review comments.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> 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 73268: New patch addressing review comments.
https://bugs.webkit.org/attachment.cgi?id=73268&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.