Bug 49113 – [chromium] Add a way for the embedder to tag a ResourceRequest with arbitrary data

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Platform

[Darin Fisher (:fishd, Google)]

[chromium] Add a way for the embedder to tag a ResourceRequest with arbitrary data

The problem is that we are teaching WebKit to store fields on ResourceRequest that
are meaningless to WebKit.  We are doing that just so we can pass information from
WebFrameClient::willSendRequest to WebURLLoader.  m_requestorID, m_requestorProcessID,
and now m_hasUserGesture (see bug 37057) are in this category.

I suspect we may be better served by generalizing this.  We already do something
like that for WebDataSource.  See WebDataSource::ExtraData.  One difference is that
the ExtraData that would be associated with a ResourceRequest needs to be copied when
a ResourceRequest is copied.  The DocumentLoader (which WebDataSource wraps) does not
get copied in the same way, so it does not have this problem.

Something like reference counting seems like the right answer...

class WebURLRequest {
public:
    class ExtraData {
    public:
        virtual void ref() = 0;
        virtual void deref() = 0;
    protected:
        ~ExtraData() { }
    };

    WEBKIT_API ExtraData* extraData() const;
    WEBKIT_API void setExtraData(ExtraData*);
    ...
};

Unfortunately, though, we do not permit referring to WebKit:: classes
from WebCore, so we wouldn't want to directly stash one of these
WebURLRequest::ExtraData pointers on ResourceRequest.  We'd need to
either create another interface like ExtraData at the ResourceRequest
level, or we'd need to find some way to stash the pointer in some
opaque fashion :-(  I don't have a good answer for this.

We also need to worry about CrossThreadResourceRequestData, which is
used to copy the contents of a ResourceRequest across threads.  Or,
perhaps we can say that the ExtraData here would not be shared across
threads.  (I don't think we need it to be copied in Chromium, but that
might change in the future.)

[Adam Barth]

Did you have a patch you were planning to upload here, or are you looking for someone to implement this?

[Pierre-Antoine LaFayette]

(In reply to comment #1)
> Did you have a patch you were planning to upload here, or are you looking for someone to implement this?

I'll be working on this refactoring once I finish bug 37057. If you have thoughts on the design, I'm listening. Thanks.

[Adam Klein]

I'm looking into tackling this.  But another thing that makes it trickier than WebDataSource is that the code manipulating these extra bits in the request doesn't all live in RenderView::willSendRequest().  Some lives there, but m_requestorProcessID is set in WebPluginImpl::InitiateHTTPRequest, while m_appCacheHostID is set by WebApplicationCacheHostImpl::willStartMainResourceRequest.

[Pierre-Antoine LaFayette]

Sorry I haven't been able to get to this. When I last looked, I was also concerned about how many of these "extra" fields that WebKit doesn't care about can easily be stored in the ExtraData object. I believe I had just added m_requestorID and m_hasUserGesture to the ExtraData since, as you mentioned, those are set in willSendRequest. 

Hopefully, Darin can provide some guidance here.

(In reply to comment #3)
> I'm looking into tackling this.  But another thing that makes it trickier than WebDataSource is that the code manipulating these extra bits in the request doesn't all live in RenderView::willSendRequest().  Some lives there, but m_requestorProcessID is set in WebPluginImpl::InitiateHTTPRequest, while m_appCacheHostID is set by WebApplicationCacheHostImpl::willStartMainResourceRequest.

[Darin Fisher (:fishd, Google)]

(In reply to comment #3)
> I'm looking into tackling this.  But another thing that makes it trickier than WebDataSource is that the code manipulating these extra bits in the request doesn't all live in RenderView::willSendRequest().

I see... that suggests that we'd need to have a common subclass of ExtraData that all code in Chromium can expect a WebURLRequest::ExtraData to downcast to.  We could probably put something in src/webkit/glue/ to help make that less error prone.

I also wonder if this kind of thing wouldn't be much easier to support if ResourceRequest were reference counted, and if the allocation of ResourceRequest were done by the embedding layer.  Then we could just extend the size of the ResourceRequest structure at the embedding layer.  /shrug... not sure what is best.

[Darin Fisher (:fishd, Google)]

I was just mentioning to Nate that this bug might be a lot easier to resolve if ResourceRequest and ResourceResponse were reference counted.

This came up while he was mentioning that those structs get copied around a lot, and perhaps it would be nice if they were themselves reference counted.

I think this is important to sort out because if we were to implement this bug without changing ResourceRequest, then the holder for the embedder specific data would most likely need to be reference counted.  However, if ResourceRequest itself is reference counted, then the holder no longer needs to be reference counted.

Hmm.....

Note that the extraData field was added to ResourceRequest and WebURLRequest

what remains to do is to actually tag all requests. Currently, chromium only tags requests that go through WebFrameClient::willSendRequest

Fixed in http://trac.webkit.org/changeset/87015

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