Add WKContentRuleList warn action type
Created attachment 320936 [details] Patch
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.
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).
Created attachment 321507 [details] Patch
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 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.
Created attachment 321705 [details] Patch
Created attachment 321711 [details] Patch
Created attachment 322067 [details] Patch
http://trac.webkit.org/r222602
<rdar://problem/34706927>