Bug 80519 – [BlackBerry] Set ResourceRequest TargetType in WebPagePrivate::load()

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Lyon Chen]

Right now the TargetType of ResourceRequest for main load is not set, set them in WebPagePrivate::load().

[Lyon Chen]

Created attachment 130644 [details]
Patch for 80519

[Lyon Chen]

Created attachment 130653 [details]
Updated patch to set correct target type for WebKit internal loads too.

[Nate Chapin]

Comment on attachment 130653 [details]
Updated patch to set correct target type for WebKit internal loads too.

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

> Source/WebCore/loader/FrameLoader.cpp:1223
> +#if PLATFORM(BLACKBERRY)
> +    Frame* loadframe = targetFrame ? targetFrame : frame();
> +    if (loadframe->page()->mainFrame() == loadframe)
> +        request.setTargetType(ResourceRequest::TargetIsMainFrame);
> +    else
> +        request.setTargetType(ResourceRequest::TargetIsSubframe);
> +#endif
> +

Is there any way we can set this somewhere other than FrameLoader, preferably somewhere that doesn't require an #if PLATFORM()? chromium, e.g., does this in its FrameLoaderClientImpl::dispatchWillSendRequest().

[Joe Mason]

(In reply to comment #3)
> Is there any way we can set this somewhere other than FrameLoader, preferably somewhere that doesn't require an #if PLATFORM()? chromium, e.g., does this in its FrameLoaderClientImpl::dispatchWillSendRequest().

We do this too right now.  This patch should remove the existing code from dispatchWillSendRequest as well as adding it to FrameLoader.  (If we decide it's a good idea at all.)

The problem is then we have to be very careful not to check the targetType before it's actually set, such as from dispatchDecidePolicyForResponse.  Which can be done, but arranging things so we don't have to be that vigilant is good defensive programming.

I'm not happy with the ifdef myself - we should make another try to find a better solution.

[Lyon Chen]

(In reply to comment #3)

> Is there any way we can set this somewhere other than FrameLoader, preferably somewhere that doesn't require an #if PLATFORM()? chromium, e.g., does this in its FrameLoaderClientImpl::dispatchWillSendRequest().

The problem is FrameLoaderClientImpl::dispatchWillSendRequest() is too late a place to set target type. FrameLoaderClient::dispatchDecidePolicyForNavigationAction() could check the target type too, and it is called before dispatchWillSendRequest().

We can avoid check target type in dispatchDecidePolicyForNavigationAction(), or any other places before dispatchWillSendRequest() is called, but it will create a implicit situation that you need to know when the target type is set, not just assuming it is always right.

[Lyon Chen]

(In reply to comment #4)
> The problem is then we have to be very careful not to check the targetType before it's actually set, such as from dispatchDecidePolicyForResponse.  Which can be done, but arranging things so we don't have to be that vigilant is good defensive programming.

Yeah, the problem is, if we put these code elsewhere, not the places the ResourceRequest is created, we create the situation we need to know when the target type is set and trustable, not sure it is worth to pay this to avoid the #if PLATFORM stuff.

[Antonio Gomes]

(In reply to comment #5)
> (In reply to comment #3)
> 
> > Is there any way we can set this somewhere other than FrameLoader, preferably somewhere that doesn't require an #if PLATFORM()? chromium, e.g., does this in its FrameLoaderClientImpl::dispatchWillSendRequest().
> 
> The problem is FrameLoaderClientImpl::dispatchWillSendRequest() is too late a place to set target type. FrameLoaderClient::dispatchDecidePolicyForNavigationAction() could check the target type too, and it is called before dispatchWillSendRequest().

can we have asserts to make sure no one is calling it at too late moment?

[Antonio Gomes]

Nate, any suggestion based on the above?

[Lyon Chen]

(In reply to comment #7)

> can we have asserts to make sure no one is calling it at too late moment?

Actually we have assertion in upstream code now. It is just we check the target type in dispatchDecidePolicyForNavigationAction() and other policy methods. We need to re-implement them if we want to avoid the #ifdef thing.

[Nate Chapin]

(In reply to comment #8)
> Nate, any suggestion based on the above?

Both ifdefs and new code in FrameLoader are good to avoid when possible, so I just wanted to make sure other alternatives had been considered. :)

I'm not immediately seeing a good alternative, so I won't fight this as written (though it'd be great if a FIXME were included).

It might be good to give abarth a chance to comment, too.

[Lyon Chen]

(In reply to comment #10)
> It might be good to give abarth a chance to comment, too.

Thanks, pinged abarth on #webkit.

[Adam Barth]

Comment on attachment 130653 [details]
Updated patch to set correct target type for WebKit internal loads too.

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

>> Source/WebCore/loader/FrameLoader.cpp:1223
>> +#if PLATFORM(BLACKBERRY)
>> +    Frame* loadframe = targetFrame ? targetFrame : frame();
>> +    if (loadframe->page()->mainFrame() == loadframe)
>> +        request.setTargetType(ResourceRequest::TargetIsMainFrame);
>> +    else
>> +        request.setTargetType(ResourceRequest::TargetIsSubframe);
>> +#endif
>> +
> 
> Is there any way we can set this somewhere other than FrameLoader, preferably somewhere that doesn't require an #if PLATFORM()? chromium, e.g., does this in its FrameLoaderClientImpl::dispatchWillSendRequest().

We don't want to have PLATFORM-specific ifdefs in FrameLoader.   We have a few of them, but they are mostly sadness.  The Chromium port uses TargetType.  How does it avoid needing this code here?

[Joe Mason]

(In reply to comment #12)
> We don't want to have PLATFORM-specific ifdefs in FrameLoader.   We have a few of them, but they are mostly sadness.  The Chromium port uses TargetType.  How does it avoid needing this code here?

I think the Chromium port has the same design flaw, but happens to have not tried to read TargetType in any methods that are called before dispatchWillSendRequest to it hasn't hit them.

[Adam Barth]

Wouldn't this code then be needed for PLATFORM(CHROMIUM) as well?  I've added jochen to the CC list since he understands TargetType better than I do.

[Lyon Chen]

(In reply to comment #14)
> Wouldn't this code then be needed for PLATFORM(CHROMIUM) as well?  I've added jochen to the CC list since he understands TargetType better than I do.

Yes, I think logically CHROMIUM need this code as well to make the TargetType always correct, but I am not sure how ResourceRequest::TargetType is used in Chromium, so not sure whether Chromium needs it here in real world.

But the way Chromium does it, setting TargetType in FrameLoaderClient::dispatchWillSendRequest(), leaves a gap when the target type has a default value, and in this gap, FrameLoadClient::dispatchDecidePolicyForXXX() is called, if the FrameLoaderClient decides to check TargetType in these methods, there will be a problem.

(In reply to comment #15)
> (In reply to comment #14)
> > Wouldn't this code then be needed for PLATFORM(CHROMIUM) as well?  I've added jochen to the CC list since he understands TargetType better than I do.
> 
> Yes, I think logically CHROMIUM need this code as well to make the TargetType always correct, but I am not sure how ResourceRequest::TargetType is used in Chromium, so not sure whether Chromium needs it here in real world.
> 
> But the way Chromium does it, setting TargetType in FrameLoaderClient::dispatchWillSendRequest(), leaves a gap when the target type has a default value, and in this gap, FrameLoadClient::dispatchDecidePolicyForXXX() is called, if the FrameLoaderClient decides to check TargetType in these methods, there will be a problem.

In the chromium port, the target type is not set correctly in lots of different places. However, I think the correct solution to this problem is to move TargetType out of ResourceRequest, and have all calls that pass a ResourceRequest around in loader/ also take a TargetType parameter.

[Lyon Chen]

(In reply to comment #16)

> In the chromium port, the target type is not set correctly in lots of different places. However, I think the correct solution to this problem is to move TargetType out of ResourceRequest, and have all calls that pass a ResourceRequest around in loader/ also take a TargetType parameter.

The problem in moving TargetType out of ResourceRequest is, where we can save the TargetType info if the load is postponed or re-scheduled?

(In reply to comment #17)
> (In reply to comment #16)
> 
> > In the chromium port, the target type is not set correctly in lots of different places. However, I think the correct solution to this problem is to move TargetType out of ResourceRequest, and have all calls that pass a ResourceRequest around in loader/ also take a TargetType parameter.
> 
> The problem in moving TargetType out of ResourceRequest is, where we can save the TargetType info if the load is postponed or re-scheduled?

the embedder should link the target type to the ResourceRequest in willSendRequest

For chromium, I added ResourceRequest::ExtraData (http://trac.webkit.org/browser/trunk/Source/WebCore/platform/network/chromium/ResourceRequest.h#L63) to carry arbitrary additional information in an ResourceRequest. 

In willSendRequest, the TargetType is put into a class deriving from ExtraData and stashed into the ResourceRequest. Now when the ResourceRequest later is passed to our resource loader, it can retrieve the target type from there.

[Lyon Chen]

(In reply to comment #18)
> the embedder should link the target type to the ResourceRequest in willSendRequest
> 
> For chromium, I added ResourceRequest::ExtraData (http://trac.webkit.org/browser/trunk/Source/WebCore/platform/network/chromium/ResourceRequest.h#L63) to carry arbitrary additional information in an ResourceRequest. 
> 
> In willSendRequest, the TargetType is put into a class deriving from ExtraData and stashed into the ResourceRequest. Now when the ResourceRequest later is passed to our resource loader, it can retrieve the target type from there.

Yeah, the ExtraData class solves the issue of saving TargetType, but still when the TargetType is decided is still an issue, and I feel it is too late to decide in willSendRequest() as it is called after the policy decision stage, at which stage, it seems to me, it is proper to check the TargetType info before deciding the policy for that particular loading.

If the TargetType is still decided in FrameLoader::loadURL(), then whether make it part of ResourceRequest or as an parameter, we still need platform #ifdef.

(In reply to comment #19)
> If the TargetType is still decided in FrameLoader::loadURL(), then whether make it part of ResourceRequest or as an parameter, we still need platform #ifdef.

The reason that it's behind a #ifdef is that TargetType should not be defined in platform/

If we move the definition to loader/, we can just enable this behavior for all ports. In fact, the more ports start to have out-of-process network stacks, the more will need this feature.

[Lyon Chen]

(In reply to comment #20)
> The reason that it's behind a #ifdef is that TargetType should not be defined in platform/
> 
> If we move the definition to loader/, we can just enable this behavior for all ports. In fact, the more ports start to have out-of-process network stacks, the more will need this feature.

Thanks for the info! So the solution can be a common ResourceRequestBase (which will have TargetType info) in loader, and every platform still have their own ResourceRequest (which inherits ResourceRequestBase) for their own platform specific stuff?

(In reply to comment #21)
> (In reply to comment #20)
> > The reason that it's behind a #ifdef is that TargetType should not be defined in platform/
> > 
> > If we move the definition to loader/, we can just enable this behavior for all ports. In fact, the more ports start to have out-of-process network stacks, the more will need this feature.
> 
> Thanks for the info! So the solution can be a common ResourceRequestBase (which will have TargetType info) in loader, and every platform still have their own ResourceRequest (which inherits ResourceRequestBase) for their own platform specific stuff?

I think there are two options:

(1) have a class in loader/ that wraps a ResourceRequest (similar to FrameLoadRequest) - however, failing to come up with a good name for such a class, this hasn't happened yet

(2) don't touch ResourceRequest at all, and just define TargetType as a standalone enum and pass it along with the ResourceRequest everywhere an ResourceRequest is passed around in loader/ - I think that's the more likely option right now

See also https://bugs.webkit.org/show_bug.cgi?id=65855

[Lyon Chen]

(In reply to comment #23)
> See also https://bugs.webkit.org/show_bug.cgi?id=65855

Thanks for the link, now I have a better understanding of the whole situation. How about add 2 more methods to FrameLoaderClient?


    void addExtraFieldsToSubresourceRequest(ResourceRequest& request, CachedResource::Type type);
    void addExtraFieldsToMainResourceRequest(ResourceRequest& request, FrameLoadType type);

And then we can do whatever our own portal like to the ResourceRequest, without all these platfrom #ifdefs.

[Lyon Chen]

(In reply to comment #24)
> Thanks for the link, now I have a better understanding of the whole situation. How about add 2 more methods to FrameLoaderClient?
> 
> 
>     void addExtraFieldsToSubresourceRequest(ResourceRequest& request, CachedResource::Type type);
>     void addExtraFieldsToMainResourceRequest(ResourceRequest& request, FrameLoadType type);
> 
> And then we can do whatever our own portal like to the ResourceRequest, without all these platfrom #ifdefs.

Sorry I forgot to mention: and calling them in FrameLoader::addExtraFieldsToSubresourceRequest() and FrameLoader::addExtraFieldsToMainResourceRequest().

[Lyon Chen]

Created attachment 135118 [details]
Updated patch based on review.

Based on the review comments, and comments of bug https://bugs.webkit.org/show_bug.cgi?id=80713, update the patch to limit the change to BlackBerry port files, and only change the ResourceRequest in dispatchWillSendRequest(), at the same time also adjust the TargetType of platform NetworkRequest for dispatchDecidePolicyForNavigationAction() and decidePolicyForExternalLoad().

[Lyon Chen]

Created attachment 135150 [details]
Fix compile error and some minor coding style fix.

[Rob Buis]

Comment on attachment 135150 [details]
Fix compile error and some minor coding style fix.

Looks good.

[WebKit Review Bot]

Comment on attachment 135150 [details]
Fix compile error and some minor coding style fix.

Clearing flags on attachment: 135150

Committed r112940: <http://trac.webkit.org/changeset/112940>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.