RESOLVED FIXED 52614
[Qt] Fix LoadHTMLStringItem::invoke() after r75966
https://bugs.webkit.org/show_bug.cgi?id=52614
Summary [Qt] Fix LoadHTMLStringItem::invoke() after r75966
Csaba Osztrogonác
Reported 2011-01-18 00:40:43 PST
http/tests/navigation/go-back-to-error-page.html introduced in r75966, and fails because of missing fix for LoadHTMLStringItem::invoke() of Qt DRT: http://build.webkit.org/results/Qt%20Linux%20Release/r75966%20(26617)/http/tests/navigation/go-back-to-error-page-pretty-diff.html
Attachments
Patch (8.83 KB, patch)
2011-01-21 13:40 PST, Robert Hogan
no flags
Patch (14.28 KB, patch)
2011-03-28 12:00 PDT, Robert Hogan
no flags
Csaba Osztrogonác
Comment 1 2011-01-18 00:44:46 PST
I added http/tests/navigation/go-back-to-error-page.html to the Skipped list until fix: http://trac.webkit.org/changeset/76003
Robert Hogan
Comment 2 2011-01-21 13:40:22 PST
Robert Hogan
Comment 3 2011-03-27 05:49:39 PDT
ping!
Jessie Berlin
Comment 4 2011-03-27 08:12:06 PDT
Comment on attachment 79783 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79783&action=review I am not a reviewer, and you should probably get somebody a little more familiar with the Qt platform to review it, but this overall looks good to me! > Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:889 > +// API Candidate? What does this comment mean? API in what sense? (Is there a separate API for DRT?) > Tools/DumpRenderTree/qt/WorkQueueItemQt.cpp:31 > +#include "../../../Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.h" I am not familiar with the Qt port style, but what happens if this file gets moved into some sub-directory, etc? Do all such paths have to be updated? Is there no way to set it up so that you can just include "DumpRenderTreeSupportQt.h" and add this path to the include paths? > Tools/DumpRenderTree/qt/WorkQueueItemQt.h:85 > + LoadAlternateHTMLStringItem(const QString& content, const QString &baseURL, const QString &failingURL, QWebPage *page) Extra space here between "&baseURL," and "const QString"
Antonio Gomes
Comment 5 2011-03-27 21:59:21 PDT
Comment on attachment 79783 [details] Patch You say nothing about what is the problem in ChangeLog. Please fix that before landing. Also it is not so easy to review without knowing the problem, what caused it, and how you want to fix. Does it all have to do with ErrorPageExtension in QWebPage? This is how error pages are to be handled ..
Robert Hogan
Comment 6 2011-03-28 12:00:20 PDT
Robert Hogan
Comment 7 2011-03-28 12:02:18 PDT
(In reply to comment #4) > > What does this comment mean? API in what sense? (Is there a separate API for DRT?) Yes, DRTSupportQt just provides a private API to DumpRenderTree. It avoids the need to add public Qt API in order to just pass tests. I think I've addressed your other comment's and Antonio's in the patch.
WebKit Commit Bot
Comment 8 2011-03-30 13:17:31 PDT
Comment on attachment 87176 [details] Patch Rejecting attachment 87176 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'apply-..." exit_code: 2 Last 500 characters of output: ded at 98 with fuzz 1 (offset 18 lines). patching file Tools/QtTestBrowser/QtTestBrowser.pro Hunk #1 FAILED at 26. 1 out of 1 hunk FAILED -- saving rejects to file Tools/QtTestBrowser/QtTestBrowser.pro.rej patching file Tools/QtTestBrowser/launcherwindow.h Hunk #1 FAILED at 61. 1 out of 1 hunk FAILED -- saving rejects to file Tools/QtTestBrowser/launcherwindow.h.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Antonio Gomes', u'--fo..." exit_code: 1 Full output: http://queues.webkit.org/results/8285955
WebKit Commit Bot
Comment 9 2011-03-30 13:23:34 PDT
Comment on attachment 87176 [details] Patch Clearing flags on attachment: 87176 Committed r82489: <http://trac.webkit.org/changeset/82489>
WebKit Commit Bot
Comment 10 2011-03-30 13:23:38 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 11 2011-03-30 14:41:19 PDT
http://trac.webkit.org/changeset/82489 might have broken Qt Linux Release The following tests are not passing: platform/qt/plugins/qt-qwidget-plugin.html plugins/application-plugin-plugins-disabled.html
Robert Hogan
Comment 12 2011-03-30 15:16:10 PDT
(In reply to comment #11) > http://trac.webkit.org/changeset/82489 might have broken Qt Linux Release > The following tests are not passing: > platform/qt/plugins/qt-qwidget-plugin.html > plugins/application-plugin-plugins-disabled.html No, it's http://trac.webkit.org/changeset/82240/trunk/Source/WebKit.pri that caused this.
Note You need to log in before you can comment on or make changes to this bug.