[webkit-reviews] review denied: [Bug 102878] Get main resource loads happening in the NetworkProcess : [Attachment 175497] Patch v2 - Fix EWS (boneheaded last minute edits before...) and add comments clarifying the temporary nature of this code.

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Brady Eidson <beidson at apple.com>'s
request for review:
Bug 102878: Get main resource loads happening in the NetworkProcess
https://bugs.webkit.org/show_bug.cgi?id=102878

Attachment 175497: Patch v2 - Fix EWS (boneheaded last minute edits before...)
and add comments clarifying the temporary nature of this code.
https://bugs.webkit.org/attachment.cgi?id=175497&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
> 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_nh
Lm2Gi__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.