Bug 65544 – Introduce a new ResourceRequest::TargetType for XHRs

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

Introduce a new ResourceRequest::TargetType for XHRs

Created attachment 102655 [details]
Patch

ResourceRequests by XMLHttpRequest won't go through WebFrameClient::willSendRequest, so I don't have a chance to mark them. As they can be attributed to a Document (or Worker) via the ScriptExecutionContext, I wonder whether there's a way to tag those requests, or whether it was possible to introduce something like ScriptExecutionContextClient::willSendRequest?

[Adam Barth]

Comment on attachment 102655 [details]
Patch

I'm surprised ResourceRequest has a TargetType.  ResourceRequest isn't supposed to know that it's in a web browser.  How does that property round-trip through NSURLRequest?

(In reply to comment #3)
> (From update of attachment 102655 [details])
> I'm surprised ResourceRequest has a TargetType.  ResourceRequest isn't supposed to know that it's in a web browser.  How does that property round-trip through NSURLRequest?

I don't think it does. TargetType is actually part of ResourceRequestBase, and a quick git grep suggests that only chromium uses this property at all

[Alexey Proskuryakov]

Let's reopen the original bug if you disagree with its resolution.

See also: 48483.

*** This bug has been marked as a duplicate of bug 61542 ***

[Adam Barth]

Sounds like we need to resolve Bug 48483 before this gets worse.

[Darin Fisher (:fishd, Google)]

(In reply to comment #2)
> ResourceRequests by XMLHttpRequest won't go through WebFrameClient::willSendRequest

^^^ Are you sure about that?  This surprises me a lot.

[Darin Fisher (:fishd, Google)]

(In reply to comment #7)
> (In reply to comment #2)
> > ResourceRequests by XMLHttpRequest won't go through WebFrameClient::willSendRequest
> 
> ^^^ Are you sure about that?  This surprises me a lot.

If you find that this is true for some requests, then perhaps we should fix the code to always send requests through willSendRequest.

(In reply to comment #7)
> (In reply to comment #2)
> > ResourceRequests by XMLHttpRequest won't go through WebFrameClient::willSendRequest
> 
> ^^^ Are you sure about that?  This surprises me a lot.

no, I'm not

probably I'm mistaken :-/

Created attachment 102919 [details]
Patch

As discussed in  bug 48483, I'll move the target type to chromium's ResourceRequest and add TargetIsXHR (this patch is based on the pending patch for bug 48483 so I can't submit it to EWS yet)

Created attachment 102956 [details]
Patch

Darin, could you have a look?

[Adam Barth]

Isn't this still moving us in the opposite direction of where we want to go?  This would seem to make it harder to remove this field in the future.

(In reply to comment #14)
> Isn't this still moving us in the opposite direction of where we want to go?  This would seem to make it harder to remove this field in the future.

My understanding is that I'll work on something like LoadRequest or whatever it'll be called that would carry this information, and the embedder can connect information from the LoadRequest using ResourceRequest::extraData in its willSendRequest() implementation

until then, adding this target type will unblock my webRequest API work

[Adam Barth]

Ok.  I'll let Darin review the actual patch.  If you're planning to remove this field yourself, then you're not imposing this cost on anyone else, which sounds fine to me.

[WebKit Review Bot]

Comment on attachment 102956 [details]
Patch

Clearing flags on attachment: 102956

Committed r92415: <http://trac.webkit.org/changeset/92415>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

Re-opened because this patch caused crashes.  See the attached rollout patch for details.

Created attachment 103068 [details]
Patch

unconditionally #define'ing now, fixes the failures for me

[WebKit Review Bot]

Comment on attachment 103068 [details]
Patch

Clearing flags on attachment: 103068

Committed r92487: <http://trac.webkit.org/changeset/92487>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.