| Summary: | Introduce a new ResourceRequest::TargetType for XHRs | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | jochen | ||||||||||
| Component: | Page Loading | Assignee: | jochen | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | abarth, ap, fishd, webkit.review.bot | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Bug Depends on: | 65728 | ||||||||||||
| Bug Blocks: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
jochen
2011-08-02 08:27:04 PDT
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? 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 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 *** Sounds like we need to resolve Bug 48483 before this gets worse. (In reply to comment #2) > ResourceRequests by XMLHttpRequest won't go through WebFrameClient::willSendRequest ^^^ Are you sure about that? This surprises me a lot. (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? 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 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. Comment on attachment 102956 [details] Patch Clearing flags on attachment: 102956 Committed r92415: <http://trac.webkit.org/changeset/92415> All reviewed patches have been landed. Closing bug. 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 Comment on attachment 103068 [details] Patch Clearing flags on attachment: 103068 Committed r92487: <http://trac.webkit.org/changeset/92487> All reviewed patches have been landed. Closing bug. |