Bug 177013 - Add WKContentRuleList notify action type
Summary: Add WKContentRuleList notify action type
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
Keywords: InRadar
Depends on: 177460
  Show dependency treegraph
Reported: 2017-09-15 10:32 PDT by Alex Christensen
Modified: 2017-09-27 22:45 PDT (History)
8 users (show)

See Also:

Patch (44.02 KB, patch)
2017-09-15 11:42 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (66.81 KB, patch)
2017-09-21 19:38 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (68.37 KB, patch)
2017-09-25 11:15 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (68.10 KB, patch)
2017-09-25 11:40 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (67.91 KB, patch)
2017-09-27 22:10 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2017-09-15 10:32:33 PDT
Add WKContentRuleList warn action type
Comment 1 Alex Christensen 2017-09-15 11:42:53 PDT
Created attachment 320936 [details]
Comment 2 Geoffrey Garen 2017-09-15 12:50:36 PDT
Comment on attachment 320936 [details]

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.
Comment 3 Sam Weinig 2017-09-16 21:48:48 PDT
Comment on attachment 320936 [details]

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).
Comment 4 Alex Christensen 2017-09-21 19:38:21 PDT
Created attachment 321507 [details]
Comment 5 Alex Christensen 2017-09-21 20:34:12 PDT
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?
Comment 6 Darin Adler 2017-09-23 17:49:18 PDT
Comment on attachment 321507 [details]

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)


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

No need for const here.
Comment 7 Alex Christensen 2017-09-25 11:15:46 PDT
Created attachment 321705 [details]
Comment 8 Alex Christensen 2017-09-25 11:40:58 PDT
Created attachment 321711 [details]
Comment 9 Alex Christensen 2017-09-27 22:10:29 PDT
Created attachment 322067 [details]
Comment 10 Alex Christensen 2017-09-27 22:43:55 PDT
Comment 11 Radar WebKit Bug Importer 2017-09-27 22:45:14 PDT