Bug 29975

Created attachment 40468 [details] Patch

Created attachment 40515 [details] Patch Rename the signal, as agreed on IRC.

Created attachment 40518 [details] Patch More doc changes as discussed on IRC.

*** Bug 28884 has been marked as a duplicate of this bug. ***

Comment on attachment 40518 [details] Patch r=me, but please fix "signel" typo when landing and add \preliminary to the API docs.

Committed r49100: <http://trac.webkit.org/changeset/49100>

Comment on attachment 40518 [details] Patch Clearing review after landing

After resubscribing everyone I'd like to re-open the discussion about this signal / topic. We had a 1.5 hours discussion around various alternate solutions to this API, that appears a little bit out of place in QWebPage. And we always came back to the one question: * What exactly is the use-case that requires an assocation of network requests with frames? Yael, Robert, you were the first ones asking for this API, perhaps you can help us with these questions? Could you explain your use-case here in Bugzilla in detail? Why does it have to be per-frame? Is the assocation all you need to implement your use-case or is it just a tool on the way? Are you both perhaps looking for a way to filter network requests? What kind of filtering are you looking for? Do you need a convenient way to just accept or reject a request or do you have to change individual properties of the requests that are somehow associated with the frame? Please help us, so that we can help you to find the right API.

> Could you explain your use-case here in Bugzilla in detail? > Why does it have to be per-frame? In our web runtime, each frame could have its own origin, hence its own security context. We need to know which frame initiated the request, so that we can decide if to allow the request or not. Once we know the frame, there is a lot of preprocessing we can do.

