Bug 42330 – WebKitTestRunner needs layoutTestController.waitForPolicyDelegate

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Maciej Stachowiak]

WebKitTestRunner needs layoutTestController.waitForPolicyDelegate

[Maciej Stachowiak]

<rdar://problem/8193639>

[Fehér Zsolt]

Created attachment 102512 [details]
patch

[Fehér Zsolt]

Created attachment 102629 [details]
patch

[Andras Becsi]

Comment on attachment 102629 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102629&action=review

Informal review. I think this patch needs another iteration.

> LayoutTests/platform/wk2/Skipped:1612
> +# Unexplained failures
> +fast/encoding/mailto-always-utf-8.html
> +

Unexplained failure doesn't say much. Any idea why this still fails? This would need further investigation and a bug report.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:751
> +        if (InjectedBundle::shared().layoutTestController()->waitToDump()) {

This check seems redundant here since LayoutTestController::waitForPolicyDelegate() calls waitUntilDone(), so this is always true. Or is there a test which calls layoutTestController.setCustomPolicyDelegate(true) but does not call waitUntilDone()?

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.h:50
> +    static bool policyDelegateEnabled;
> +    static bool policyDelegatePermissive;

Why are these in InjectedBundle? Couldn't they be just simple members of LayoutTestController, they are never used elsewhere.

> Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.cpp:102
> +    , m_waitForPolicy(false)

This variable is only set, but never used, I think you can simply remove it.

> Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.cpp:132
> +void LayoutTestController::setCustomPolicyDelegate(bool enabled, bool permissive)

You implement this function, which seems to be needed, but don't expose it through the IDL file. There seems to be a separate issue for this function tracked in https://bugs.webkit.org/show_bug.cgi?id=42546 which has it's own skipped tests. You might need to check these tests for the above waitUntilDone concern of mine too. See LayoutTests/platform/wk2/Skipped:1284.

I think this bug should depend on 42546, and we should implement this function for all platforms which miss it.

> Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.h:150
> +    bool waitForPolicy() const { return m_waitForPolicy; }

This function is never used.

> Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.h:190
> +    bool m_waitForPolicy;

Never used, see above.

[Csaba Osztrogonác]

Comment on attachment 102629 [details]
patch

r- due to Andras' informal review.

[Fehér Zsolt]

Created attachment 103249 [details]
Add missing features (waitForPolicyDelegate) to TestRunner

[Andras Becsi]

Comment on attachment 103249 [details]
Add missing features (waitForPolicyDelegate) to TestRunner

View in context: https://bugs.webkit.org/attachment.cgi?id=103249&action=review

Seems ok now, just a few comments yet:

> Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.cpp:134
> +void LayoutTestController::setCustomPolicyDelegate(bool enabled, bool permissive)

This at least needs a comment, like "FIXME: Needs a full implementation see https://bugs.webkit.org/show_bug.cgi?id=42546"

> Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.h:54
> +    static bool policyDelegateEnabled;
> +    static bool policyDelegatePermissive;

These do not need to be static, do they? They could simply be private members, with inline getter methods.

[Fehér Zsolt]

Created attachment 103483 [details]
Add missing features (waitForPolicyDelegate) to TestRunner

[Fehér Zsolt]

Created attachment 103486 [details]
Add missing features (waitForPolicyDelegate) to TestRunner

[Balazs Kelemen]

*** Bug 42546 has been marked as a duplicate of this bug. ***

[Andras Becsi]

Comment on attachment 103486 [details]
Add missing features (waitForPolicyDelegate) to TestRunner

The patch looks good to me now. Thanks for addressing my suggestions.

[Csaba Osztrogonác]

Comment on attachment 103486 [details]
Add missing features (waitForPolicyDelegate) to TestRunner

r=me

[Csaba Osztrogonác]

Landed in https://trac.webkit.org/changeset/92839