|Summary:||[Qt] Fix tst_QWebFrame::setHtmlWithResource() API test|
|Product:||WebKit||Reporter:||Csaba Osztrogonác <ossy>|
|Component:||WebKit Qt||Assignee:||Rafael Brandao <rafael.lobo>|
|Severity:||Normal||CC:||ademar, ossy, rafael.lobo, webkit.review.bot|
|Version:||528+ (Nightly build)|
|Bug Depends on:|
Description Csaba Osztrogonác 2011-06-23 01:53:43 PDT
Comment 1 Csaba Osztrogonác 2011-06-23 05:51:17 PDT
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]  Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2518: Missing space after , [whitespace/comma]  Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2531: Missing space after , [whitespace/comma]  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]  Style + QLatin1String. >> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2518 >> + frame->setHtml(html2,QUrl("qrc:/path/to/file")); > > Missing space after , [whitespace/comma]  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>