Bug 65544 - Introduce a new ResourceRequest::TargetType for XHRs
Summary: Introduce a new ResourceRequest::TargetType for XHRs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: jochen
URL:
Keywords:
Depends on: 65728
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-02 08:27 PDT by jochen
Modified: 2011-08-05 11:02 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.03 KB, patch)
2011-08-02 08:27 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (3.21 KB, patch)
2011-08-04 08:35 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (3.20 KB, patch)
2011-08-04 12:29 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (3.20 KB, patch)
2011-08-05 06:49 PDT, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2011-08-02 08:27:04 PDT
Introduce a new ResourceRequest::TargetType for XHRs
Comment 1 jochen 2011-08-02 08:27:39 PDT
Created attachment 102655 [details]
Patch
Comment 2 jochen 2011-08-02 11:01:39 PDT
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 3 Adam Barth 2011-08-02 11:59:31 PDT
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?
Comment 4 jochen 2011-08-02 12:04:00 PDT
(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
Comment 5 Alexey Proskuryakov 2011-08-02 12:06:29 PDT
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 ***
Comment 6 Adam Barth 2011-08-02 12:09:33 PDT
Sounds like we need to resolve Bug 48483 before this gets worse.
Comment 7 Darin Fisher (:fishd, Google) 2011-08-02 12:41:00 PDT
(In reply to comment #2)
> ResourceRequests by XMLHttpRequest won't go through WebFrameClient::willSendRequest

^^^ Are you sure about that?  This surprises me a lot.
Comment 8 Darin Fisher (:fishd, Google) 2011-08-02 12:42:32 PDT
(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.
Comment 9 jochen 2011-08-02 12:48:51 PDT
(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 :-/
Comment 10 jochen 2011-08-04 08:35:44 PDT
Created attachment 102919 [details]
Patch
Comment 11 jochen 2011-08-04 08:39:07 PDT
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)
Comment 12 jochen 2011-08-04 12:29:32 PDT
Created attachment 102956 [details]
Patch
Comment 13 jochen 2011-08-04 12:30:25 PDT
Darin, could you have a look?
Comment 14 Adam Barth 2011-08-04 12:53:33 PDT
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.
Comment 15 jochen 2011-08-04 12:56:40 PDT
(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
Comment 16 Adam Barth 2011-08-04 13:00:30 PDT
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 17 WebKit Review Bot 2011-08-04 14:50:18 PDT
Comment on attachment 102956 [details]
Patch

Clearing flags on attachment: 102956

Committed r92415: <http://trac.webkit.org/changeset/92415>
Comment 18 WebKit Review Bot 2011-08-04 14:50:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Adam Barth 2011-08-04 15:53:26 PDT
Re-opened because this patch caused crashes.  See the attached rollout patch for details.
Comment 20 jochen 2011-08-05 06:49:45 PDT
Created attachment 103068 [details]
Patch
Comment 21 jochen 2011-08-05 06:51:04 PDT
unconditionally #define'ing now, fixes the failures for me
Comment 22 WebKit Review Bot 2011-08-05 11:02:21 PDT
Comment on attachment 103068 [details]
Patch

Clearing flags on attachment: 103068

Committed r92487: <http://trac.webkit.org/changeset/92487>
Comment 23 WebKit Review Bot 2011-08-05 11:02:27 PDT
All reviewed patches have been landed.  Closing bug.