99736 – Refactor CachedResourceLoader: add CachedResourceRequest

RESOLVED FIXED99736

Refactor CachedResourceLoader: add CachedResourceRequest

https://bugs.webkit.org/show_bug.cgi?id=99736

Summary Refactor CachedResourceLoader: add CachedResourceRequest

Marja Hölttä

Reported 2012-10-18 10:59:26 PDT

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).

Attachments
Patch (41.22 KB, patch) 2012-10-18 12:29 PDT, Marja Hölttä	no flags	Details Formatted Diff Diff
Patch (45.25 KB, patch) 2012-10-18 18:15 PDT, Marja Hölttä	no flags	Details Formatted Diff Diff
Patch (51.10 KB, patch) 2012-10-19 11:58 PDT, Marja Hölttä	no flags	Details Formatted Diff Diff
Patch (51.92 KB, patch) 2012-10-19 13:01 PDT, Marja Hölttä	no flags	Details Formatted Diff Diff
Patch (52.77 KB, patch) 2012-10-19 14:54 PDT, Marja Hölttä	no flags	Details Formatted Diff Diff
Patch (59.09 KB, patch) 2012-10-19 16:52 PDT, Marja Hölttä	no flags	Details Formatted Diff Diff
Patch (59.16 KB, patch) 2012-10-19 17:05 PDT, Marja Hölttä	no flags	Details Formatted Diff Diff
Patch (58.34 KB, patch) 2012-10-19 18:51 PDT, Marja Hölttä	no flags	Details Formatted Diff Diff
Patch (58.34 KB, patch) 2012-10-19 18:54 PDT, Marja Hölttä	no flags	Details Formatted Diff Diff
Patch (58.34 KB, patch) 2012-10-19 19:10 PDT, Marja Hölttä	no flags	Details Formatted Diff Diff
Patch (58.35 KB, patch) 2012-10-20 15:44 PDT, Marja Hölttä	no flags	Details Formatted Diff Diff
Patch (58.38 KB, patch) 2012-10-20 17:39 PDT, Marja Hölttä	no flags	Details Formatted Diff Diff
Patch (62.04 KB, patch) 2012-10-21 11:39 PDT, Marja Hölttä	no flags	Details Formatted Diff Diff
Patch (58.75 KB, patch) 2012-10-21 11:41 PDT, Marja Hölttä	no flags	Details Formatted Diff Diff
Show Obsolete (13) View All Add attachment proposed patch, testcase, etc.

Marja Hölttä

Comment 1 2012-10-18 12:29:18 PDT

Created attachment 169450 [details] Patch

jochen

Comment 2 2012-10-18 12:36:33 PDT

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 3 2012-10-18 13:00:09 PDT

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 4 2012-10-18 15:48:32 PDT

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

Comment 5 2012-10-18 15:49:12 PDT

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

Alexey Proskuryakov

Comment 6 2012-10-18 15:57:57 PDT

> 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

Comment 7 2012-10-18 15:58:35 PDT

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

Adam Barth

Comment 8 2012-10-18 16:07:50 PDT

> 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

Comment 9 2012-10-18 16:16:31 PDT

(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ä

Comment 10 2012-10-18 18:15:46 PDT

Created attachment 169525 [details] Patch

Marja Hölttä

Comment 11 2012-10-18 18:17:42 PDT

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

Gyuyoung Kim

Comment 12 2012-10-18 18:24:24 PDT

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 13 2012-10-18 18:25:06 PDT

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 14 2012-10-18 18:26:10 PDT

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 15 2012-10-18 18:27:16 PDT

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 16 2012-10-18 18:31:41 PDT

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 17 2012-10-18 19:45:19 PDT

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 18 2012-10-19 04:55:19 PDT

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ä

Comment 19 2012-10-19 11:58:31 PDT

Created attachment 169668 [details] Patch

Build Bot

Comment 20 2012-10-19 12:33:27 PDT

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 21 2012-10-19 12:46:44 PDT

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ä

Comment 22 2012-10-19 13:01:06 PDT

Created attachment 169679 [details] Patch

Marja Hölttä

Comment 23 2012-10-19 14:54:42 PDT

Created attachment 169698 [details] Patch

Build Bot

Comment 24 2012-10-19 15:02:25 PDT

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 25 2012-10-19 15:14:12 PDT

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 26 2012-10-19 15:38:06 PDT

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 27 2012-10-19 16:13:22 PDT

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 28 2012-10-19 16:14:34 PDT

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ä

Comment 29 2012-10-19 16:52:50 PDT

Created attachment 169726 [details] Patch

Marja Hölttä

Comment 30 2012-10-19 17:05:11 PDT

Created attachment 169730 [details] Patch

Build Bot

Comment 31 2012-10-19 17:20:34 PDT

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ä

Comment 32 2012-10-19 18:51:14 PDT

Created attachment 169740 [details] Patch

Marja Hölttä

Comment 33 2012-10-19 18:54:19 PDT

Created attachment 169741 [details] Patch

Build Bot

Comment 34 2012-10-19 18:58:48 PDT

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ä

Comment 35 2012-10-19 19:10:49 PDT

Created attachment 169744 [details] Patch

Adam Barth

Comment 36 2012-10-19 23:44:51 PDT

Comment on attachment 169744 [details] Patch Marking r- so the EWS will stop spinning.

Marja Hölttä

Comment 37 2012-10-20 15:44:01 PDT

Created attachment 169776 [details] Patch

Marja Hölttä

Comment 38 2012-10-20 17:14:42 PDT

Comment on attachment 169776 [details] Patch (still doesn't pass tests)

Marja Hölttä

Comment 39 2012-10-20 17:39:12 PDT

Created attachment 169777 [details] Patch

Marja Hölttä

Comment 40 2012-10-21 11:39:35 PDT

Created attachment 169809 [details] Patch

Marja Hölttä

Comment 41 2012-10-21 11:41:25 PDT

Created attachment 169810 [details] Patch

Adam Barth

Comment 42 2012-10-21 15:17:35 PDT

Green bubbles!

Adam Barth

Comment 43 2012-10-21 15:23:37 PDT

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 44 2012-10-22 16:21:21 PDT

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 45 2012-10-22 16:24:45 PDT

Comment on attachment 169810 [details] Patch SGTM. Thanks for closing the loop with ap.

WebKit Review Bot

Comment 46 2012-10-22 16:35:39 PDT

Comment on attachment 169810 [details] Patch Clearing flags on attachment: 169810 Committed r132157: <http://trac.webkit.org/changeset/132157>

WebKit Review Bot

Comment 47 2012-10-22 16:35:48 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component WebCore Misc.

Assignee

Marja Hölttä

Reported

2012-10-18 10:59 PDT

Modified

2012-11-05 22:24 PST History

CC List

21 users Show

URL

Keywords

Depends on

Blocks

101300

Dependencies

tree graph

abarth
allan.jensen
ap
beidson
cmarcelo
dglazkov
fmalita
gustavo
gyuyoung.kim
japhet
jochen
macpherson
menard
pdr
peter+ews
philn
rakuco
schenney
simonjam
webkit.review.bot
xan.lopez