Bug 65855 – Add a ResourceRequestBase::Context class that is part of a ResourceRequest

• Component: Page Loading
• Status: NEW

Add empty header for LoadRequest

Created attachment 103238 [details]
Patch

Alexey, could you have a look please?

[Alexey Proskuryakov]

I think that making a "fatter" ResourceRequest could be a viable start for refactoring lower level networking code, but I would like to know more about the next steps.

We have a number of classes incorporating "request" data and/or logic. Can you describe how LoadRequest will be different form each of these?

ResourceRequest
ResourceHandle
FrameLoadRequest
CachedResourceRequest
ScheduledNavigation
ResourceLoader

In particular, is your plan to have ResourceLoader contain LoadRequest instead of ResourceRequest, and move data members such as m_defersLoading into LoadRequest?

My rough plan is to make LoadRequest look almost like FrameLoadRequest, except for that it doesn't have a security origin nor a name, but a target type instead.

FrameLoadRequest will the inherit from LoadRequest and add the security origin and the frame name to it.

Everything outside of platform/network would then pass around LoadRequests instead of ResourceRequests

(In reply to comment #3)
> I think that making a "fatter" ResourceRequest could be a viable start for refactoring lower level networking code, but I would like to know more about the next steps.
> 
> We have a number of classes incorporating "request" data and/or logic. Can you describe how LoadRequest will be different form each of these?
> 
> ResourceRequest

LoadRequest will contain a ResourceRequest as data member

> ResourceHandle

Being part of platform/network, ResourceHandle won't know about LoadRequest

> FrameLoadRequest

This will inherit from LoadRequest

> CachedResourceRequest

CachedResoure will include a LoadRequest instead of a ResourceRequest, and CachedResourceRequest will therefore also deal with LoadRequests

> ScheduledNavigation

this will still deal with FrameLoadRequests

> ResourceLoader

this will deal with LoadRequests instead of ResourceRequests

of course when passing a LoadRequest to an api in platform/network, the respective class has to unwrap the ResourceRequest from the LoadRequest

> 
> In particular, is your plan to have ResourceLoader contain LoadRequest instead of ResourceRequest, and move data members such as m_defersLoading into LoadRequest?

My current plan doesn't include moving additional data fields. However, if it makes sense to do so, this could certainly be done

I hope that makes sense, wdyt?

[Alexey Proskuryakov]

Thank you. I'd like to ask Maciej and Darin for their opinion on this plan at this point.

For some background, see also bug 48483 - I'm trying to move ResourceRequest::m_targetType to a higher layer. Right now, it's part of ResourceRequest that shouldn't care about the target type.

[Darin Adler]

This is not good naming.

The point of the name “resource load request” is that it’s everything needed to load a resource.

The class “frame load request” then adds to the resource request everything that you need to load a frame.

If we add have something else named “load request” that is also everything needed to load a resource, then things get really confusing. Removing the adjective makes the class vague in purpose.

We’re going to have to talk about this more to figure out what this class really is so we can give it a suitable name.

I guess the real issue here is that you at Google don’t want our loading built on top of platform/network and want your own out-of-process networking library that requires higher level context, like which frame is doing the call. If so, then the naming needs to make clear that distinction rather than just using the rule “if I remove the word Resource then I am not talking about platform/network”.

We could try to rename the platform/network code instead so we have the shorter names for the high level concept in the loader. The word “resource” is not the right word, though, to indicate it’s part of the low-level loader in the platform directory.

[Darin Fisher (:fishd, Google)]

I agree that LoadRequest is not such a great name.  That was my suggestion, derived from FrameLoadRequest, but I can see how that derivation suffers.

What if we were to make the low-level platform thing use names like:

  URLRequest   <-- what is today ResourceRequest
  URLLoader    <-- what is today ResourceHandle
  URLResponse  <-- what is today ResourceResponse

Then, we could move the definition of Resource{Request,Response} to loader/ and we could continue regarding it the way we do today at the loader/ level.

I think since the current ResourceRequest is supposed to be translatable to NSURLRequest and CFNetwork seems to like the URL prefix, that perhaps we should use it too.  In Chromium land, we name the thing backing a ResourceRequest, WebURLRequest and so on.  So, this URL prefix seems to already be in people's minds when they think about the underlying network stack.

By the way, I'm pretty sure that any multi-process embedding of WebKit would share Chromium's desire to add more context to ResourceRequest.  (I can go into much greater detail if it would be helpful.)  We are happy to hide those details from WebCore by leveraging FrameLoaderClient::dispatchWillSendRequest.  All we require from WebCore is the ability to have WebCore classify the type of resource request being made.  That's what led us to this point.  It was suggested that such classification does not belong at the network stack request level, which totally makes sense, but it can be passed to dispatchWillSendRequest in some form.

[Darin Adler]

(In reply to comment #8)
> What if we were to make the low-level platform thing use names like:
> 
>   URLRequest   <-- what is today ResourceRequest
>   URLLoader    <-- what is today ResourceHandle
>   URLResponse  <-- what is today ResourceResponse

I applaud this direction, but I am not overjoyed with the names. It might be worth a little more thinking about plain language names for what those objects are to look for good words. I tend to think that the word “load” might need to be added.

My passion for words rather than abbreviations or acronyms is why the ResourceXXX classes have the names they do rather than names with URL in them. The word “resource” came from the “R” in URL and I figured a word was nicer than an acronym, even one as well loved as URL. I didn’t think that “universal” or “locator” were needed.

Here are some of my attempts to describe the objects in plain language: These are abstract requests that combine a URL with the other arguments that a loader needs to perform a load. These lowest level requests include everything needed at the networking layer to do the networking, and not semantic or contextual arguments needed at a higher level.

> Then, we could move the definition of Resource{Request,Response} to loader/ and we could continue regarding it the way we do today at the loader/ level.

If we free up the names in that lower level it’s fine with me to use the name ResourceRequest in the loader for something that’s a bit more than the current ResourceRequest. I do worry a bit about many different flavors of request, each with more data, so we should consider how we can keep this as simple as possible.

> I think since the current ResourceRequest is supposed to be translatable to NSURLRequest and CFNetwork seems to like the URL prefix

I agree that it’s the same name. I am not in love with the URL prefix here either.

Ken Kocienda and I named these classes WebRequest and WebResponse in the early days of Safari development, but when they were moved into the Foundation framework they were given the prefix NSURL instead.

> By the way, I'm pretty sure that any multi-process embedding of WebKit would share Chromium's desire to add more context to ResourceRequest.

Agreed, although I don’t think the key factor here is “multi-process”.

I think that longer term we may need some additional arguments even for the code in platform/network. For example, the low level networking may need information about the security context of the top level frame for all loads done in any frame on a webpage.

One thing I don’t understand is the layering. I’d like to see this new higher level loading mechanism still be in the platform directory rather than being built on top of things such as Page and Frame. I don’t think that’s incompatible with the notion of communicating across processes. If the context for a load actually uses an object such as a Page or Frame, then we don’t have any separation.

In many cases we’ve managed to create separation within in the platform layer rather than hoisting things out of it. For example, we created the HostWindow class in the platform layer and derived the Chrome class from that. I’d like to see that approach with networking rather than giving up on separating the platform layer.

(In reply to comment #9)
> One thing I don’t understand is the layering. I’d like to see this new higher level loading mechanism still be in the platform directory rather than being built on top of things such as Page and Frame. I don’t think that’s incompatible with the notion of communicating across processes. If the context for a load actually uses an object such as a Page or Frame, then we don’t have any separation.

Some time ago, I've added a ExtraData interface to ResourceRequestBase and a member variable of that type with getters and setters. Currently, the chromium layer implements a class of this interface that keeps all the additional information it associates with a resource request.

What about implementing this interface in loader/ with a class that stores e.g. the request type, say LoaderExtraData. Whenever something in WebCore creates a ResourceRequest, it would also create a LoaderExtraData object, store the request type in that object (and in the future possibly other information), and link it to that resource request.

Since chromium will also want to store some information in this ExtraData, we could use a factory to create the LoaderExtraData object, and chromium would provide its own factory creating an chromium specific object derived form LoaderExtraData.

wdyt?

[Darin Adler]

(In reply to comment #10)
> Some time ago, I've added a ExtraData interface to ResourceRequestBase and a member variable of that type with getters and setters. Currently, the chromium layer implements a class of this interface that keeps all the additional information it associates with a resource request.
> 
> What about implementing this interface in loader/ with a class that stores e.g. the request type, say LoaderExtraData. Whenever something in WebCore creates a ResourceRequest, it would also create a LoaderExtraData object, store the request type in that object (and in the future possibly other information), and link it to that resource request.
> 
> Since chromium will also want to store some information in this ExtraData, we could use a factory to create the LoaderExtraData object, and chromium would provide its own factory creating an chromium specific object derived form LoaderExtraData.
> 
> wdyt?

I need more time to think about this, but at first blush this does not seem like the right architecture to me. Offering a “store some extra data in this object” interface doesn’t seem like the best service to make the higher level loader abstraction work.

(In reply to comment #11)
> (In reply to comment #10)
> > Some time ago, I've added a ExtraData interface to ResourceRequestBase and a member variable of that type with getters and setters. Currently, the chromium layer implements a class of this interface that keeps all the additional information it associates with a resource request.
> > 
> > What about implementing this interface in loader/ with a class that stores e.g. the request type, say LoaderExtraData. Whenever something in WebCore creates a ResourceRequest, it would also create a LoaderExtraData object, store the request type in that object (and in the future possibly other information), and link it to that resource request.
> > 
> > Since chromium will also want to store some information in this ExtraData, we could use a factory to create the LoaderExtraData object, and chromium would provide its own factory creating an chromium specific object derived form LoaderExtraData.
> > 
> > wdyt?
> 
> I need more time to think about this, but at first blush this does not seem like the right architecture to me. Offering a “store some extra data in this object” interface doesn’t seem like the best service to make the higher level loader abstraction work.

You're right, the naming could be better. When I introduced ResourceRequest::ExtraData, it wasn't used anywhere in WebCore, so I gave it a rather generic name.

If we actually use it in loader, we could name the interface ResourceRequest::Context (or move it out of ResourceRequest and call it ResourceRequestContext). I can't think of a good name for the implementation of this interface in loader/ right now..

Created attachment 111699 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Early Warning System Bot]

Comment on attachment 111699 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 111699 [details]
Patch

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

Created attachment 111707 [details]
Patch

I've implemented my proposed change. It moves the TargetType out of platform/network and thereby removes the layering violation. wdyt?

As a follow-up we can think about a more generic approach to remodel this

[Early Warning System Bot]

Comment on attachment 111707 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 111707 [details]
Patch

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

[Gustavo Noronha (kov)]

Comment on attachment 111707 [details]
Patch

Attachment 111707 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10178311

Created attachment 111790 [details]
Patch

[Darin Adler]

Comment on attachment 111790 [details]
Patch

I’m sorry I didn’t explain myself well enough. Allowing a platform to pass through opaque data is a hack around not having an abstraction for loading that allows accomplishing the same thing. I’m sorry I haven’t been able to communicate specifics of a better design, but I am convinced this is the wrong direction.

(In reply to comment #23)
> (From update of attachment 111790 [details])
> I’m sorry I didn’t explain myself well enough. Allowing a platform to pass through opaque data is a hack around not having an abstraction for loading that allows accomplishing the same thing. I’m sorry I haven’t been able to communicate specifics of a better design, but I am convinced this is the wrong direction.

Ok, thank you for clarifying this.

Was a better design creating a wrapper, similar to FrameLoadRequest, around a ResourceRequest that is used everywhere outside of platform/network in place of a ResourceRequest?

[Darin Fisher (:fishd, Google)]

(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 111790 [details] [details])
> > I’m sorry I didn’t explain myself well enough. Allowing a platform to pass through opaque data is a hack around not having an abstraction for loading that allows accomplishing the same thing. I’m sorry I haven’t been able to communicate specifics of a better design, but I am convinced this is the wrong direction.
> 
> Ok, thank you for clarifying this.
> 
> Was a better design creating a wrapper, similar to FrameLoadRequest, around a ResourceRequest that is used everywhere outside of platform/network in place of a ResourceRequest?

I think we should either do that or simply pass the TargetType parameter through to FrameLoaderClient::dispatchWillSendRequest since that's minimally what we need.  The ResourceRequest wrapper idea was born from trying to more cleanly pass TargetType information through to dispatchWillSendRequest.  Maybe it is not worth it?

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > (From update of attachment 111790 [details] [details] [details])
> > > I’m sorry I didn’t explain myself well enough. Allowing a platform to pass through opaque data is a hack around not having an abstraction for loading that allows accomplishing the same thing. I’m sorry I haven’t been able to communicate specifics of a better design, but I am convinced this is the wrong direction.
> > 
> > Ok, thank you for clarifying this.
> > 
> > Was a better design creating a wrapper, similar to FrameLoadRequest, around a ResourceRequest that is used everywhere outside of platform/network in place of a ResourceRequest?
> 
> I think we should either do that or simply pass the TargetType parameter through to FrameLoaderClient::dispatchWillSendRequest since that's minimally what we need.  The ResourceRequest wrapper idea was born from trying to more cleanly pass TargetType information through to dispatchWillSendRequest.  Maybe it is not worth it?

What if we want to pass furthr information in the future, like the DOM element the request is being made for?

[Lyon Chen]

Any comments to https://bugs.webkit.org/show_bug.cgi?id=80519#c24 ?