[webkit-reviews] review granted: [Bug 37319] [Qt] tst_QWebFrame::overloadedSlots() fails : [Attachment 96633] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has granted Caio Marcelo de Oliveira Filho
<cmarcelo at webkit.org>'s request for review:
Bug 37319: [Qt] tst_QWebFrame::overloadedSlots() fails
https://bugs.webkit.org/show_bug.cgi?id=37319

Attachment 96633: Patch
https://bugs.webkit.org/attachment.cgi?id=96633&action=review

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=96633&action=review

r=me as is, but please consider making the following adjustments before
landing:

> Source/WebKit/qt/ChangeLog:23
> +	   (tst_QWebFrame::documentHasDocumentElement): Evaluate 'document' and
extracts

Evaluate -> Evaluates

> Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:985
> -    WebCore::Element* webElement = document.m_element;
> +    QWebElement documentElement =
document.value(QLatin1String("documentElement")).value<QWebElement>();

We could ASSERT that the QVariant returned by document.value() here is valid.

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2313
> +    QVariant v = m_page->mainFrame()->evaluateJavaScript("document");

This variable is used for a bit of this and that, the function would be more
readable if you used separate variables with clearer names.