Bug 102878 – Get main resource loads happening in the NetworkProcess

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit2

[Brady Eidson]

Get main resource loads happening in the NetworkProcess.

The approach for this right now is simple and probably not great for the long term, but it is important to unblock other NetworkProcess related tasks in the short term.

Patch forthcoming.

[Brady Eidson]

Created attachment 175342 [details]
Patch v1

[Build Bot]

Comment on attachment 175342 [details]
Patch v1

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

[WebKit Review Bot]

Comment on attachment 175342 [details]
Patch v1

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

[Adam Barth]

Comment on attachment 175342 [details]
Patch v1

Once we load main resources through the CachedResourceLoader, we won't need to have this special-case path for main resources.  Adding this code now makes it harder to move main resources to loading through the CachedResourceLoader, so we should move to using the CachedResourceLoader first.

[Adam Barth]

Specifically, main resources will be able to use the general code in CachedResource::load, just like every other resource.

[Brady Eidson]

(In reply to comment #4)
> (From update of attachment 175342 [details])
> Once we load main resources through the CachedResourceLoader, we won't need to have this special-case path for main resources.  Adding this code now makes it harder to move main resources to loading through the CachedResourceLoader, so we should move to using the CachedResourceLoader first.

Believe me, I am very, very aware of this.  That's why I've been watching that bug closely for weeks, if not months.

We need this temporary fix in place for now.  The "MainResource -> CachedResource" bug can simply remove this code.

I'm going to clean up the EWS and resubmit.

[Adam Barth]

> We need this temporary fix in place for now.

That's not consistent with the approach you laid out in <http://lists.webkit.org/pipermail/webkit-dev/2012-October/022444.html>.  Specifically, you wrote:

"I don't plan to shoehorn in changes, but rather to make sensical refactoring that cleans up the code for all ports."

Now you're asking to shoehorn this feature in without the proper refactoring, which is to finish the work in bug 49246.

[Brady Eidson]

(In reply to comment #7)
> > We need this temporary fix in place for now.
> 
> That's not consistent with the approach you laid out in <http://lists.webkit.org/pipermail/webkit-dev/2012-October/022444.html>.  Specifically, you wrote:
> 
> "I don't plan to shoehorn in changes, but rather to make sensical refactoring that cleans up the code for all ports."
> 
> Now you're asking to shoehorn this feature in without the proper refactoring, which is to finish the work in bug 49246.

I did say that and I did mean it, and do still mean it.

I also think you are mischaracterizing this change.

Your stance here seems to be that transient code can never be landed in the project while a major port feature that requires a lot of iteration is being worked on.  I'm not sure if that is the stance of the project or that it should be.

[Brady Eidson]

Created attachment 175497 [details]
Patch v2 - Fix EWS (boneheaded last minute edits before...) and add comments clarifying the temporary nature of this code.

[Sam Weinig]

Given how small the change is, and the clear direction of how to improve things, and the fact that this enables us to test loading through the NetworkProcess much more thoroughly, I would like to see this patch land.  Given its size, I don't think it will all negatively affect the changes to the MainResourceLoader to be a cached resource.

Adam, if you still object, please let us know (I won't r+ until I hear back from you).  We do not want to have hacks in the code, and that is why both Maciej and I are pushing for the other MainResourceLoader changes to be made ASAP.

[Adam Barth]

Comment on attachment 175497 [details]
Patch v2 - Fix EWS (boneheaded last minute edits before...) and add comments clarifying the temporary nature of this code.

> Your stance here seems to be that transient code can never be landed in the project while a major port feature that requires a lot of iteration is being worked on.  I'm not sure if that is the stance of the project or that it should be.

That's not what I'm saying.

> Given how small the change is, and the clear direction of how to improve things, and the fact that this enables us to test loading through the NetworkProcess much more thoroughly, I would like to see this patch land.  Given its size, I don't think it will all negatively affect the changes to the MainResourceLoader to be a cached resource.

The size of the change isn't at issue.  The issue is that it's a poor design.

As I'm sure you're aware, the loader has, historically, been a messy piece of code.  Over the past serveral years, we've been working to straighten it out, and out of that work has emerged a clearer layering diagram, which I recently drew on this slide:

https://docs.google.com/presentation/pub?id=1ZRIQbUKw9Tf077odCh66OrrwRIVNLvI_nhLm2Gi__F0#slide=id.g30f0fc55_0_90

In implementing an out-of-process network stack, we need to decide at which layer of the loader we should intercept requests and proxy them out of process.  Chromium intercepts request at the ResourceHandle layer for a number of reasons that we can discuss if you're interested.

Based on the previous discussion of the work on WebKit2's NetworkProces, my understanding is that your plan is to intercept network requests at the CachedResource layer.  As I mentioned when this work started, I don't think that's necessarily the best choice, but I can understand why you've chosen that approach given that you want to have a shared memory MemoryCache.

One of the problems with intercepting requests at the CachedResource layer is that not all requests flow through the CachedResourceLoader.  That's something we're actively working on improving, for reasons that I think we all agree are worthwhile.

My issue with this patch is that adds a new interception point in a new layer, the ResourceLoader layer.  I understand why you've written this patch: the loader is poorly factored and not all requests flow through the layer you're using to intercept the requests.  However, the correct thing to do is to refactor how the loader works to make these requests flow though the CachedResourceLoader, not to shoehorn interception into another layer.

Part of my concern with this work initially was that the loader is messy and there's a strong temptation in doing work like this to add to the mess rather than doing the hard refactoring work.  That's why Brady's commitment to do this work the right way is important to me, and why I would like to see him follow though on that even when it's not expedient at the moment.

> We do not want to have hacks in the code, and that is why both Maciej and I are pushing for the other MainResourceLoader changes to be made ASAP.

It seems you agree that this patch is a hack.  Rather than shoehorning it in, we should engineer this feature correctly and do the necessary refactoring before adding to the mess.

[Sam Weinig]

We just wanted something temporary, but I don't think it is worth the time to debate the merits of it.  We'll just fix up MainResourceLoader and that will be that.