Bug 28884

Created attachment 38876 [details] Add CreateRequestExtension

Created attachment 38880 [details] Update patch to add missing doc Was missing doc for CreateRequestExtension (had only doc'd *Option and *Return values)

Comment on attachment 38880 [details] Update patch to add missing doc I understand what you're trying to achieve (through earlier emails), but I don't understand who this extension helps you to be able to track the originating frame of a QNetworkRequest if nothing in WebCore/WebKit actually call this function. The documentation also doesn't explain what I'm wondering about: * What's the use of the output value? * Why does it have to be filled out? Usually the extension pattern is used to allow developers to change existing behaviour where we would normally add a new virtual function but cannot because of binary compatibility constraints. Perhaps all you need is a signal that is emitted whenever a request is created? :) Perhaps the request callbacks in FrameLoaderClientQt.cpp could be the source that triggers the API?

Yah looking that part of the patch is missing. The purpose of using the extension system is so that anyone can add new attributes to the request so that later one could look at a reply->request() and pull out any user atrributes from the request (such as a frame pointer). emiting a signal with a pointer to a request is one way to go, but the extension system seemed more Qtish.

Created attachment 39517 [details] Updated Patch Corrected patch - qnetworkreplyhandler now calls the extension and uses the qnetworkrequest returned by its output. This will allow clients to reimplement extension() and modify the qnetworkrequest's attributes to contain a pointer to the frame, for example.

Comment on attachment 39517 [details] Updated Patch > + // Calling CreateRequestExtension allows clients who implement it to perform > + // operations on the QNetworkRequest::Attributes. For example, clients could > + // store a pointer to the frame in QNetworkRequest::User + X. This would allow > + // the client to associate the network request with it's originating frame. > + QWebPage::CreateRequestExtensionOption option; > + option.frame = d->m_frame; > + option.request = m_request; > + option.type = QWebPage::NavigationTypeOther; > + > + QWebPage::CreateRequestExtensionReturn output; > + if (d->m_frame->page()->extension(QWebPage::CreateRequestExtension, &option, &output)) > + m_request = output.request; I am not sure about this part. Seems that we will need to construct both structures even in the case (probably 90%) where this extension will not be used by the client. That seems like a waste. Is there a way to not support the extension by default?

(In reply to comment #6) > > I am not sure about this part. Seems that we will need to construct both > structures even in the case (probably 90%) where this extension will not be > used by the client. > That seems like a waste. > > Is there a way to not support the extension by default? Well we could add the extension to qwebpage, but return it as false in supportsExtension() by default. It would then be up to the client to reimplement supportsExtension() to indicate support of CreateRequestExtension, as well as extension() to manipulate the QNetworkRequest. Would that do?

Created attachment 40014 [details] Updated Patch Updated to only call extension() if client has reimplemented supportsExtension() to support it.

I admit I still don't understand why such a complex API is necessary, in particular that the implementation of the extension _has_ to copy in the incoming request into the output argument, otherwise you get random crashes. That IMHO is bad and too complex API. Why is it necessary to allow the developer to provide a complete replacement for the network request? Why wouldn't a simple single notification signal in QWebPage (with a frame and the request as argument) be sufficient for your use-case?

(In reply to comment #9) > I admit I still don't understand why such a complex API is necessary, in > particular that the implementation of the extension _has_ to copy in the > incoming request into the output argument, otherwise you get random crashes. > That IMHO is bad and too complex API. > I agree with you here, though I think the complexity is largely a feature of the extension() API design. > Why is it necessary to allow the developer to provide a complete replacement > for the network request? > > Why wouldn't a simple single notification signal in QWebPage (with a frame and > the request as argument) be sufficient for your use-case? It would certainly be sufficient for my use-case and I would happily proceed on that basis. If I understand the discussion that has taken place on this, the extension() API offers the following benefits over a simple signal: 1. It supports the design decision of introducing the extension() API, i.e. it's the new Qt-ish way of doing things. 2. It exposes the QNetworkRequest attributes() to clients in a way that allows them to make better use of them than is currently available. That's my (possibly) incomplete understanding of it. I think Ben or others may be able to add more to the case.

If simon is ok with having a signal where the request is a pointer I am ok with it. Passing a pointer to a temporary object in a signal where it is expected the receiver will only be one object and will modify the pointer didn't seem to match up to the usual qt api, but it is cleaner then the extension way. void requestCreated(QNetworkRequest *request, QWebFrame *frame, type) Simon?

(In reply to comment #11) > If simon is ok with having a signal where the request is a pointer I am ok with > it. Passing a pointer to a temporary object in a signal where it is expected > the receiver will only be one object and will modify the pointer didn't seem to > match up to the usual qt api, but it is cleaner then the extension way. > > void requestCreated(QNetworkRequest *request, QWebFrame *frame, type) Well, the extension didn't change anything about that, didn't it? I just made it harder to use :) Think of this similar to QWebPage's frameCreated() signal. I wonder if it should be a signal of QWebFrame or QWebPage. If the latter, then I guess the frame should be the first argument? What's type? :)

Woops, that should have been void requestCreated(QNetworkRequest *request, QWebFrame *frame, QWebPage::NavigationType type) The frame and the type are the two args passed to the extension in the patch.

Created attachment 40595 [details] possible patch Not marking this one for review, because I'm not sure it meets Ben's needs (though it meets mine). The requestCreate() signal is emitted from qnetworkreplyhandler->start() - this ensures all networkrequests handled by qtwebkit are caught. However NavigationType is not available there so I can't include it in the signal.

(In reply to comment #14) > Created an attachment (id=40595) [details] > possible patch > > Not marking this one for review, because I'm not sure it meets Ben's needs > (though it meets mine). > > The requestCreate() signal is emitted from qnetworkreplyhandler->start() - this > ensures all networkrequests handled by qtwebkit are caught. However > NavigationType is not available there so I can't include it in the signal. Please have a look at https://bugs.webkit.org/show_bug.cgi?id=29975

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