[BlackBerry] Upstream DumpRenderTreeBlackBerry

[Rob Buis]

Upstream part of our DRT implementation.

[Rob Buis]

Created attachment 125932 [details]
Patch

[Rob Buis]

Add Antonio.

[Antonio Gomes]

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?

[Rob Buis]

Created attachment 126096 [details]
Patch

[WebKit Review Bot]

Comment on attachment 126096 [details]
Patch

Clearing flags on attachment: 126096

Committed r107105: <http://trac.webkit.org/changeset/107105>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Rob Buis]

Reopening to attach new patch.

[Rob Buis]

Created attachment 126121 [details]
Patch

[Antonio Gomes]

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.

[Rob Buis]

Created attachment 126153 [details]
Patch

[Antonio Gomes]

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

[Rob Buis]

Final part landed as r107247, closing.