Bug 80713 – Add new methods to FrameLoaderClient so different WebKit ports can customize the ResourceRequest when it is created.

• Component: Page Loading
• Status: RESOLVED INVALID

[Lyon Chen]

Right now there are many port-specific stuff in ResourceRequest, but they are not always correctly setup at the right time, like the ResourceRequest::TargetType for BlackBerry and Chromium platform. There are some #ifdef platform stuff in PingLoader/CachedResource/Worker/WorkerScriptLoader/XMLHttpRequest, adding the following callback in FrameLoaderClient and calling them at the right time can move these #ifdef into platform specific codes.


        virtual void addExtraFieldsToSubresourceRequest(ResourceRequest& request, FrameLoadType loadType, CachedResource::Type resourceType) { };
        virtual void addExtraFieldsToMainResourceRequest(ResourceRequest& request, FrameLoadType type) { };

[Lyon Chen]

Created attachment 131066 [details]
Suggested patch for 80713

[Lyon Chen]

Add Darin Adler and Darin Fisher to CC list.

darin & fishd: Sorry for adding both of you into this bug, do you have any comments to my patch?

[Lyon Chen]

Forgot to add jochen.

jochen: any comments?

how would you in this model annotate e.g. the request fired for XMLHttpRequests?

[Build Bot]

Comment on attachment 131066 [details]
Suggested patch for 80713

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

[Lyon Chen]

