[webkit-reviews] review granted: [Bug 219001] Allow WKContentRuleList to block only in frames or only in main frame : [Attachment 424204] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 219001: Allow WKContentRuleList to block only in frames or only in main
frame
https://bugs.webkit.org/show_bug.cgi?id=219001

Attachment 424204: Patch

https://bugs.webkit.org/attachment.cgi?id=424204&action=review




--- Comment #10 from Benjamin Poulain <benjamin at webkit.org> ---
Comment on attachment 424204
  --> https://bugs.webkit.org/attachment.cgi?id=424204
Patch

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

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:271
> +    ResourceLoadInfo resourceLoadInfo = { url, mainDocumentURL, {
ResourceType::Ping, ResourceType::Other } };

Why not add ping to the "other" resource type in readResourceType?
Wouldn't this be more consistent with the other loaded resources?

> Source/WebCore/loader/PingLoader.cpp:125
> +    if (processContentRuleListsForLoad(frame, request, {
ContentExtensions::ResourceType::Other, ContentExtensions::ResourceType::Ping
}))

Ditto here regarding ping flags.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentRuleListNotification.mm:307
> +    auto handler = [[TestURLSchemeHandler new] autorelease];

I hope TEST has an autorelease pool :-D

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentRuleListNotification.mm:373
> +    [userContentController
addContentRuleList:listWithLoadContext("child-frame").get()];
> +    [webView reload];
> +    EXPECT_WK_STREQ([webView _test_waitForAlert], "main frame fetched
successfully");
> +    EXPECT_WK_STREQ([webView _test_waitForAlert], "iframe fetch failed");
> +
> +    [userContentController removeAllContentRuleLists];
> +    [userContentController
addContentRuleList:listWithLoadContext("top-frame").get()];
> +    [webView reload];
> +    EXPECT_WK_STREQ([webView _test_waitForAlert], "main frame fetch
failed");
> +    EXPECT_WK_STREQ([webView _test_waitForAlert], "iframe fetched
successfully");

I like those tests!