Bug 34314 – Add a test for FrameLoaderClient::allowImages

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Darin Fisher (:fishd, Google)]

Add a test for FrameLoaderClient::allowImages

Similar to https://bugs.webkit.org/show_bug.cgi?id=33991, there is no
mechanism yet for testing this.  We need to add one, and then write
some tests.

The tests should cover at least these three cases:

1)  Load an inline image (not from cache).
2)  Load an inline image (from cache).
3)  Load an image into an iframe.

Created attachment 95405 [details]
Patch

Comment on attachment 95405 [details]
Patch

it's quite difficult to detect that an image is not loaded because we're not sending the onerror signal on purpose. So I'm just testing that neither onerror nor onloaded gets called

Created attachment 95408 [details]
Patch

[Adam Barth]

Comment on attachment 95408 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95408&action=review

> LayoutTests/platform/chromium/permissionclient/image-permissions.html:24
> +    img.src = "resources/boston2.gif";

You don't actually need a separate image.  You can just use a query parameter.  We have a bunch of images you can use for tests already.  Do we the rights to the image you're adding?

> Tools/DumpRenderTree/chromium/WebPermissions.cpp:36
> +WebPermissions::WebPermissions()
> +    : m_imagesAllowed(true)
> +    , m_storageAllowed(true)

Should this just call reset() ?

> Tools/DumpRenderTree/chromium/WebPermissions.cpp:52
> +bool WebPermissions::allowImages(WebKit::WebFrame*, bool enabledPerSettings)
> +{
> +    return enabledPerSettings && m_imagesAllowed;
> +}
> +
> +bool WebPermissions::allowStorage(WebKit::WebFrame*, bool)
> +{
> +    return m_storageAllowed;
> +}

Why doesn't allowStorage use enabledPerSettings as well?

Comment on attachment 95408 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95408&action=review

>> LayoutTests/platform/chromium/permissionclient/image-permissions.html:24
>> +    img.src = "resources/boston2.gif";
> 
> You don't actually need a separate image.  You can just use a query parameter.  We have a bunch of images you can use for tests already.  Do we the rights to the image you're adding?

the image is from fast/$something/resources

>> Tools/DumpRenderTree/chromium/WebPermissions.cpp:36
>> +    , m_storageAllowed(true)
> 
> Should this just call reset() ?

i can see advantages for both. I'll change it to reset

>> Tools/DumpRenderTree/chromium/WebPermissions.cpp:52
>> +}
> 
> Why doesn't allowStorage use enabledPerSettings as well?

the second parameter is "local" as in localStorage vs sessionStorage. There's no setting that controls storage

Created attachment 95636 [details]
Patch

Created attachment 95639 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 95639 [details]
Patch

Clearing flags on attachment: 95639

Committed r87860: <http://trac.webkit.org/changeset/87860>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.