Bug 177013 – Add WKContentRuleList notify action type

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Add WKContentRuleList warn action type

[Alex Christensen]

Created attachment 320936 [details]
Patch

[Geoffrey Garen]

Comment on attachment 320936 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentRuleListWarning.mm:77
> +    [[WKContentRuleListStore defaultStore] compileContentRuleListForIdentifier:@"testidentifier" encodedContentRuleList:@"[{\"action\":{\"type\":\"warn\"},\"trigger\":{\"url-filter\":\"match\"}}]" completionHandler:^(WKContentRuleList *list, NSError *error) {

I think we need a more specific name than "warn", since Safari supports lots of different kinds of warnings, and warning services also provide lots of different kinds of warnings. Maybe "warn-phishing".

Same comment for the client-level API too.

[Sam Weinig]

Comment on attachment 320936 [details]
Patch

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

> Source/WebCore/contentextensions/ContentExtensionActions.h:49
> +    Warn,

Warn feels to prescriptive. If all it's doing is sending a message to the host, I think something like, "message-host" (I hate the word host, so maybe something better than that).

[Alex Christensen]

Created attachment 321507 [details]
Patch

[Alex Christensen]

I changed it to "notify" with a string notification argument.  Since cocoa and WK* don't have an analog to std::pair, I expose them to the NavigationClient as two arrays, one of the notification arguments, and one of the identifiers of the WKContentRuleList they came from.  What do you think?

[Darin Adler]

Comment on attachment 321507 [details]
Patch

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

> Source/WebCore/contentextensions/ContentExtensionActions.h:48
>      BlockLoad,
>      BlockCookies,
>      CSSDisplayNoneSelector,
> -    IgnorePreviousRules = 3,
> -    MakeHTTPS = 4,
> +    Notify,
> +    IgnorePreviousRules,
> +    MakeHTTPS,

These were in alphabetical order before. Maybe keep them that way instead of "almost alphabetical"?

> Source/WebCore/contentextensions/ContentExtensionActions.h:61
> +        break;

I suggest just return false here so we can ASSERT_NOT_REACHED outside the function.

> Source/WebCore/contentextensions/ContentExtensionActions.h:63
> +    return false;

Should have ASSERT_NOT_REACHED here.

> Source/WebCore/contentextensions/ContentExtensionActions.h:70
> +    HashSet<std::pair<String, String>> notifications;

Seems pretty unclear what each of these two strings are.

> Source/WebCore/contentextensions/ContentExtensionActions.h:73
> +WEBCORE_EXPORT void applyBlockedStatusToRequest(const BlockedStatus&, Page*, ResourceRequest&);

Why Page* instead of Page&?

> Source/WebCore/contentextensions/ContentExtensionCompiler.cpp:74
> +    Vector<uint32_t> clientLocations;

Unclear why Vector<uint32_t> is an improvement over Vector<unsigned> in a source file the also uses Vector<unsigned> for the return value of serializeActions, for example.

If "locations" are positions within a String, then "unsigned" is the type we have used for that. I don’t see a good reason to start using uint32_t for it here unless we have plans to make a transition.

> Source/WebCore/contentextensions/ContentExtensionCompiler.cpp:150
> +            const auto existingAction = map.find(std::make_pair(argument, flags));

The const here seems unnecessary.

> Source/WebCore/contentextensions/ContentExtensionError.h:62
>      JSONInvalidActionType,
>      JSONInvalidCSSDisplayNoneActionType,
>      JSONInvalidRegex,
> +    JSONInvalidNotification,

This was in alphabetical order before. But not any more.

> Source/WebCore/contentextensions/ContentExtensionParser.cpp:283
> +        if (!field || scope.exception() || !field.isString())
> +            return makeUnexpected(ContentExtensionError::JSONInvalidNotification);

No need for the "!field" check here. That can’t happen unless there is an exception.

Seems a little peculiar to use isString rather than toString. Not typical JavaScript idiom.

> Source/WebCore/contentextensions/ContentExtensionParser.cpp:284
> +        return { Action(ActionType::Notify, asString(field)->value(&exec)) };

Wouldn’t "{{" work instead of "{ Action(" ? As used above?

> Source/WebCore/page/ChromeClient.h:373
> +    virtual void contentRuleListNotification(const WebCore::URL&, const HashSet<std::pair<String, String>>&) { };

Is it OK that the rule list is a randomly ordered set? Is there no ordering needed?

> Source/WebKit/UIProcess/API/C/WKPage.cpp:2304
> +            for (const auto& identifier : listIdentifiers)

No need for const here.

> Source/WebKit/UIProcess/API/C/WKPage.cpp:2308
> +            for (const auto& notification : notifications)

Ditto.

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:883
> +    for (const auto& notification : notificationPairs) {

No need for const here.

[Alex Christensen]

Created attachment 321705 [details]
Patch

[Alex Christensen]

Created attachment 321711 [details]
Patch

[Alex Christensen]

Created attachment 322067 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r222602

[Radar WebKit Bug Importer]

<rdar://problem/34706927>