Bug 42330

<rdar://problem/8193639>

Created attachment 102512 [details] patch

Created attachment 102629 [details] patch

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.

Comment on attachment 102629 [details] patch r- due to Andras' informal review.

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

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.

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

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

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

Comment on attachment 103486 [details] Add missing features (waitForPolicyDelegate) to TestRunner The patch looks good to me now. Thanks for addressing my suggestions.

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

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