[Webkit-unassigned] [Bug 42330] WebKitTestRunner needs layoutTestController.waitForPolicyDelegate

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=42330





--- Comment #4 from Andras Becsi <abecsi at webkit.org>  2011-08-04 07:03:18 PST ---
(From update of attachment 102629)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.