Get rid of ResourceRequestBase::m_targetType

• Component: Page Loading
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

Keeping the knowledge of Web platform concepts (main frame? subframe?) in ResourceRequest is a layering violation. The only valid use for target type that I know is centralized request prioritization on platforms that have a separate process for network loading.

I thought that the initial step could be to move it behind #if PLATFORM(CHROMIUM). But Joe explained that Blackberry is also using a separate process.

My biggest concern is with attempts to use ResourceRequest::m_targetType for anything but passing priority to network loading back-end (as we've seen in bug 27165). Keeping it as a cross-platform data member will undoubtedly keep causing confusion.

I can think of two things to do to improve the situation.

1. Make the priority values passed to loader process Web-agnostic to address layering violations. Arguments can be made either way, but to me, it seems more of a WebCore job to set relative priorities for various request types. Otherwise, we'd need to update loaders (separate projects living in different repositories) simultaneously once a new target type appears in the Web platform.

2. Pass the priority outside of ResourceRequest (as a separate argument). Or alternatively, just make sure to reset it in Mac WebKit code after re-creating ResourceRequest from NSResourceRequest - it's not like a delegate call will change top frame resource into a subframe resource!

[Adam Barth]

This is related to the larger question of how to interface with network stacks that provide more functionality than CFNetwork.

We can look at what all Chrome uses this bit of data for.  I suspect it uses it in deciding whether something should be treated as a download.

Also, the number of processses isn't really relevant here.  With respect to prioritization, the issue arises because some network transports (e.g., SPDY) have different prioritization tradeoffs.  WebCore can't do the optimal prioritization because it doesn't know what transport is being used.

[Alexey Proskuryakov]

I've been previously told that WebCore can't do the optimal prioritization because it doesn't know what other WebCores are doing at the moment.

I'm not convinced that WebCore can realistically be network back-end agnostic. Surely Web Inspector needs to know if it's SPDY or HTTP. In addition, this prioritization logic should ideally be shared between WebKit ports, so that any port that had a SPDY capable back-end could use it.

[Adam Barth]

> I've been previously told that WebCore can't do the optimal prioritization because it doesn't know what other WebCores are doing at the moment.

That's also true.

> I'm not convinced that WebCore can realistically be network back-end agnostic. Surely Web Inspector needs to know if it's SPDY or HTTP.

Why does the web inspector need to know that?  Does the web inspector need to know whether we're using Ethernet or CDMA?

> In addition, this prioritization logic should ideally be shared between WebKit ports, so that any port that had a SPDY capable back-end could use it.

I'm not quite sure I follow.  Prioritization is part of the SPDY protocol.  Another implementation of SPDY would need to handle that as well.

[Alexey Proskuryakov]

> Why does the web inspector need to know that? 

The Web Inspector displays HTTP request and response headers for resources. Being a binary protocol, SPDY doesn't have these, right?

Web developers will want to know what protocol was used and how, so synthesizing fake HTTP request and response would be against the purpose of Web Inspector.

> Prioritization is part of the SPDY protocol.

Are you saying that SPDY prioritization is specified in terms of top frames, subframes, scripts and such? The Web part of prioritization that deals with those should obviously (to me) be handled by WebCore.

[Alexey Proskuryakov]

The reason why developers want to see details of HTTP and other protocols is of course that they usually have some control over what servers send, and they always have control over their own XMLHttpRequests. This is different from Ethernet or CDMA details in common case.

[Adam Barth]

(In reply to comment #4)
> > Why does the web inspector need to know that? 
> 
> The Web Inspector displays HTTP request and response headers for resources. Being a binary protocol, SPDY doesn't have these, right?

SPDY is a layer below HTTP that makes HTTP go faster by multiplexing multiple HTTP request/response pairs over a single socket.

> Web developers will want to know what protocol was used and how, so synthesizing fake HTTP request and response would be against the purpose of Web Inspector.

There's nothing fake about the HTTP request / response pairs.

> > Prioritization is part of the SPDY protocol.
> 
> Are you saying that SPDY prioritization is specified in terms of top frames, subframes, scripts and such? The Web part of prioritization that deals with those should obviously (to me) be handled by WebCore.

I'm saying that the mapping of those HTML concepts to priorities differs in different scenarios.  Certainly holding request in WebCore is counter-productive in cases were the networking library can make better prioritization decisions than WebCore.

In some sense, you can compute a more optimal prioritization of requests when you have as much information available as possible.  In some cases, some of that information is transport-specific and so only exists at the lower layers of the network stack.

[Alexey Proskuryakov]

> SPDY is a layer below HTTP

OK. Clearly, I've been misunderstanding it.

> I'm saying that the mapping of those HTML concepts to priorities differs in different scenarios.  Certainly holding request in WebCore is counter-productive in cases were the networking library can make better prioritization decisions than WebCore.

I wasn't saying that we should be holding the request in WebCore for prioritization. But the networking back-end shouldn't speak in terms of frames and scripts, that's way too high level for it. It's WebCore that knows that stylesheets should be loaded ahead of images, it's WebCore that knows which page is frontmost, and so on.

[Adam Barth]

Here's some more information about SPDY, if you're interested:

http://www.chromium.org/spdy/spdy-whitepaper

In particular, the diagram in the section titled "SPDY design and features" shows the layering.

[Alexey Proskuryakov]

Another developer got caught in the targetType trap in bug 49838.

[Darin Fisher (:fishd, Google)]

Originally, TargetType was added to chromium/ResourceRequest.h (not ResourceRequestBase.h) with the intention of it being Chromium specific.

Chromium uses TargetType information for several purposes:

1- To prioritize network requests.

2- To determine requests that could be subject to being treated as a download.

3- To implement an optional security filter, where by images loaded with a bad certificate are replaced with a placeholder image.  This feature is not enabled by default.

4- To identify prefetch requests, which are loaded with special policy.

5- To identify cases where an external protocol handler may be loaded via ShellExecute (i.e., only if the target type is a frame).

6- As part of identifying main frame requests that correspond to a cross-process tab navigation.

7- Probably for some other things that I didn't turn up with a quick grep.

As you can see TargetType is more than just a priority field.

I believe that fixing bug 49113 would make it so that we no longer need this field for PLATFORM(CHROMIUM).  The additional meta data required by Chromium could be captured by Chromium within the willSendRequest callback and it could tag that information on the ResourceRequest for its use.

While I agree the above does not address the "laying violation" concerns, it at least isolates the issue, which is an improvement.

A good first step may be to move TargetType back to chromium/ResourceRequest.h, and copy it to the Blackberry's version of ResourceRequest.h as well.  That will help avoid extra uses of this field from cropping up.

Maybe this is for a different discussion, but long-term, I believe we need a layer between ResourceLoader and ResourceHandle that abstracts the concept of "network stack", providing the right layer for indirection.  This is the layer where appcache, blobs, and other concepts that CFNetwork doesn't understand would live.

[Alexey Proskuryakov]

> A good first step may be to move TargetType back to chromium/ResourceRequest.h, and copy
> it to the Blackberry's version of ResourceRequest.h as well.  That will help avoid extra uses of
> this field from cropping up.

Sounds good. Can someone from Chromium land do that? It would probably be less build breakage that way.

[Darin Fisher (:fishd, Google)]

One way to solve this problem is to add a parameter to FrameLoaderClient::willSendRequest that is like the current TargetType enumeration.  (Maybe it needs a better name.)

Chromium needs a way to know the intended purpose of a ResourceRequest that WebCore is making.  This is for all the reasons listed in comment #10.  It seems like passing that information up to the FrameLoaderClient is the correct way to address a use case like this.

Chromium can then store that information for later use.  This would allow us to remove ResourceRequest{Base}::TargetType completely.

(In reply to comment #12)
> One way to solve this problem is to add a parameter to FrameLoaderClient::willSendRequest that is like the current TargetType enumeration.  (Maybe it needs a better name.)
> 
> Chromium needs a way to know the intended purpose of a ResourceRequest that WebCore is making.  This is for all the reasons listed in comment #10.  It seems like passing that information up to the FrameLoaderClient is the correct way to address a use case like this.
> 
> Chromium can then store that information for later use.  This would allow us to remove ResourceRequest{Base}::TargetType completely.

How would you get that information from the point of where the ResourceRequest is created to FrameLoaderClient::willSendRequest. Add it to the options used to create the loader?

[Darin Fisher (:fishd, Google)]

(In reply to comment #13)
> How would you get that information from the point of where the ResourceRequest is created to FrameLoaderClient::willSendRequest. Add it to the options used to create the loader?

I'm not sure.  Maybe we would need to create a wrapper for ResourceRequest that WebCore can use whenever it constructs a ResourceRequest.  That would probably be the most robust way to plumb this (as opposed to adding another parameter to a zillion functions).

We have FrameLoadRequest, which I'm sure everyone hates, and my proposal would be to add another thing like that, but what else can you do?  If ResourceRequest is supposed to be low-level, then WebCore needs some higher-level wrapper to construct whenever it wants to make a ResourceRequest.  Maybe call it LoadRequest to be ultra-confusing but at least somewhat related to FrameLoadRequest?

(In reply to comment #14)
> (In reply to comment #13)
> > How would you get that information from the point of where the ResourceRequest is created to FrameLoaderClient::willSendRequest. Add it to the options used to create the loader?
> 
> I'm not sure.  Maybe we would need to create a wrapper for ResourceRequest that WebCore can use whenever it constructs a ResourceRequest.  That would probably be the most robust way to plumb this (as opposed to adding another parameter to a zillion functions).
> 
> We have FrameLoadRequest, which I'm sure everyone hates, and my proposal would be to add another thing like that, but what else can you do?  If ResourceRequest is supposed to be low-level, then WebCore needs some higher-level wrapper to construct whenever it wants to make a ResourceRequest.  Maybe call it LoadRequest to be ultra-confusing but at least somewhat related to FrameLoadRequest?

ok, I'll try to come up with something along these lines

[Alexey Proskuryakov]

I think that what Darin suggested in comment 10 is still a practical way forward.

[Darin Fisher (:fishd, Google)]

(In reply to comment #16)
> I think that what Darin suggested in comment 10 is still a practical way forward.

Really?  We need to add a call from XMLHttpRequest.cpp to set the new target type for XHR.  If we don't have the enumeration on ResourceRequestBase, then it means that XMLHttpRequest.cpp will need to grow a #if PLATFORM(CHROMIUM) around the code that sets the XHR target type.

Is that OK by you while we sort out this refactoring?

[Alexey Proskuryakov]

That sounds fine to me. My immediate concern and the reason for filing this bug is about cross platform code (like Web Inspector) becoming dependent on m_targetType.

Created attachment 102916 [details]
Patch

Created attachment 102917 [details]
Patch

I introduced HAVE(RESOURCEREQUEST_TARGETTYPE) instead of using PLATFORM(chromium) so blackberry can easily adopt the change

[Alexey Proskuryakov]

Comment on attachment 102917 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102917&action=review

The essence of this patch seems fine to me. One nit that needs to be addressed: this is not a correct use of HAVE - it's supposed to tell you what an underlying platform has.

/* HAVE() - specific system features (headers, functions or similar) that are present or not */

I don't think that there is a macro in Platform.h that can be used for this difference. I'd just use PLATFORM(CHROMIUM) and let platforms that don't contribute back to webkit.org deal with it. In a way, PLATFORM sends the correct message - even for Chromium, it's a layering violation that needs to be addressed some day.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

This line will prevent the patch from being landed.

Created attachment 102947 [details]
Patch

(In reply to comment #22)
> (From update of attachment 102917 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102917&action=review
> 
> The essence of this patch seems fine to me. One nit that needs to be addressed: this is not a correct use of HAVE - it's supposed to tell you what an underlying platform has.
> 
> /* HAVE() - specific system features (headers, functions or similar) that are present or not */
> 
> I don't think that there is a macro in Platform.h that can be used for this difference. I'd just use PLATFORM(CHROMIUM) and let platforms that don't contribute back to webkit.org deal with it. In a way, PLATFORM sends the correct message - even for Chromium, it's a layering violation that needs to be addressed some day.

OTOH it makes it harder for them to upstream their changes. Anyway, I hope we fix this layering violation rather sooner than later, so I'm fine with PLATFORM(CHROMIUM)

> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests. (OOPS!)
> 
> This line will prevent the patch from being landed.

done

[Adam Barth]

Thanks jochen!!

[WebKit Review Bot]

Comment on attachment 102947 [details]
Patch

Clearing flags on attachment: 102947

Committed r92399: <http://trac.webkit.org/changeset/92399>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.