Created attachment 125932 [details] Patch

Add Antonio.

Comment on attachment 125932 [details] Patch 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?

Created attachment 126096 [details] Patch

Comment on attachment 126096 [details] Patch Clearing flags on attachment: 126096 Committed r107105: <http://trac.webkit.org/changeset/107105>

All reviewed patches have been landed. Closing bug.

Reopening to attach new patch.

Created attachment 126121 [details] Patch

Comment on attachment 126121 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126121&action=review > Tools/DumpRenderTree/blackberry/EventSender.cpp:325 > + for (Vector<BlackBerry::Platform::TouchPoint>::iterator it = touches.begin(); > + it != touches.end(); > + ++it) { bad identation > Tools/DumpRenderTree/blackberry/WorkQueueItemBlackBerry.cpp:87 > + return BlackBerry::WebKit::DumpRenderTree::currentInstance()->page()->goBackOrForward(m_howFar); as a follow up we could rename ::currentInstance to ::instance maybe.

Created attachment 126153 [details] Patch

Comment on attachment 126153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126153&action=review > Tools/DumpRenderTree/blackberry/LayoutTestControllerBlackBerry.cpp:266 > +void LayoutTestController::setPrivateBrowsingEnabled(bool flag) > +{ > + UNUSED_PARAM(flag); > + notImplemented(); > +} I think we would support this one in a followup :) > Tools/DumpRenderTree/blackberry/LayoutTestControllerBlackBerry.cpp:470 > + if (keyStr == "WebKitUsesPageCachePreferenceKey") > + BlackBerry::WebKit::DumpRenderTree::currentInstance()->page()->settings()->setMaximumPagesInCache(1); > + else if (keyStr == "WebKitUsePreHTML5ParserQuirks") > + mainFrame->page()->settings()->setUsePreHTML5ParserQuirks(true); > + else if (keyStr == "WebKitTabToLinksPreferenceKey") > + DumpRenderTreeSupport::setLinksIncludedInFocusChain(valueStr == "true" || valueStr == "1"); > + else if (keyStr == "WebKitHyperlinkAuditingEnabled") > + mainFrame->page()->settings()->setHyperlinkAuditingEnabled(valueStr == "true" || valueStr == "1"); we could also support the preference needed for spatial navigation tests here too I think > Tools/DumpRenderTree/blackberry/LayoutTestControllerBlackBerry.cpp:631 > + if (!mainFrame) > + return; > + mainFrame->page()->settings()->setAllowFileAccessFromFileURLs(enabled); newline here for consistency > Tools/DumpRenderTree/blackberry/LayoutTestControllerBlackBerry.cpp:638 > +void LayoutTestController::setAllowUniversalAccessFromFileURLs(bool enabled) > +{ > + if (!mainFrame) > + return; > + mainFrame->page()->settings()->setAllowUniversalAccessFromFileURLs(enabled); ditto

Final part landed as r107247, closing.