Bug 34314 - Add a test for FrameLoaderClient::allowImages
Summary: Add a test for FrameLoaderClient::allowImages
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: jochen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-29 00:47 PST by Darin Fisher (:fishd, Google)
Modified: 2011-06-01 15:57 PDT (History)
3 users (show)

See Also:


Attachments
Patch (85.82 KB, patch)
2011-05-31 01:41 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (161.32 KB, patch)
2011-05-31 02:23 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (87.16 KB, patch)
2011-06-01 11:39 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (87.18 KB, patch)
2011-06-01 11:58 PDT, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Fisher (:fishd, Google) 2010-01-29 00:47:35 PST
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.
Comment 1 jochen 2011-05-31 01:41:01 PDT
Created attachment 95405 [details]
Patch
Comment 2 jochen 2011-05-31 01:43:12 PDT
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
Comment 3 jochen 2011-05-31 02:23:30 PDT
Created attachment 95408 [details]
Patch
Comment 4 Adam Barth 2011-05-31 12:47:40 PDT
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 5 jochen 2011-05-31 12:56:38 PDT
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
Comment 6 jochen 2011-06-01 11:39:13 PDT
Created attachment 95636 [details]
Patch
Comment 7 jochen 2011-06-01 11:58:56 PDT
Created attachment 95639 [details]
Patch
Comment 8 WebKit Commit Bot 2011-06-01 15:57:47 PDT
Comment on attachment 95639 [details]
Patch

Clearing flags on attachment: 95639

Committed r87860: <http://trac.webkit.org/changeset/87860>
Comment 9 WebKit Commit Bot 2011-06-01 15:57:54 PDT
All reviewed patches have been landed.  Closing bug.