[webkit-reviews] review granted: [Bug 78042] [BlackBerry] Upstream DumpRenderTreeBlackBerry : [Attachment 125932] Patch

[bugzilla-daemon at webkit.org]

Antonio Gomes <tonikitoo at webkit.org> has granted Rob Buis <rwlbuis at gmail.com>'s
request for review:
Bug 78042: [BlackBerry] Upstream DumpRenderTreeBlackBerry
https://bugs.webkit.org/show_bug.cgi?id=78042

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

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=125932&action=review


please address nits before commiting..

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:69
> +volatile bool done;

can we rename it to indicate the global nature.

g_done?  dDone? also  "done" is a very bad name.

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:195
> +    (m_page->client())->notifyRunLayoutTestsFinished();

unneeded wrapping (...) ?

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:213
> +    if (m_currentTest < m_tests.end() - 1) {
> +	   m_currentTest++;
> +	   if (isHTTPTest(m_currentTest->utf8().data())) {
> +	       m_currentHttpTest = m_currentTest->utf8().data();
> +	       m_currentHttpTest.remove(0, strlen(httpTestSyntax));
> +	       runTest(httpPrefixURL + m_currentHttpTest);
> +	   } else
> +	       runTest(kSDCLayoutTestsURI + *m_currentTest);
> +    } else
> +	   doneDrt();

can we early return here?

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:572
> +    if (!done && gLayoutTestController->dumpFrameLoadCallbacks())
> +	   printf("%s - didFinishDocumentLoadForFrame\n",
drtFrameDescription(frame).utf8().data());
> +    else if (!done) {

extract !done to an outer if?