Bug 102808 – Add tests for CachedResourceRequest initiators

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Marja Hölttä]

Bug 101935 added initiators to CachedResourceRequests (for simple cases), and this adds tests.

When / if resource timing is implemented, there will be more tests (and these might be obsolete).

[Marja Hölttä]

Created attachment 175198 [details]
Patch

Comment on attachment 175198 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceLoader.h:163
> +    void startTrackingCahcedResourceRequests() { m_shouldTrackRequests = true; }

Cahced -> Cached

> LayoutTests/fast/loader/cached-resource-request-callbacks.html:16
> +internals.startTrackingCachedResourceRequests();

can you write the test such that it fails gracefully if you don't run it in DRT?

i.e. something like if (!window.internals) testFailed("Requires DRT to run");

> LayoutTests/fast/loader/cached-resource-request-callbacks.html:20
> +"empty-script.js SCRIPT normal-script-tag",

nit. indent 4 spaces

> LayoutTests/fast/loader/cached-resource-request-callbacks.html:60
> +       if (contains(requests[i], expected[expectedIx])) {

nit. strange indent? 7 spaces, should be 8

also, no { } required for single line if bodies

> LayoutTests/fast/loader/cached-resource-request-callbacks.html:81
> +  This test checks that the initiators of CachedResourceRequests are set correctly.

indent 4 spaces

Comment on attachment 175198 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceLoader.h:184
> +    Vector<String> m_trackedRequests;

we could consider to turn this into an OwnPtr to save some bytes when not running tests

[Marja Hölttä]

Created attachment 175204 [details]
Patch.

[Marja Hölttä]

Created attachment 175206 [details]
Patch.

[Marja Hölttä]

Created attachment 175207 [details]
Patch.

[Marja Hölttä]

(The latest patch addresses all comments.)

Comment on attachment 175204 [details]
Patch.

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

> Source/WebCore/ChangeLog:17
> +        (WebCore::CachedResourceLoader::startTrackingCahcedResourceRequests):

Cahced

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:951
> +        m_trackedRequests = adoptPtr(new Vector<String>);

should we maybe reset the tracked requests when we start tracking them again?

> LayoutTests/fast/loader/cached-resource-request-callbacks.html:38
> +    document.getElementById("with-background").style.backgroundImage="url(../images/resources/palatted-color-png-gamma-one-color-profile.png)";

nit. spaces around =

> LayoutTests/fast/loader/cached-resource-request-callbacks.html:63
> +    if (expectedIx == expected.length) {

no { } for single lines ifs and elses

> LayoutTests/fast/loader/cached-resource-request-callbacks.html:102
> +<img id="img-fail" />

can you add another image that has a URL which points to a non-existant file?

[Build Bot]

Comment on attachment 175207 [details]
Patch.

Attachment 175207 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14939006

[Build Bot]

Comment on attachment 175204 [details]
Patch.

Attachment 175204 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14916602

[kov's GTK+ EWS bot]

Comment on attachment 175204 [details]
Patch.

Attachment 175204 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14916604

[kov's GTK+ EWS bot]

Comment on attachment 175207 [details]
Patch.

Attachment 175207 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14905623

[Build Bot]

Comment on attachment 175207 [details]
Patch.

Attachment 175207 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14911645

[Build Bot]

Comment on attachment 175204 [details]
Patch.

Attachment 175204 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14910632

[Marja Hölttä]

Created attachment 175239 [details]
Patch

[Marja Hölttä]

Thanks for comments again, the latest patch fixes them.

[Adam Barth]

Comment on attachment 175239 [details]
Patch

This is too much test code in the production binary.  I thought you were going to add a willSendRequest equivalent for CachedResourceRequests.  Can't we go further down that path and get something that tests this code the way we plan to use it?

[Marja Hölttä]

I'll close this, and I'll add tests as part of reviving bug 92761.