Bug 52614 – [Qt] Fix LoadHTMLStringItem::invoke() after r75966

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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]

Created attachment 79783 [details]
Patch

[Robert Hogan]

ping!

[Jessie Berlin]

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 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]

Created attachment 87176 [details]
Patch

[Robert Hogan]

(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 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 on attachment 87176 [details]
Patch

Clearing flags on attachment: 87176

Committed r82489: <http://trac.webkit.org/changeset/82489>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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]

(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.