[Webkit-unassigned] [Bug 48781] Add a resourceload delegate method to query if WebCore should paint the default broken image for failed images.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=48781


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #73268|review?                     |review-
               Flag|                            |




--- Comment #12 from Darin Adler <darin at apple.com>  2010-12-29 17:46:11 PST ---
(From update of attachment 73268)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.