Bug 80519

Summary: [BlackBerry] Set ResourceRequest TargetType in WebPagePrivate::load()
Product: WebKit Reporter: Lyon Chen <liachen>
Component: WebKit BlackBerryAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, japhet, jochen, joenotcharles, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 80713    
Bug Blocks:    
Attachments:
Description Flags
Patch for 80519
none
Updated patch to set correct target type for WebKit internal loads too.
abarth: review-
Updated patch based on review.
none
Fix compile error and some minor coding style fix. none

Description Lyon Chen 2012-03-07 10:17:23 PST
Right now the TargetType of ResourceRequest for main load is not set, set them in WebPagePrivate::load().
Comment 1 Lyon Chen 2012-03-07 10:27:16 PST
Created attachment 130644 [details]
Patch for 80519
Comment 2 Lyon Chen 2012-03-07 11:20:17 PST
Created attachment 130653 [details]
Updated patch to set correct target type for WebKit internal loads too.
Comment 3 Nate Chapin 2012-03-07 11:33:20 PST
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().
Comment 4 Joe Mason 2012-03-07 12:00:53 PST
(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.
Comment 5 Lyon Chen 2012-03-07 12:23:37 PST
(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.
Comment 6 Lyon Chen 2012-03-07 12:26:15 PST
(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.
Comment 7 Antonio Gomes 2012-03-07 12:37:13 PST
(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?
Comment 8 Antonio Gomes 2012-03-07 12:37:46 PST
Nate, any suggestion based on the above?
Comment 9 Lyon Chen 2012-03-07 12:51:20 PST
(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.
Comment 10 Nate Chapin 2012-03-07 12:52:37 PST
(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.
Comment 11 Lyon Chen 2012-03-07 13:03:56 PST
(In reply to comment #10)
> It might be good to give abarth a chance to comment, too.

Thanks, pinged abarth on #webkit.
Comment 12 Adam Barth 2012-03-07 17:09:32 PST
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?
Comment 13 Joe Mason 2012-03-07 17:52:12 PST
(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.
Comment 14 Adam Barth 2012-03-07 18:06:47 PST
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.
Comment 15 Lyon Chen 2012-03-07 19:00:31 PST
(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.
Comment 16 jochen 2012-03-08 05:24:14 PST
(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.
Comment 17 Lyon Chen 2012-03-08 06:34:49 PST
(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?
Comment 18 jochen 2012-03-08 06:49:07 PST
(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.
Comment 19 Lyon Chen 2012-03-08 06:59:09 PST
(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.
Comment 20 jochen 2012-03-08 07:02:21 PST
(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.
Comment 21 Lyon Chen 2012-03-08 07:09:11 PST
(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?
Comment 22 jochen 2012-03-08 07:13:15 PST
(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
Comment 23 jochen 2012-03-08 07:14:02 PST
See also https://bugs.webkit.org/show_bug.cgi?id=65855
Comment 24 Lyon Chen 2012-03-08 12:18:48 PST
(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.
Comment 25 Lyon Chen 2012-03-08 12:21:58 PST
(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().
Comment 26 Lyon Chen 2012-04-02 09:18:01 PDT
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().
Comment 27 Lyon Chen 2012-04-02 12:05:54 PDT
Created attachment 135150 [details]
Fix compile error and some minor coding style fix.
Comment 28 Rob Buis 2012-04-02 12:20:07 PDT
Comment on attachment 135150 [details]
Fix compile error and some minor coding style fix.

Looks good.
Comment 29 WebKit Review Bot 2012-04-02 14:15:39 PDT
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>
Comment 30 WebKit Review Bot 2012-04-02 14:15:45 PDT
All reviewed patches have been landed.  Closing bug.