(In reply to comment #9) > In our web runtime, each frame could have its own origin, hence its own > security context. We need to know which frame initiated the request, so that we > can decide if to allow the request or not. > Once we know the frame, there is a lot of preprocessing we can do. So each frame is independent, am I right? Why you don't use different QWebPage for different contents?

(In reply to comment #10) > So each frame is independent, am I right? Why you don't use different QWebPage > for different contents? I was talking about iframes in one QWebPage, not sure how to do what you are suggesting.

* What exactly is the use-case that requires an assocation of network requests with frames? When you get the unsupportedContent signal you want to know what frame it came from so you should know how to handle the error. If it is the main frame then you load up a large error page. If it is a subframe then you do something different. In the existing Wallet support of Arora we must wait until the createRequest comes through to QNetworkAccessManager to be able to read the outgoingData which contains the final form data. At this point in time We only have a QNetworkRequest and need a way to make a request->qwebframe to find the form in the frame. Not only do we want the frame, but we want the QWebPage::NavigationType. On the status bar Arora currently only shows a percentage of the webpage that was loaded. If we had a way to see what frame requests came from we could provide better information about what a page is downloading. All of the QNetworkAccessManager signals that bring up a dialog currently only give you a QNetworkReply void authenticationRequired ( QNetworkReply * reply, QAuthenticator * authenticator ) void finished ( QNetworkReply * reply ) void proxyAuthenticationRequired ( const QNetworkProxy & proxy, QAuthenticator * authenticator ) void sslErrors ( QNetworkReply * reply, const QList<QSslError> & errors ) Because of this if you have multiple QWebPages you must block the entire application rather then just the QWebPage that need to prompt the user for something. QtWebKit actively encourages that application have only 1 QNetworkAccessManager that is shared among all of the existing QWebPages. There are a number of valid reasons for this - Only X number of connections to a host at a time - QNetworkCookieJar and QNetworkDiskCache syncing issues - Memory and cpu usage of running multiple of these of course While you can go through and setup a QNetworkAccessManager that is a proxy for each QWebPage this has many downsides such as more confusing code, memory overhead for each manager/cookiejar object. You can see this approach in Arora today here: http://github.com/Arora/arora/tree/master/src/utils/ Talking with Thiago back in the day about this he mentioned how QNetworkAccessManager and QNetworkRequest were designed so that it was QNetworkRequest that had the ability for developers to add data to that would be sent through the system using QNetworkRequest::User http://doc.trolltech.com/4.5/qnetworkrequest.html#Attribute-enum Having a QNetworkAccessManager proxy is working around the fact that QtWebKit currently actively doesn't let you set anything in QNetworkRequests.

(In reply to comment #12) > * What exactly is the use-case that requires an assocation of network > requests with frames? > > When you get the unsupportedContent signal you want to know what frame it came > from so you should know how to handle the error. If it is the main frame then > you load up a large error page. If it is a subframe then you do something > different. In 4.6 errors should be handled before unsupportedContent is called. There was a bug before preventing this from happening. This is not the right place to handle errors.

> > * What exactly is the use-case that requires an assocation of network > requests with frames? > > Could you explain your use-case here in Bugzilla in detail? > Why does it have to be per-frame? When a qnetworkreply/qnetworkrequest encounters an SSL error, I need to know from which qwebframe the request originated so I can display the error there. The SSL error is displayed as a HTML page with proceed/cancel buttons within the frame. Ideally the qwebpage information would be available explicitly too, but it is easy enough to get at once you have the frame pointer. > Is the assocation all you need to implement your use-case or is it just a tool > on the way? > Are you both perhaps looking for a way to filter network requests? > What kind of filtering are you looking for? > Do you need a convenient way to just accept or reject a request or do you have > to change individual properties of the requests that are somehow associated > with the frame? In my case the answer to all of the above is 'no', since the request is aborted by default and you have to emit reply->ignoreSslErrors() if you want to ignore the error. Yael's patch is enough for me. > > > Please help us, so that we can help you to find the right API.

(In reply to comment #14) > > > > * What exactly is the use-case that requires an assocation of network > > requests with frames? > > > > Could you explain your use-case here in Bugzilla in detail? > > Why does it have to be per-frame? > > When a qnetworkreply/qnetworkrequest encounters an SSL error, I need to know > from which qwebframe the request originated so I can display the error there. > The SSL error is displayed as a HTML page with proceed/cancel buttons within > the frame. > > Ideally the qwebpage information would be available explicitly too, but it is > easy enough to get at once you have the frame pointer. Is your specific use-case also covered by Kenneth's QWebPage::ErrorPageExtension?

(In reply to comment #11) > (In reply to comment #10) > > So each frame is independent, am I right? Why you don't use different QWebPage > > for different contents? > > I was talking about iframes in one QWebPage, not sure how to do what you are > suggesting. So in your setup you have one QWebPage that places different iframes into layout you define and each iframe shows external content with different origins, right? What is the reason for combining all documents through iframes into one QWebPage as opposed to using one QWebPage per document? Is it reduced memory usage or do you for example need to allow some documents to share the same security origin so that they can exchange data?

> So in your setup you have one QWebPage that places different iframes into > layout you define and each iframe shows external content with different > origins, right? > > What is the reason for combining all documents through iframes into one > QWebPage as opposed to using one QWebPage per document? Is it reduced memory > usage or do you for example need to allow some documents to share the same > security origin so that they can exchange data? Every other webkit based browser behaves this way. There is one page that contains multiple frames. I see no reason to change that model, just to avoid adding this signal.

(In reply to comment #12) [...] > In the existing Wallet support of Arora we must wait until the createRequest > comes through to QNetworkAccessManager to be able to read the outgoingData > which contains the final form data. At this point in time We only have a > QNetworkRequest and need a way to make a request->qwebframe to find the form in > the frame. Not only do we want the frame, but we want the > QWebPage::NavigationType. That is a good example! So in an ideal world you'd have the QWebFrame pointer as an argument in the virtual createRequest function in QNetworkAccessManager, right? We might be able to fix that, but what about the navigation type? By the time we create the QNetworkRequest we do not know the underlying Navigation type. Can you explain how you would use the NavigationType? What we otherwise thought of was to have an extension in QWebPage, similar to Robert's proposal, that would change the current call chain from QNetworkReplyHandler -> QNetworkAccessManager::createRequest() to QNetworkReplyHandler -> QWebPage::CreateNetworkRequestExtension-with-same-arguments-as-createRequest-and-a-QWebFrame-pointer -> QNetworkAccessManager::createRequest(), where you could hook into the middle part. > On the status bar Arora currently only shows a percentage of the webpage that > was loaded. If we had a way to see what frame requests came from we could > provide better information about what a page is downloading. Can you elaborate on what you would display? > All of the QNetworkAccessManager signals that bring up a dialog currently only > give you a QNetworkReply > > void authenticationRequired ( QNetworkReply * reply, QAuthenticator * > authenticator ) > void finished ( QNetworkReply * reply ) > void proxyAuthenticationRequired ( const QNetworkProxy & proxy, QAuthenticator > * authenticator ) > void sslErrors ( QNetworkReply * reply, const QList<QSslError> & errors ) > > Because of this if you have multiple QWebPages you must block the entire > application rather then just the QWebPage that need to prompt the user for > something. That is a very good point! > QtWebKit actively encourages that application have only 1 QNetworkAccessManager > that is shared among all of the existing QWebPages. There are a number of > valid reasons for this > - Only X number of connections to a host at a time > - QNetworkCookieJar and QNetworkDiskCache syncing issues > - Memory and cpu usage of running multiple of these of course > > While you can go through and setup a QNetworkAccessManager that is a proxy for > each QWebPage this has many downsides such as more confusing code, memory > overhead for each manager/cookiejar object. You can see this approach in Arora > today here: http://github.com/Arora/arora/tree/master/src/utils/ That is also a very good point. It should not be necessary to write such a proxy. Avoiding it should be a goal of the API. Thanks for the elaborate explanation! > Talking with Thiago back in the day about this he mentioned how > QNetworkAccessManager and QNetworkRequest were designed so that it was > QNetworkRequest that had the ability for developers to add data to that would > be sent through the system using QNetworkRequest::User > http://doc.trolltech.com/4.5/qnetworkrequest.html#Attribute-enum > > Having a QNetworkAccessManager proxy is working around the fact that QtWebKit > currently actively doesn't let you set anything in QNetworkRequests. I admit I like the idea of adding properties or attributes to QNetworkRequest "on the way" as it is passed through the software stack, but I am concerned about storing a pointer to a QObject in this value based class.

(In reply to comment #17) > > So in your setup you have one QWebPage that places different iframes into > > layout you define and each iframe shows external content with different > > origins, right? > > > > What is the reason for combining all documents through iframes into one > > QWebPage as opposed to using one QWebPage per document? Is it reduced memory > > usage or do you for example need to allow some documents to share the same > > security origin so that they can exchange data? > > Every other webkit based browser behaves this way. There is one page that > contains multiple frames. I see no reason to change that model, just to avoid > adding this signal. Yael, I'm not trying to say that your use-case is wrong, I'm just trying to understand what you're doing :-). Let me ask differently: Are you showing different widsets in the web runtime within the same page by using iframes?

(In reply to comment #12) [...] > QtWebKit actively encourages that application have only 1 QNetworkAccessManager > that is shared among all of the existing QWebPages. There are a number of > valid reasons for this > - Only X number of connections to a host at a time > - QNetworkCookieJar and QNetworkDiskCache syncing issues > - Memory and cpu usage of running multiple of these of course I think these are very very valid points, but I also think they are technically bugs in the QNetworkAccessManager implementation. Multiple QNAMs should share the connection count. The implementation should allow for sharing CookieJar and DiskCache, just as the API suggest that this is possible. Do you agree Benjamin?

(In reply to comment #19) > Let me ask differently: Are you showing different widsets in the web runtime > within the same page by using iframes? Each widget is treated the same as a web page, and has its own QWebPage. The use case is mashups, where a widget is getting content from multiple sources into multiple iframes. Each iframe has its own origin and security context.

>> QtWebKit actively encourages that application have only 1 QNetworkAccessManager >> that is shared among all of the existing QWebPages. There are a number of >> valid reasons for this >> - Only X number of connections to a host at a time >> - QNetworkCookieJar and QNetworkDiskCache syncing issues >> - Memory and cpu usage of running multiple of these of course > > I think these are very very valid points, but I also think they are technically > bugs in the QNetworkAccessManager implementation. Multiple QNAMs should share > the connection count. The implementation should allow for sharing CookieJar and > DiskCache, just as the API suggest that this is possible. I guess what I was meant to say way "QtWebKit actively encourages that application have only 1 QNetworkAccessManager, 1 CookieJar and 1 cache that is shared among all of the existing QWebPages." You can have multiple QNetworkAccessManagers and they could share the objects, but it is not encouraged. For example when you set a cookie jar on a QNetworkAccessManagers it sets the parent to itself. So it is technically possible the network code is not designed to work with this without some extra work.

(In reply to comment #15) > > > > Ideally the qwebpage information would be available explicitly too, but it is > > easy enough to get at once you have the frame pointer. > > Is your specific use-case also covered by Kenneth's > QWebPage::ErrorPageExtension? It looks like the natural place to put some of the client-specific things I do so potentially, yes.

Ok, I sat down with Lars and we submitted a patch into Qt 4.6 that adds two methods to QNetworkRequest: --- a/src/network/access/qnetworkrequest.h +++ b/src/network/access/qnetworkrequest.h @@ -120,6 +120,9 @@ public: void setSslConfiguration(const QSslConfiguration &configuration); #endif · + void setOriginatingObject(QObject *object); + QObject *originatingObject() const; Internally that QObject is stored as a QWeakPointer, so originatingObject() will return a null pointer if the object in question died in the meantime. What needs to be done now is to extend QtWebKit to use that API and set the QWebFrame as the originating object. Then we can remove the signal from QWebPage again, as it is not necessary anymore.

(In reply to comment #24) > Ok, I sat down with Lars and we submitted a patch into Qt 4.6 that adds two > methods to QNetworkRequest: > > --- a/src/network/access/qnetworkrequest.h > +++ b/src/network/access/qnetworkrequest.h > @@ -120,6 +120,9 @@ public: > void setSslConfiguration(const QSslConfiguration &configuration); > #endif > · > + void setOriginatingObject(QObject *object); > + QObject *originatingObject() const; > > > Internally that QObject is stored as a QWeakPointer, so originatingObject() > will return a null pointer if the object in question died in the meantime. > > What needs to be done now is to extend QtWebKit to use that API and set the > QWebFrame as the originating object. Then we can remove the signal from > QWebPage again, as it is not necessary anymore. I am guessing that this will only be available in Qt 4.6, but our release is based on Qt 4.5, so this has to be carefully coordinated.

(In reply to comment #25) > (In reply to comment #24) > > Ok, I sat down with Lars and we submitted a patch into Qt 4.6 that adds two > > methods to QNetworkRequest: > > > > --- a/src/network/access/qnetworkrequest.h > > +++ b/src/network/access/qnetworkrequest.h > > @@ -120,6 +120,9 @@ public: > > void setSslConfiguration(const QSslConfiguration &configuration); > > #endif > > · > > + void setOriginatingObject(QObject *object); > > + QObject *originatingObject() const; > > > > > > Internally that QObject is stored as a QWeakPointer, so originatingObject() > > will return a null pointer if the object in question died in the meantime. > > > > What needs to be done now is to extend QtWebKit to use that API and set the > > QWebFrame as the originating object. Then we can remove the signal from > > QWebPage again, as it is not necessary anymore. > > I am guessing that this will only be available in Qt 4.6, but our release is > based on Qt 4.5, so this has to be carefully coordinated. Yes. Would it be feasible for you to apply the patch in question against your Qt 4.5 version?

(In reply to comment #26) > Yes. Would it be feasible for you to apply the patch in question against your > Qt 4.5 version? Probably yes, this is more of a coordination issue :-)

Created attachment 42375 [details] Patch

Committed r50454: <http://trac.webkit.org/changeset/50454>

*** Bug 25166 has been marked as a duplicate of this bug. ***