Bug 78042 - [BlackBerry] Upstream DumpRenderTreeBlackBerry
Summary: [BlackBerry] Upstream DumpRenderTreeBlackBerry
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords:
Depends on:
Blocks: 73144
  Show dependency treegraph
 
Reported: 2012-02-07 14:25 PST by Rob Buis
Modified: 2012-02-09 08:39 PST (History)
2 users (show)

See Also:


Attachments
Patch (40.50 KB, patch)
2012-02-07 14:47 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (40.65 KB, patch)
2012-02-08 09:07 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (40.13 KB, patch)
2012-02-08 11:08 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (42.20 KB, patch)
2012-02-08 14:29 PST, Rob Buis
tonikitoo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2012-02-07 14:25:10 PST
Upstream part of our DRT implementation.
Comment 1 Rob Buis 2012-02-07 14:47:17 PST
Created attachment 125932 [details]
Patch
Comment 2 Rob Buis 2012-02-07 14:51:33 PST
Add Antonio.
Comment 3 Antonio Gomes 2012-02-08 08:08:26 PST
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?
Comment 4 Rob Buis 2012-02-08 09:07:19 PST
Created attachment 126096 [details]
Patch
Comment 5 WebKit Review Bot 2012-02-08 10:36:06 PST
Comment on attachment 126096 [details]
Patch

Clearing flags on attachment: 126096

Committed r107105: <http://trac.webkit.org/changeset/107105>
Comment 6 WebKit Review Bot 2012-02-08 10:36:10 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Rob Buis 2012-02-08 11:08:02 PST
Reopening to attach new patch.
Comment 8 Rob Buis 2012-02-08 11:08:04 PST
Created attachment 126121 [details]
Patch
Comment 9 Antonio Gomes 2012-02-08 12:10:24 PST
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.
Comment 10 Rob Buis 2012-02-08 14:29:55 PST
Created attachment 126153 [details]
Patch
Comment 11 Antonio Gomes 2012-02-09 07:51:22 PST
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
Comment 12 Rob Buis 2012-02-09 08:39:16 PST
Final part landed as r107247, closing.