Bug 63235

Summary:

[Qt] Fix tst_QWebFrame::setHtmlWithResource() API test

Product:

WebKit

Reporter:

Csaba Osztrogonác <ossy>

Component:

WebKit Qt

Assignee:

Rafael Brandao <rafael.lobo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ademar, ossy, rafael.lobo, webkit.review.bot

Priority:

Keywords:

Qt, QtTriaged

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

38654

Attachments:

Description	Flags
Specified baseUrl to be a local file as we must get a security origin with granted permission to request local resources.	benjamin: review-
modified style as suggested, and also have split the test into two (one for image, and other for stylesheet)	benjamin: review+, benjamin: commit-queue-
minor changes on the last one	none

Csaba Osztrogonác

Reported 2011-06-23 01:53:43 PDT

http://build.webkit.org/builders/Qt%20Linux%20Release/builds/34519 FAIL! : tst_QWebFrame::setHtmlWithResource() Compared values are not the same Actual (frame->evaluateJavaScript("document.images[0].width").toInt()): 0 Expected (128): 128 Loc: [/ramdisk/qt-linux-release/build/Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp(2506)]

Attachments
Specified baseUrl to be a local file as we must get a security origin with granted permission to request local resources. (4.21 KB, patch) 2011-06-30 17:04 PDT, Rafael Brandao	benjamin: review-	Details Formatted Diff Diff
modified style as suggested, and also have split the test into two (one for image, and other for stylesheet) (5.41 KB, patch) 2011-07-05 10:55 PDT, Rafael Brandao	benjamin: review+ benjamin: commit-queue-	Details Formatted Diff Diff
minor changes on the last one (5.39 KB, patch) 2011-07-05 11:46 PDT, Rafael Brandao	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Csaba Osztrogonác

Comment 1 2011-06-23 05:51:17 PDT

Two more fails: FAIL! : tst_QWebFrame::setHtmlWithResource() Compared values are not the same Actual (frame->evaluateJavaScript("document.images[0].height").toInt()): 0 Expected (128): 128 Loc: [/home/oszi/WebKit/Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp(2508)] FAIL! : tst_QWebFrame::setHtmlWithResource() Compared values are not the same Actual (p.styleProperty("color", QWebElement::CascadedStyle)): Expected (QLatin1String("red")): red Loc: [/home/oszi/WebKit/Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp(2527)] Marked as expected fails: http://trac.webkit.org/changeset/89561

Rafael Brandao

Comment 2 2011-06-29 13:11:56 PDT

I've tried to explain how is the issue that is causing this on https://bugs.webkit.org/show_bug.cgi?id=63236 (that test is also failing because of the same thing). To be quick, it's not loading local resources when you specify the susbtitute data for a page. When I say resources, I mean, for example, an image. If you use "file" or "qrc", it won't download the image, just because you've loaded this html replacing its substitute data. Still investigating why this is happening.

Rafael Brandao

Comment 3 2011-06-30 17:04:10 PDT

Created attachment 99400 [details] Specified baseUrl to be a local file as we must get a security origin with granted permission to request local resources. Also added the expected behavior for the opposite situation. I've removed QSignalSpy because it would always increment its counter, no matter what happens related to the local resources, so it looks like it was pointless to check it there. I've also added a comment to the test setHtmlWithBaseURL to explain the difference between this test and the other one. Perhaps we should rename them? Or is this unrelated to this patch and I should remove it? I don't know, I think it's nice to explain why they are different and what they're actually testing. We've discussed the use of baseUrl to fix those tests at https://bugs.webkit.org/show_bug.cgi?id=63236. To make it short, it looks like we've been using setHtml in the wrong way. Feedbacks are welcome! :)

WebKit Review Bot

Comment 4 2011-06-30 17:07:46 PDT

Attachment 99400 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/ChangeLog', u'Source/WebK..." exit_code: 1 Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2501: Missing space after , [whitespace/comma] [3] Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2518: Missing space after , [whitespace/comma] [3] Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2531: Missing space after , [whitespace/comma] [3] Total errors found: 3 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Benjamin Poulain

Comment 5 2011-07-04 03:21:36 PDT

Comment on attachment 99400 [details] Specified baseUrl to be a local file as we must get a security origin with granted permission to request local resources. View in context: https://bugs.webkit.org/attachment.cgi?id=99400&action=review The change make sense. Just a few comments on the style... This test is doing 2 things: image from setHtml <-> doc properties from setHTML. You should split it, the two tests with image together. The two tests using html2 in a new function. > Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2496 > + QString html("<html><body><p>hello world</p><img src='qrc:/image.png'/></body></html>"); QLatin1String. (I know it is not widespread and not enforced in tests at the moment.) > Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2499 > + QWebElement webElement; This variable is declared a bit too far for its first use. You should declare next to the second part of the test. >> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2501 >> + frame->setHtml(html,QUrl("file:///path/to/file")); > > Missing space after , [whitespace/comma] [3] Style + QLatin1String. >> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2518 >> + frame->setHtml(html2,QUrl("qrc:/path/to/file")); > > Missing space after , [whitespace/comma] [3] QLatin1String Why do you need a path here? > Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2520 > + webElement = frame->documentElement().findAll("p").at(0); QLatin1String And you can replace findAll().at(0) by QWebElement::findFirst(). > Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2533 > + webElement = frame->documentElement().findAll("p").at(0); findFirst() > Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2541 > + // This will test if baseUrl is indeed affecting the relative paths from resources. > + // As we're using a local file as baseUrl, its security origin will be able to load local resources. "This tests", the future form here is a bit strange :) "As we are" "its security origin should be able"

Rafael Brandao

Comment 6 2011-07-05 10:55:38 PDT

Created attachment 99726 [details] modified style as suggested, and also have split the test into two (one for image, and other for stylesheet) I'm not sure if the new names are ideal, but I'm not good with it anyway. They're essentially the same test, just the way it checks is different, but I like the idea to separate them into two, it made the code more clear. Feedbacks are welcome. :)

Benjamin Poulain

Comment 7 2011-07-05 11:02:10 PDT

Comment on attachment 99726 [details] modified style as suggested, and also have split the test into two (one for image, and other for stylesheet) View in context: https://bugs.webkit.org/attachment.cgi?id=99726&action=review Looks good. > Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2531 > + QLatin1String html = QLatin1String(htmlData); QLatin1String html(htmlData); > Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2536 > + frame->setHtml(html, QUrl(QLatin1String("qrc:file"))); qrc:file is not a valid qrc url (maybe it is at the moment but that is not very robust). Better use qrc:///file (I can't remember if that should be 2 or 3 / :)) > Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2546 > + QCOMPARE(webElement.styleProperty("color", QWebElement::CascadedStyle), QLatin1String("")); QLatin1String("") -> QString() I guess

Rafael Brandao

Comment 8 2011-07-05 11:46:04 PDT

Created attachment 99733 [details] minor changes on the last one

WebKit Review Bot

Comment 9 2011-07-05 12:45:12 PDT

Comment on attachment 99733 [details] minor changes on the last one Clearing flags on attachment: 99733 Committed r90403: <http://trac.webkit.org/changeset/90403>

WebKit Review Bot

Comment 10 2011-07-05 12:45:18 PDT

All reviewed patches have been landed. Closing bug.

Ademar Reis

Comment 11 2011-07-05 13:53:21 PDT

Revision r90403 cherry-picked into qtwebkit-2.2 with commit dfe73c3 <http://gitorious.org/webkit/qtwebkit/commit/dfe73c3>

Note You need to log in before you can comment on or make changes to this bug.