[webkit-reviews] review denied: [Bug 63235] [Qt] Fix tst_QWebFrame::setHtmlWithResource() API test : [Attachment 99400] Specified baseUrl to be a local file as we must get a security origin with granted permission to request local resources.

Mon Jul 4 03:21:36 PDT 2011

Benjamin Poulain <benjamin at webkit.org> has denied Rafael Brandao
<rafael.lobo at openbossa.org>'s request for review:
Bug 63235: [Qt] Fix tst_QWebFrame::setHtmlWithResource() API test
https://bugs.webkit.org/show_bug.cgi?id=63235

Attachment 99400: Specified baseUrl to be a local file as we must get a
security origin with granted permission to request local resources.
https://bugs.webkit.org/attachment.cgi?id=99400&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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"