(In reply to comment #4)
> how would you in this model annotate e.g. the request fired for XMLHttpRequests?

When FrameLoaderClient::addExtraFieldsToSubresourceRequest() is called, do what you are doing now in willSendRequest().

This is like willSendRequest(), just it is called before any policy checking.

[WebKit Review Bot]

Comment on attachment 131066 [details]
Suggested patch for 80713

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

[Early Warning System Bot]

Comment on attachment 131066 [details]
Suggested patch for 80713

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

(In reply to comment #6)
> (In reply to comment #4)
> > how would you in this model annotate e.g. the request fired for XMLHttpRequests?
> 
> When FrameLoaderClient::addExtraFieldsToSubresourceRequest() is called, do what you are doing now in willSendRequest().

That is not invoked by XMLHttpRequest.cpp, so the information that this is an XHR is lost.

> 
> This is like willSendRequest(), just it is called before any policy checking.


that currently relies on the TargetType being set. I thought the point of this is getting rid of the TargetType?

[Early Warning System Bot]

Comment on attachment 131066 [details]
Suggested patch for 80713

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

[Lyon Chen]

(In reply to comment #9)
> 
> That is not invoked by XMLHttpRequest.cpp, so the information that this is an XHR is lost.

For loads not going through FrameLoader, like XMLHttpRequest, I will have to make call to addExtraFieldsToSubresourceRequest(), or a new addExtraFieldsToResourceRequest(). Thanks for pointing this out.

> that currently relies on the TargetType being set. I thought the point of this is getting rid of the TargetType?

The patch here is not exactly to get rid of TargetType, it is just to get rid of TargetType, and any other platform specific stuff, from loader (and other cross platform codes). And make it easier for us to add more platform specific to ResourceRequest without changing cross-platform stuff.

[Gyuyoung Kim]

Comment on attachment 131066 [details]
Suggested patch for 80713

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

[Lyon Chen]

(In reply to comment #9)

> That is not invoked by XMLHttpRequest.cpp, so the information that this is an XHR is lost.
> 

Are you sure? Just checked my callstack, it is called for XMLHttpRequest in SubresourceLoader::create().

[Alexey Proskuryakov]

Customizing headers is really what dispatchWillSendRequest is for.

[Gustavo Noronha (kov)]

Comment on attachment 131066 [details]
Suggested patch for 80713

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

[Lyon Chen]

(In reply to comment #14)
> Customizing headers is really what dispatchWillSendRequest is for.

The problem for dispatchWillSendRequest() is:

* It is called after dispatchDecidePolicyForXXX(), during which some aspects, like TargetType of ResourceRequest, could be checked, so it is too late to set it in dispatchWillSendRequest().

* In dispatchWillSendRequest(), some info, like resource type, is not available, make it harder to do some customizing.

[Lyon Chen]

Created attachment 131346 [details]
Update patch so it compiles.

[Early Warning System Bot]

Comment on attachment 131346 [details]
Update patch so it compiles.

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

[Early Warning System Bot]

Comment on attachment 131346 [details]
Update patch so it compiles.

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

[Gustavo Noronha (kov)]

Comment on attachment 131346 [details]
Update patch so it compiles.

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

[Gyuyoung Kim]

Comment on attachment 131346 [details]
Update patch so it compiles.

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

[Lyon Chen]

Created attachment 131358 [details]
Patch fixed GStreamer compile error.

[Alexey Proskuryakov]

It still feels entirely wrong to me to add two new callbacks when there is already one designed largely for the same purpose.

[Build Bot]

Comment on attachment 131358 [details]
Patch fixed GStreamer compile error.

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

[Lyon Chen]

(In reply to comment #23)
> It still feels entirely wrong to me to add two new callbacks when there is already one designed largely for the same purpose.

Hi, Alexey: Yeah, I agree with that. But considering the timing issue, would removing willSendRequest() make things more logical and more acceptable? Or change willSendRequest(const ResourceRequest&) so it becomes just a notification?

[Joe Mason]

(In reply to comment #23)
> It still feels entirely wrong to me to add two new callbacks when there is already one designed largely for the same purpose.

Well, it seems to me that if the platform is allowed to make customizations to the request, it should be allowed to do it before dispatchDecidePolicyForXXX.  Moving willSendRequest before that doesn't seem right, since until dispatchDecidePolicyForXXX returns PolicyUse, it's not guaranteed the request will be sent.

And the name of "willSendRequest" doesn't tell me that it's intended for altering the request - it tells me that it's intended to be a notification that a request is about to be sent.  The fact that it has a non-const parameter which can be altered isn't clear from the name.

It seems reasonable to me to have two entry points to customize the request, one before dispatchDecidePolicy and one only for requests that will actually be sent.  For instance, the platform might want to do one set of customizations that applies both to PolicyUse and PolicyDownload, and another that applies only to PolicyUse.

I don't like the name of the two new methods, though.  First, they can be used to update existing fields as well as add new fields, so "addExtraFields" doesn't seem right.  Second, I think we should have just one new method, with an enum parameter saying whether it's a main resource of subresource request.  I suggest calling it "customizeRequest", or to be more explicit about timing, "customizeRequestBeforePolicyCheck".

[Alexey Proskuryakov]

> Well, it seems to me that if the platform is allowed to make customizations to the request, it should be allowed to do it before dispatchDecidePolicyForXXX. 

Could you please elaborate on why you think so? Surely client code doesn't need to have the modifications it can make itself already applied to request in order to make the decision in dispatchDecidePolicyForXXX.

[Lyon Chen]

(In reply to comment #26)
> I don't like the name of the two new methods, though.  First, they can be used to update existing fields as well as add new fields, so "addExtraFields" doesn't seem right.  Second, I think we should have just one new method, with an enum parameter saying whether it's a main resource of subresource request.  I suggest calling it "customizeRequest", or to be more explicit about timing, "customizeRequestBeforePolicyCheck".

Yeah, the method name is not well chosen and I am more than willing to change them to more proper name.

But I think the resource type for subresource load is needed to better understand what this request for, and for better customization.

[Lyon Chen]

(In reply to comment #27)
> > Well, it seems to me that if the platform is allowed to make customizations to the request, it should be allowed to do it before dispatchDecidePolicyForXXX. 
> 
> Could you please elaborate on why you think so? Surely client code doesn't need to have the modifications it can make itself already applied to request in order to make the decision in dispatchDecidePolicyForXXX.

The point is, it could check the info, like TargetType, to do the decision whether to allow the loading to go ahead or not, or handle specially. And this target type info is related to the sub-resource type, and/or the load type. The sub-resource type is not available in dispatchDecidePolicyForXXX().

And these 2 new methods, along with its 2 parameters, provide a way for client to remember these info.

Of course, if we can add TargetType, along with other things, to cross-platform codes, then these are not necessary.

[Lyon Chen]

(In reply to comment #29)
> Of course, if we can add TargetType, along with other things, to cross-platform codes, then these are not necessary.

I might not express myself clear, what I want to say is, if we can save the resource type and load type into the cross-platform ResourceRequest, then we don't need any new methods for BlackBerry port.

And these 2 new methods are in essential doing the saving of these 2 info.

[Alexey Proskuryakov]

Do you have a concrete example where resource type is needed in dispatchDecidePolicyForXXX? For example, for navigations, it's immediately clear that it's a main resource.

[Lyon Chen]

(In reply to comment #31)
> Do you have a concrete example where resource type is needed in dispatchDecidePolicyForXXX? For example, for navigations, it's immediately clear that it's a main resource.

Actually for main resource there is no resource type. Resource type info is for subresources. In our PlayBook API, we allow client of WebKit to filter our network request, and the TargetType of the resource request is used by client when deciding what to do with these requests. Like it can only handle some specific content (like image) as there are encrypted and let other loads go ahead as normal. So for us keeping the TargetType info accurate is required. And the TargetType info is hard to be accurate without the resource type info.

These 2 methods are needed to save the resource type info.

[Lyon Chen]

(In reply to comment #31)
> Do you have a concrete example where resource type is needed in dispatchDecidePolicyForXXX? For example, for navigations, it's immediately clear that it's a main resource.

Hmmm, checked the code again, so all these 3 dispatchDecidePolicyForXXX (Navigation, NewWindow, Response) are for main loads. So for them, no resource type is needed. Sorry for my misunderstanding and misleading argument.

But we will still need resource type for sub-resource loads to accurately decide the customization, like TargetType.

(In reply to comment #33)
> But we will still need resource type for sub-resource loads to accurately decide the customization, like TargetType.

As embedders that depend on this information will require this for lots of different locations (at least the ones using chromium's TargetType)

I think instead of adding new callbacks, all loader methods that take a ResourceRequest or a FrameLoadRequest should also take a TargetType parameter, until they hit willSendRequest (TargetType could then be moved out of platform to loader and we could also drop the #ifdef PLATFORM(CHROMIUM)'s)

[Lyon Chen]

(In reply to comment #34)
> I think instead of adding new callbacks, all loader methods that take a ResourceRequest or a FrameLoadRequest should also take a TargetType parameter, until they hit willSendRequest (TargetType could then be moved out of platform to loader and we could also drop the #ifdef PLATFORM(CHROMIUM)'s)

Then for most sub-resource loads that are scheduled (and delayed), this info need to be saved in the ResourceLoadScheduler too. I believe an callback at the right time is simpler, and enabled different ports to do some other customization at this time.

[Lyon Chen]

Created attachment 131617 [details]
Patch based on comments and fix Mac's build error.

I can create a separate patch to make dispatchWillSendRequest() just a notification, and move the existing codes to the newly added methods for different platforms.

[Gyuyoung Kim]

Comment on attachment 131617 [details]
Patch based on comments and fix Mac's build error.

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

[Lyon Chen]

Created attachment 131621 [details]
Add GStreamer fix back.

[Alexey Proskuryakov]

> I can create a separate patch to make dispatchWillSendRequest() just a notification, and move the existing codes to the newly added methods for different platforms.

I'm not sure what you mean by this. The various willSendRequest methods are part of WebKit API.

[Lyon Chen]

(In reply to comment #39)
> I'm not sure what you mean by this. The various willSendRequest methods are part of WebKit API.

I mean if the new methods I suggested is approved, then willSendRequest() is redundant and should be changed to use a const ResourceRequest, which means it is just a notification callback, not for ResourceRequest customization anymore.

(In reply to comment #40)
> (In reply to comment #39)
> > I'm not sure what you mean by this. The various willSendRequest methods are part of WebKit API.
> 
> I mean if the new methods I suggested is approved, then willSendRequest() is redundant and should be changed to use a const ResourceRequest, which means it is just a notification callback, not for ResourceRequest customization anymore.

The new methods are not sufficient for the chromium port.

Anyway, we need to cache the target type (or equivalently the cached resource types) already now in a few locations. I don't see that as a problem.

[Lyon Chen]

(In reply to comment #41)
> 
> The new methods are not sufficient for the chromium port.

Do you mind elaborate what extra is needed for Chromium?

> Anyway, we need to cache the target type (or equivalently the cached resource types) already now in a few locations. I don't see that as a problem.

Do you mean these setTargetType() calls inside these platform #ifdef?

(In reply to comment #42)
> (In reply to comment #41)
> > 
> > The new methods are not sufficient for the chromium port.
> 
> Do you mind elaborate what extra is needed for Chromium?

It's not clear to me how the ResourceRequest created by XMLHttpRequest will be annotated with the XHR target type using your proposed set of callbacks.

> 
> > Anyway, we need to cache the target type (or equivalently the cached resource types) already now in a few locations. I don't see that as a problem.
> 
> Do you mean these setTargetType() calls inside these platform #ifdef?

In comment #35 you said that in order to plumb the target type everywhere, we'd need to cache it in ResourceLoadScheduler. I was referring to that comment: I don't think that's a big issue, as we do that already e.g. in CachedResource.cpp (i.e. the information what target type to use is kept in the cache so we can attach it to the resource request when we need it)

[Lyon Chen]

(In reply to comment #43)
> It's not clear to me how the ResourceRequest created by XMLHttpRequest will be annotated with the XHR target type using your proposed set of callbacks.

Yeah that is a problem. Do you think it make sense to call frame()->loader()->client()->customizeSubresourceRequest() in XMLHttpRequest::createRequest()?

> In comment #35 you said that in order to plumb the target type everywhere, we'd need to cache it in ResourceLoadScheduler. I was referring to that comment: I don't think that's a big issue, as we do that already e.g. in CachedResource.cpp (i.e. the information what target type to use is kept in the cache so we can attach it to the resource request when we need it)

So you mean it is fine for us to leave these #ifdef there? And add more when it is needed?

I actually don't mind this approach that much, just thought removing these platform #ifdef and doing all these customization in BlackBerry platform will be cleaner.

(In reply to comment #44)
> (In reply to comment #43)
> > It's not clear to me how the ResourceRequest created by XMLHttpRequest will be annotated with the XHR target type using your proposed set of callbacks.
> 
> Yeah that is a problem. Do you think it make sense to call frame()->loader()->client()->customizeSubresourceRequest() in XMLHttpRequest::createRequest()?

That's not possible, as the XHR might be created on the worker thread, so we have to jump to the main thread first before we can invoke methods on the FrameLoaderClient

> 
> > In comment #35 you said that in order to plumb the target type everywhere, we'd need to cache it in ResourceLoadScheduler. I was referring to that comment: I don't think that's a big issue, as we do that already e.g. in CachedResource.cpp (i.e. the information what target type to use is kept in the cache so we can attach it to the resource request when we need it)
> 
> So you mean it is fine for us to leave these #ifdef there? And add more when it is needed?

No, I want to get rid of the ifdefs

target type should not be part of resource request

Instead, all loader related methods that take a resource request should also take a target type parameter, e.g. ThreadableLoader::create should take ScriptExecutionContext, ThreadableLoaderClient, ResourceRequest, TargetType, and ThreadableLoaderOptions parameters.

TargetType could either be owned by FrameLoader or some other class in loader/

> 
> I actually don't mind this approach that much, just thought removing these platform #ifdef and doing all these customization in BlackBerry platform will be cleaner.

[Lyon Chen]

(In reply to comment #45)
> 
> No, I want to get rid of the ifdefs
> 
> target type should not be part of resource request
> 
> Instead, all loader related methods that take a resource request should also take a target type parameter, e.g. ThreadableLoader::create should take ScriptExecutionContext, ThreadableLoaderClient, ResourceRequest, TargetType, and ThreadableLoaderOptions parameters.
> 
> TargetType could either be owned by FrameLoader or some other class in loader/
> 

Like your patch for https://bugs.webkit.org/show_bug.cgi?id=65855? It is unfortunately there for about 5 months now without a r+ or r-.

(In reply to comment #46)
> (In reply to comment #45)
> > 
> > No, I want to get rid of the ifdefs
> > 
> > target type should not be part of resource request
> > 
> > Instead, all loader related methods that take a resource request should also take a target type parameter, e.g. ThreadableLoader::create should take ScriptExecutionContext, ThreadableLoaderClient, ResourceRequest, TargetType, and ThreadableLoaderOptions parameters.
> > 
> > TargetType could either be owned by FrameLoader or some other class in loader/
> > 
> 
> Like your patch for https://bugs.webkit.org/show_bug.cgi?id=65855? It is unfortunately there for about 5 months now without a r+ or r-.

I take comment 23 on that bug as a r-

However, the approach I took in that patch was also different from what I described above. It introduces a new class that binds a ResourceRequest and a TargetType together. What I think might be a solution (and I tried to describe above) is not introducing a new class, but just plumbing the two values everywhere we need them.

[Lyon Chen]

(In reply to comment #47)
> 
> I take comment 23 on that bug as a r-
> 
> However, the approach I took in that patch was also different from what I described above. It introduces a new class that binds a ResourceRequest and a TargetType together. What I think might be a solution (and I tried to describe above) is not introducing a new class, but just plumbing the two values everywhere we need them.

Hi, jochen: any progress on this approach? Or any bug to track it?

(In reply to comment #48)
> (In reply to comment #47)
> > 
> > I take comment 23 on that bug as a r-
> > 
> > However, the approach I took in that patch was also different from what I described above. It introduces a new class that binds a ResourceRequest and a TargetType together. What I think might be a solution (and I tried to describe above) is not introducing a new class, but just plumbing the two values everywhere we need them.
> 
> Hi, jochen: any progress on this approach? Or any bug to track it?

No, sorry. Once I have time for it, I'll attach the patch to https://bugs.webkit.org/show_bug.cgi?id=65855

of course, feel free to implement this yourself, or come up with an even better solution :)

[Lyon Chen]

(In reply to comment #50)
> of course, feel free to implement this yourself, or come up with an even better solution :)

For my current patch, do you think it will work if I use 2 thread-safe plain C function, instead of FrameLoaderClient method, so they can be safely called from any threads, including worker threads?

(In reply to comment #51)
> (In reply to comment #50)
> > of course, feel free to implement this yourself, or come up with an even better solution :)
> 
> For my current patch, do you think it will work if I use 2 thread-safe plain C function, instead of FrameLoaderClient method, so they can be safely called from any threads, including worker threads?

I think it's preferable to do all customization of ResourceRequests on the main thread.

[Nate Chapin]

Comment on attachment 131621 [details]
Add GStreamer fix back.

I'm inclined to agree with ap that adding these new callbacks is not the right answer, especially for the subresource case. 

Since we don't do policy checks for subresources, dispatchWillSendRequest() seem like the right time for them. For the policy checks for main resource, perhaps we can approximate the TargetType, especially since there should be only 2 possibilities, then just set the real TargetType in dispatchWillSendRequest() as well?

[Lyon Chen]

(In reply to comment #53)
> Since we don't do policy checks for subresources, dispatchWillSendRequest() seem like the right time for them. For the policy checks for main resource, perhaps we can approximate the TargetType, especially since there should be only 2 possibilities, then just set the real TargetType in dispatchWillSendRequest() as well?

Thanks for closing this bug! Even r- is better than leaving it there without a conclusion.

And yes I have updated our BlackBerry code and only ensure right TargetType in dispatchWillSendRequest(), though I personally still would like making TargetType of ResourceRequest always right.

[Philippe Normand]

Ping? Not sure this bug is still valid.

[Lyon Chen]

(In reply to comment #55)
> Ping? Not sure this bug is still valid.

It is no longer valid.