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).
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?
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
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.
Your patch is also failing a number of Chromium unit tests: AssociatedURLLoaderTest.*
> 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?
(I'm not objecting against this change per se, FWIW)
> 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.
(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.
Created attachment 169525 [details] Patch
Thanks for comments; the new patch addresses them, and also fixes the chromium unit tests (hopefully the mac tests too).
Comment on attachment 169525 [details] Patch Attachment 169525 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14465211
Comment on attachment 169525 [details] Patch Attachment 169525 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14464223
Comment on attachment 169525 [details] Patch Attachment 169525 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14463231
Comment on attachment 169525 [details] Patch Attachment 169525 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14469044
Comment on attachment 169525 [details] Patch Attachment 169525 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14424739
Comment on attachment 169525 [details] Patch Attachment 169525 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14412832
Comment on attachment 169525 [details] Patch Attachment 169525 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14461413
Created attachment 169668 [details] Patch
Comment on attachment 169668 [details] Patch Attachment 169668 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14455641
Comment on attachment 169668 [details] Patch Attachment 169668 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14466525
Created attachment 169679 [details] Patch
Created attachment 169698 [details] Patch
Comment on attachment 169698 [details] Patch Attachment 169698 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14455685
Comment on attachment 169698 [details] Patch Attachment 169698 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14462607
Comment on attachment 169698 [details] Patch Attachment 169698 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14455698
Comment on attachment 169698 [details] Patch Attachment 169698 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14469417
Comment on attachment 169698 [details] Patch Attachment 169698 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14473152
Created attachment 169726 [details] Patch
Created attachment 169730 [details] Patch
Comment on attachment 169730 [details] Patch Attachment 169730 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14465670
Created attachment 169740 [details] Patch
Created attachment 169741 [details] Patch
Comment on attachment 169741 [details] Patch Attachment 169741 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14465692
Created attachment 169744 [details] Patch
Comment on attachment 169744 [details] Patch Marking r- so the EWS will stop spinning.
Created attachment 169776 [details] Patch
Comment on attachment 169776 [details] Patch (still doesn't pass tests)
Created attachment 169777 [details] Patch
Created attachment 169809 [details] Patch
Created attachment 169810 [details] Patch
Green bubbles!
Comment on attachment 169810 [details] Patch Thanks. This looks like a good step. Please give ap a chance to comment before landing the patch.
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 :)"
Comment on attachment 169810 [details] Patch SGTM. Thanks for closing the loop with ap.
Comment on attachment 169810 [details] Patch Clearing flags on attachment: 169810 Committed r132157: <http://trac.webkit.org/changeset/132157>
All reviewed patches have been landed. Closing bug.