Bug 99736 – Refactor CachedResourceLoader: add CachedResourceRequest

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Marja Hölttä]

For fixing bug 84883 and bug 92761, we need to keep track on which Element (or css / xhr) requests a resource.

For this, CachedResourceLoader::requestXYZ functions will be refactored so that they take a CachedResourceRequest as a parameter, instead of taking a long list of parameters.

Next steps: a new patch for bug 92761 will add an Element* to CachedResourceRequest (and create the CachedResourceRequest in a place where the Element* is known).

[Marja Hölttä]

Created attachment 169450 [details]
Patch

Comment on attachment 169450 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceLoader.h:72
> +    class CachedResourceRequest {

what's the reason for making this a sub class? Why not even put it into a separate file?

[Build Bot]

Comment on attachment 169450 [details]
Patch

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

New failing tests:
compositing/layers-inside-overflow-scroll.html
animations/animation-direction-normal.html
animations/3d/matrix-transform-type-animation.html
canvas/philip/tests/2d.clearRect.clip.html
animations/additive-transform-animations.html
canvas/philip/tests/2d.clearRect.zero.html
canvas/philip/tests/2d.clearRect.globalalpha.html
accessibility/accessibility-node-reparent.html
canvas/philip/tests/2d.clearRect.globalcomposite.html
animations/animation-border-overflow.html
animations/animation-direction-alternate-reverse.html
accessibility/accessibility-object-detached.html
canvas/philip/tests/2d.canvas.readonly.html
compositing/animation/computed-style-during-delay.html
compositing/geometry/clipped-video-controller.html
animations/3d/transform-perspective.html
canvas/philip/tests/2d.clearRect.transform.html
canvas/philip/tests/2d.clearRect.negative.html
compositing/animation/animated-composited-inside-hidden.html
canvas/philip/tests/2d.clearRect.path.html
canvas/philip/tests/2d.canvas.reference.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
canvas/philip/tests/2d.clearRect.basic.html
accessibility/accessibility-node-memory-management.html
animations/animation-css-rule-types.html
canvas/philip/tests/2d.clearRect.shadow.html
accessibility/adjacent-continuations-cause-assertion-failure.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html
canvas/philip/tests/2d.clearRect.nonfinite.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

[Adam Barth]

Comment on attachment 169450 [details]
Patch

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

This looks great.  Some comments below.  Also, it looks like your patch is causing a large number of test failures.

> Source/WebCore/ChangeLog:8
> +        This is needed for fixing bugs 84883 and 92761.

Please add more information to the ChangeLog that explains why we're making this change.

>> Source/WebCore/loader/cache/CachedResourceLoader.h:72
>> +    class CachedResourceRequest {
> 
> what's the reason for making this a sub class? Why not even put it into a separate file?

Yeah, we should put it in a separate CachedResourceRequest.h and in the WebCore namespace directly.

> Source/WebCore/loader/cache/CachedResourceLoader.h:82
> +        ResourceRequest& resourceRequest() { return m_resourceRequest; }

resourceRequest -> mutableResourceRequest to emphasize that this is a non-const reference.

> Source/WebCore/loader/cache/CachedResourceLoader.h:89
> +    private:

Please add a blank line above this line.

[Adam Barth]

Your patch is also failing a number of Chromium unit tests:

AssociatedURLLoaderTest.*

[Alexey Proskuryakov]

> For fixing bug 84883 and bug 92761, we need to keep track on which Element (or css / xhr) requests a resource.

This rationale seems conceptually weak, at least on the surface. What if multiple elements request the same resource, for example? Or if a preload is started before any element is created?

[Alexey Proskuryakov]

(I'm not objecting against this change per se, FWIW)

[Adam Barth]

> This rationale seems conceptually weak, at least on the surface. What if multiple elements request the same resource, for example? Or if a preload is started before any element is created?

That's the problem that we're hoping to solve with CachedResourceRequest.  Each of the requesters will have a separate CachedResourceRequest so we can sensibly disentangle their separate requests from the single response they all get back.

[James Simonsen]

(In reply to comment #6)
> > For fixing bug 84883 and bug 92761, we need to keep track on which Element (or css / xhr) requests a resource.
> 
> This rationale seems conceptually weak, at least on the surface. What if multiple elements request the same resource, for example? Or if a preload is started before any element is created?

For 84883, these cases are tightly defined:

1. The first element wins.

2. We don't need the actual element, just its name and document.

[Marja Hölttä]

Created attachment 169525 [details]
Patch

[Marja Hölttä]

Thanks for comments; the new patch addresses them, and also fixes the chromium unit tests (hopefully the mac tests too).

[Gyuyoung Kim]

Comment on attachment 169525 [details]
Patch

Attachment 169525 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14465211

[Early Warning System Bot]

Comment on attachment 169525 [details]
Patch

Attachment 169525 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14464223

[WebKit Review Bot]

Comment on attachment 169525 [details]
Patch

Attachment 169525 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14463231

[Early Warning System Bot]

Comment on attachment 169525 [details]
Patch

Attachment 169525 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14469044

[Peter Beverloo (cr-android ews)]

Comment on attachment 169525 [details]
Patch

Attachment 169525 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14424739

[Build Bot]

Comment on attachment 169525 [details]
Patch

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

[Build Bot]

Comment on attachment 169525 [details]
Patch

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

[Marja Hölttä]

Created attachment 169668 [details]
Patch

[Build Bot]

Comment on attachment 169668 [details]
Patch

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

[Build Bot]

Comment on attachment 169668 [details]
Patch

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

[Marja Hölttä]

Created attachment 169679 [details]
Patch

[Marja Hölttä]

Created attachment 169698 [details]
Patch

[Build Bot]

Comment on attachment 169698 [details]
Patch

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

[Build Bot]

Comment on attachment 169698 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 169698 [details]
Patch

Attachment 169698 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14455698

[Early Warning System Bot]

Comment on attachment 169698 [details]
Patch

Attachment 169698 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14469417

[Gyuyoung Kim]

Comment on attachment 169698 [details]
Patch

Attachment 169698 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14473152

[Marja Hölttä]

Created attachment 169726 [details]
Patch

[Marja Hölttä]

Created attachment 169730 [details]
Patch

[Build Bot]

Comment on attachment 169730 [details]
Patch

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

[Marja Hölttä]

Created attachment 169740 [details]
Patch

[Marja Hölttä]

Created attachment 169741 [details]
Patch

[Build Bot]

Comment on attachment 169741 [details]
Patch

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

[Marja Hölttä]

Created attachment 169744 [details]
Patch

[Adam Barth]

Comment on attachment 169744 [details]
Patch

Marking r- so the EWS will stop spinning.

[Marja Hölttä]

Created attachment 169776 [details]
Patch

[Marja Hölttä]

Comment on attachment 169776 [details]
Patch

(still doesn't pass tests)

[Marja Hölttä]

Created attachment 169777 [details]
Patch

[Marja Hölttä]

Created attachment 169809 [details]
Patch

[Marja Hölttä]

Created attachment 169810 [details]
Patch

[Adam Barth]

Green bubbles!

[Adam Barth]

Comment on attachment 169810 [details]
Patch

Thanks.  This looks like a good step.

Please give ap a chance to comment before landing the patch.

[Marja Hölttä]

Comment on attachment 169810 [details]
Patch

IRCed with ap, he says: "The idea of the patch seems good to me regardless of the goal, we do need a class that encapsulates a request with web-pertinent knowledge. I won't have any time soon to deeply review, so this makes it an ack :)"

[Adam Barth]

Comment on attachment 169810 [details]
Patch

SGTM.  Thanks for closing the loop with ap.

[WebKit Review Bot]

Comment on attachment 169810 [details]
Patch

Clearing flags on attachment: 169810

Committed r132157: <http://trac.webkit.org/changeset/132157>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.