[webkit-reviews] review denied: [Bug 68071] Rename FrameLoaderClient::allowImages to FrameLoaderClient::allowImage and include the image URL as parameter : [Attachment 107373] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied jochen at chromium.org's request for
review:
Bug 68071: Rename FrameLoaderClient::allowImages to
FrameLoaderClient::allowImage and include the image URL as parameter
https://bugs.webkit.org/show_bug.cgi?id=68071

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

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


This is looking good, but we probably should iterate another time.

> Source/WebCore/loader/FrameLoaderClient.h:307
>	   virtual bool allowJavaScript(bool enabledPerSettings) { return
enabledPerSettings; }
>	   virtual bool allowPlugins(bool enabledPerSettings) { return
enabledPerSettings; }
> -	   virtual bool allowImages(bool enabledPerSettings) { return
enabledPerSettings; }
> +	   virtual bool allowImage(const KURL& imageURL, bool
enabledPerSettings) { return enabledPerSettings; }
>	   virtual bool allowDisplayingInsecureContent(bool enabledPerSettings,
SecurityOrigin*, const KURL&) { return enabledPerSettings; }
>	   virtual bool allowRunningInsecureContent(bool enabledPerSettings,
SecurityOrigin*, const KURL&) { return enabledPerSettings; }

To make these consistent, perhaps the URL should be the last parameter?  Do we
also need a didNotAllowImage to parallel didNotAllowPlugins?  Perhaps there
isn't any code that needs to know whether images are allowed without being
about to load an image.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:131
> +	   KURL requestURL = request.url();
> +	   if (!f->loader()->client()->allowImage(requestURL, !settings ||
settings->areImagesEnabled()))

I see that the loading-from-the-MemoryCache case is handled here.

> Tools/DumpRenderTree/chromium/WebPermissions.h:42
> -    virtual bool allowImages(WebKit::WebFrame*, bool enabledPerSettings);
> +    virtual bool allowImage(WebKit::WebFrame*, const WebKit::WebURL&
imageURL, bool enabledPerSettings);

It would be ideal to add a LayoutTest to exercise this new policy lever.