[webkit-reviews] review denied: [Bug 92761] Add callbacks to the FrameLoaderClient for when a resource request finished for a script or image element : [Attachment 155773] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied jochen at chromium.org's request for
review:
Bug 92761: Add callbacks to the FrameLoaderClient for when a resource request
finished for a script or image element
https://bugs.webkit.org/show_bug.cgi?id=92761

Attachment 155773: Patch
https://bugs.webkit.org/attachment.cgi?id=155773&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=155773&action=review


I'm tempted to r+ this with comments, but I think I'd like to see one more
round, if that's ok with you.

> Source/WebCore/loader/FrameLoaderClient.h:167
> +	   virtual void dispatchImageLoadFinished(Element*, CachedResource*) {
}
> +	   virtual void dispatchScriptLoadFinished(Element*, CachedResource*) {
}

Why not provide more concerete types?  e.g.,
dispatchImageLoadFinished(Element*, CachedImage*) { }

> Source/WebCore/loader/ImageLoader.cpp:270
> +   
document()->loader()->frameLoader()->client()->dispatchImageLoadFinished(client
()->sourceElement(), resource);

Going through the DocumentLoader for these calls doesn't seem quite right.  It
makes more sense to me to go through the Frame:

document()->frame()->loader()->client()->dispatchImageLoadFinished(client()->so
urceElement(), resource);

Also, do we need to null-check the frame?

> Source/WebKit/chromium/public/WebFrameClient.h:227
> +    // The URL request for the given image element finished loading.
> +    virtual void imageLoadFinished(WebFrame*, const WebElement&, const
WebURLRequest&) { }

imageLoadFinished -> didFinishImageLoad

That fits the naming pattern of the functions above.

> Source/WebKit/chromium/public/WebFrameClient.h:230
> +    // The URL request for the given script element finished loading.
> +    virtual void scriptLoadFinished(WebFrame*, const WebElement&, const
WebURLRequest&) { }

ditto

> Source/WebKit/chromium/src/FrameLoaderClientImpl.h:111
> +    virtual void dispatchImageLoadFinished(WebCore::Element* sourceElement,
WebCore::CachedResource*);
> +    virtual void dispatchScriptLoadFinished(WebCore::Element* sourceElement,
WebCore::CachedResource*);

These should also probably be like dispatchDidFinishImageLoad to match the
similar callbacks above.