|Summary:||[Qt] REGRESSION(r87797): Broke KDEWebKit's custom QNAM.|
|Product:||WebKit||Reporter:||Dawit A. <adawit>|
|Component:||Page Loading||Assignee:||Nobody <webkit-unassigned>|
|Severity:||Normal||CC:||ademar, benjamin, hausmann, kling, menard, webkit.review.bot|
|Version:||528+ (Nightly build)|
Description Dawit A. 2011-06-28 16:28:03 PDT
The fix applied to workaround the QNAM limitation in bug 49650 breaks custom QNAM implementations, namely kdewebkit's, that rely on the unsupportedContent signal being invoked as soon as possible instead of being queued. kdewebkit requires this functionality in order to take advantage of KIO's put-slave-on-hold functionality which allows two different applications to share the same network resource. That way a single resource will not be downloaded multiple times: once when the user clicks on the link and a second time when the appropriate application attempts to display the resource.
Comment 1 Dawit A. 2011-06-28 16:37:48 PDT
Created attachment 98998 [details] proposed patchgit diff The proposed patch uses Qt's Meta Object system, namely a dynamic property, to address this issue. If the dynamic property "SKIP_QTBUG_18718_WORKAROUND" is set, the unsupportedContent signal will be invoked directly. Otherwise, the signal connection will be queued until control returns to the event loop.
Comment 2 WebKit Review Bot 2011-06-28 16:38:55 PDT
Attachment 98998 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/WebCoreSupport/FrameLoade..." exit_code: 1 Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1699: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators]  Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Andreas Kling 2011-06-29 08:27:36 PDT
Comment on attachment 99001 [details] proporsed patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=99001&action=review > Source/WebKit/qt/ChangeLog:5 > + [Qt] Regression: Fix for 49650 breaks custom QNAM implementations... Please end your sentences with one period, not three. > Source/WebKit/qt/ChangeLog:8 > + This is an improvement of the regression patch committed to address bug 49650. This adds no information. > Source/WebKit/qt/ChangeLog:9 > + It allows for immediate emition of the QWebPage::unsupportedContent signal emition -> emission > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:215 > + qRegisterMetaType<QNetworkReply*>("QNetworkRepyPtr"); Typo, should be "QNetworkReplyPtr". Is this the right place to register meta types? Note that this code may execute more than once. > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:242 > + // NOTE: This should be a signal-to-signal connection were it not for bugs. should -> would > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1699 > + if (page->property("SKIP_QTBUG_18718_WORKAROUND").toBool() I don't like the name of this property. Can we make it something that describes what it does instead? _q_emitUnsupportedContentDirectly perhaps?
Comment 5 Dawit A. 2011-06-29 10:07:11 PDT
(In reply to comment #4) > > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:215 > > + qRegisterMetaType<QNetworkReply*>("QNetworkRepyPtr"); > > Typo, should be "QNetworkReplyPtr". > Is this the right place to register meta types? > Note that this code may execute more than once. I do not think you need to register the meta-type each time you emit the queued signal. At least that is how I have been using that macro all these years, but I will check it again.
Comment 7 Kenneth Rohde Christiansen 2011-06-29 11:11:13 PDT
Comment on attachment 99110 [details] proposed patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=99110&action=review > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1698 > + // A dynamic Qt property, _q_emitUnsupportedContentDirectly, is used here to prevent breakage > + // of custom QNAM implementations that assume this signal would not be queued, e.g kdewebkit. Why not modify kdewebkit?
Comment 8 Dawit A. 2011-06-29 12:07:09 PDT
(In reply to comment #7) > (From update of attachment 99110 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=99110&action=review > > > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1698 > > + // A dynamic Qt property, _q_emitUnsupportedContentDirectly, is used here to prevent breakage > > + // of custom QNAM implementations that assume this signal would not be queued, e.g kdewebkit. > > Why not modify kdewebkit? How ? kdewebkit needs the notification as soon as possible and the workaround completely prevents that from happening. The fact that the notification is needed immediately is not a simple choice in kdewebkit. It is a requirement enforced on it by the design of KIO. The way KIO 's put-slave-on-hold feature is designed, it requires the client to put the ioslave on hold as soon as it informs it of the mime-type if the client cannot handle that content-type. Otherwise, the "put-slave-on-hold" functionality will not work and the same resource would end up being downloaded at least twice: once for the original the request and the second when the application is launched to handle the request attempts to retrieve it. That won't happen if the slave is put on hold because the request the lanuched application make would be assigned to use the already parked (put-on-hold) ioslave. Anyhow, if the unsupported content signal is queued, the slot that handles this won't receive the notification on time to pause the ioslave. The ioslave would already been in the process of downloading the request. That not only causes the issue mentioned above, duplicate download of same resource, but since the call to publish the ioslave for reuse is a sync call, it causes a freeze in the application until the ioslave can interrupt itself from downloading the content to respond to our request.
Comment 9 Ademar Reis 2011-07-13 09:51:34 PDT
Any news regarding this problem? Anyone planning to look at it?
Comment 10 Andreas Kling 2011-07-13 12:07:46 PDT
(In reply to comment #9) > Any news regarding this problem? Anyone planning to look at it? I think we should roll out the original workaround making the emission queued, as that can be worked around in user code much more easily.
Comment 11 Dawit A. 2011-07-14 04:56:51 PDT
(In reply to comment #10) > (In reply to comment #9) > > Any news regarding this problem? Anyone planning to look at it? > > I think we should roll out the original workaround making the emission queued, as that can be worked around in user code much more easily. Yes, please. Developers connecting to this signal can use Qt::QueuedConnection, if they need to use anything that creates its own local event loop.
Comment 12 Andreas Kling 2011-07-25 07:42:31 PDT
Comment on attachment 99110 [details] proposed patch v3 Let's roll out the previous workaround instead.
Comment 13 Ademar Reis 2011-07-28 11:55:09 PDT
(In reply to comment #12) > (From update of attachment 99110 [details]) > Let's roll out the previous workaround instead. How do we go from here? Should we reopen the original bug or open a new one and attach the patch with the rollout?
Comment 14 Dawit A. 2011-07-31 18:52:04 PDT
Created attachment 102471 [details] proposed patch Here is a patch that rolls back out the source of this regression.
Comment 15 Alexis Menard (darktears) 2011-08-01 10:22:11 PDT
(In reply to comment #14) > Created an attachment (id=102471) [details] > proposed patch > > Here is a patch that rolls back out the source of this regression. LGTM. Though the original bug should be re-open if there was any.
Comment 16 Ademar Reis 2011-08-01 11:05:43 PDT
(In reply to comment #15) > (In reply to comment #14) > > Created an attachment (id=102471) [details] [details] > > proposed patch > > > > Here is a patch that rolls back out the source of this regression. > > LGTM. Though the original bug should be re-open if there was any. Yep, that's bug 49650 (which is also a regression itself). I guess it should be reopen once we land this patch (assuming we still have the problem).
Comment 17 Andreas Kling 2011-08-02 04:45:35 PDT
Comment on attachment 102471 [details] proposed patch r=me indeed. Please re-open the original bug when landing, as mentioned.
Comment 18 Ademar Reis 2011-08-05 06:21:50 PDT
(In reply to comment #17) > (From update of attachment 102471 [details]) > r=me indeed. > Please re-open the original bug when landing, as mentioned. Dawit, anything else to change in the patch? You have to cq+ it so that it lands in the repository (or you can cq? if you're not a commiter). I would like to include it on this 2.2 weekly release so that it gets proper test exposure.
Comment 19 WebKit Review Bot 2011-08-05 07:17:36 PDT
Comment on attachment 102471 [details] proposed patch Clearing flags on attachment: 102471 Committed r92479: <http://trac.webkit.org/changeset/92479>
Comment 20 WebKit Review Bot 2011-08-05 07:17:41 PDT
All reviewed patches have been landed. Closing bug.
Comment 21 Ademar Reis 2011-08-05 12:48:13 PDT
Revision r92479 cherry-picked into qtwebkit-2.2 with commit 18d52e6 <http://gitorious.org/webkit/qtwebkit/commit/18d52e6>