Bug 63235 - [Qt] Fix tst_QWebFrame::setHtmlWithResource() API test
Summary: [Qt] Fix tst_QWebFrame::setHtmlWithResource() API test
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Rafael Brandao
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 38654
  Show dependency treegraph
 
Reported: 2011-06-23 01:53 PDT by Csaba Osztrogonác
Modified: 2011-07-05 13:53 PDT (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 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)]
Comment 1 Csaba Osztrogonác 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
Comment 2 Rafael Brandao 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.
Comment 3 Rafael Brandao 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! :)
Comment 4 WebKit Review Bot 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.
Comment 5 Benjamin Poulain 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"
Comment 6 Rafael Brandao 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. :)
Comment 7 Benjamin Poulain 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
Comment 8 Rafael Brandao 2011-07-05 11:46:04 PDT
Created attachment 99733 [details]
minor changes on the last one
Comment 9 WebKit Review Bot 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>
Comment 10 WebKit Review Bot 2011-07-05 12:45:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Ademar Reis 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>