Bug 150479 – Expose public APIs for content filters

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brian Nicholson]

On Firefox for iOS, we're interested in integrating our tracking protection lists into WKWebView. Exposing the WKUserContentController, WKUserContentExtensionStore, and WKUserContentFilter private APIs was enough to get this working, so it would be great if these could be made public.

[Brian Nicholson]

Created attachment 264108 [details]
Patch

[Brian Nicholson]

Here's a first attempt that simply makes the necessary APIs for content filters (WKUserContentFilter, WKUserContentExtensionStore, and the content filter methods on WKUserContentController) public.

While this builds and launches fine in Safari using the instructions at https://www.webkit.org/blog/3457/building-webkit-for-ios-simulator/, I'm not sure if/how we can test these changes in a separate app.

[Brian Nicholson]

Created attachment 264149 [details]
Patch

[WebKit Commit Bot]

Attachment 264149 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:116:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/ContentExtensionTester/main.m:49:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 264149 [details]
Patch

Hi Brian,

Public APIs need to be discussed internally and go through API review. The private API you are exposing was never designed to be public, we may not want to expose Content Blockers with that particular API.

Did you create radar for your feature request?
The first thing will be to discuss internally with the team at Apple to know if anyone objects a public API for Content Blockers. If there are not objection, we can start discussing an API.

The reason exposing API is a PITA is the long term cost of exposing something new. Once we expose an API, we restrict the freedom we have to change the implementation. Sometimes it has hurt us badly, which is why we try to avoid mistakes.

[Brian Nicholson]

Thanks for the response! I've filed rdar://23284825. Hoping this starts some discussion since AFAIK, there are simply no options for filtering individual requests using WKWebView.

[Alex Christensen]

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

[Joel]

I totally agree, this needs further discussion. When blocking analytic trackers, it would cause applications to act funny. Some result in a crash. Future collaboration is gonna to take some work.

[Alex Christensen]

Created attachment 303869 [details]
Patch

[WebKit Commit Bot]

Attachment 303869 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKUserContentExtensionStore.mm:125:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKUserContentExtensionStore.mm:138:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

Comment on attachment 303869 [details]
Patch

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

Will look more later

> Source/WebKit2/UIProcess/API/Cocoa/WKUserContentExtension.h:2
> + * Copyright (C) 2015 Apple Inc. All rights reserved.

Correct (C) date throughout would be great.

> Source/WebKit2/UIProcess/API/Cocoa/WKUserContentExtension.h:31
> +WK_CLASS_AVAILABLE(macosx(10.11), ios(9.0))
> +@interface _WKUserContentFilter : NSObject

Why is this header called WKUserContentExtension.h but declares _WKUserContentFilter?

[Alex Christensen]

Comment on attachment 303869 [details]
Patch

You are seeing the results of "svn cp" in the patch.  It shows green of the unmodified file it was copied from, then below it has the additional changes done on that file.  The copyright dates of 2015 are because I'm copying files originally written in 2015, then below it shows I update the copyright date.  WKUserContentExtension.h.  WKUserContentExtension.h is based on _WKUserContentFilter.h, then modified.

[Geoffrey Garen]

Comment on attachment 303869 [details]
Patch

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

> Source/WebKit2/ChangeLog:10
> +        This takes _WKUserContentExtensionStore and _WKUserContentFilter and turns them into the consistently-named public API
> +        WKUserContentExtensionStore and WKUserContentExtension respectively with the same design and functionaly of the original SPI.
> +        We also added public functions to WKUserContentController corresponding to existing private functions.

What meaning does the word "user" have in this API? Maybe it should just be WKContentExtensionStore / WKContentExtension. (This would imply removing "user" in lots of other places.)

I guess maybe you chose to say "user" because of WKUserContentController. Even though it's useful for this controller to distinguish between "user content" vs "web content", I think the value fades as we propagate the word "user" to other classes, which do not have a user vs web dichotomy. In the case of a content extension, there's no such thing as a content extension provided by the web author.

> Source/WebKit2/ChangeLog:15
> +        The peak-memory-reducing optimization of having NS_RELEASES_ARGUMENT in _compileContentExtensionForIdentifier is kept for Safari,
> +        but the public API doesn't need such an optimization.  The public compileContentExtensionForIdentifier will not deallocate the input

Why doesn't the public API need this optimization?

> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.h:45
> +        LookupFailed = 6, // Mirrors value of WKErrorContentExtensionStoreLookupFailed

Is this because we convert from our internal enum to the public API by static_cast? I think we should do the conversion with a switch statement instead, and not worry about making the integers equal. Speed of error reporting is not a priority.

> Source/WebKit2/UIProcess/API/Cocoa/WKError.h:47
> + @constant WKErrorContentExtensionStoreLookupFailed    Indicates that looking up a WKUserContentExtension failed.
> + @constant WKErrorContentExtensionStoreVersionMismatch Indicates that looking up a WKUserContentExtension found an extension with an incompatible binary version.
> + @constant WKErrorContentExtensionStoreCompileFailed   Indicates that compiling a WKUserContentExtension failed.
> + @constant WKErrorContentExtensionStoreRemoveFailed    Indicates that removing a WKUserContentExtension failed.

These should say "UserContent" instead of just "Content".

> Source/WebKit2/UIProcess/API/Cocoa/WKUserContentExtensionStore.mm:60
> +- (void)_compileContentExtensionForIdentifier:(NSString *)identifier encodedContentExtension:(NSString *)encodedContentExtension completionHandler:(void (^)(WKUserContentExtension *, NSError *))completionHandler releasesArgument:(BOOL)releasesArgument

I think a simpler way to make this distinction is for the internal callee always to release its argument, and for the caller that does not want releasing behavior to retain its argument.

[Alex Christensen]

(In reply to comment #13)
> > Source/WebKit2/ChangeLog:15
> > +        The peak-memory-reducing optimization of having NS_RELEASES_ARGUMENT in _compileContentExtensionForIdentifier is kept for Safari,
> > +        but the public API doesn't need such an optimization.  The public compileContentExtensionForIdentifier will not deallocate the input
> 
> Why doesn't the public API need this optimization?
NS_RELEASES_ARGUMENT is incredibly rarely used, and a developer's app would not have the same tight memory restrictions that our compiler daemon has, so keeping a ~6MB NSString alive during compiling, which uses ~150MB of memory is no big deal.

[Alex Christensen]

Created attachment 304008 [details]
Patch

[WebKit Commit Bot]

Attachment 304008 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/APIContentExtensionStore.cpp:192:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/APIContentExtensionStore.cpp:200:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/APIContentExtensionStore.cpp:207:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/APIContentExtensionStore.cpp:213:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/APIContentExtensionStore.cpp:224:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/APIContentExtensionStore.cpp:409:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/APIContentExtensionStore.cpp:414:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/APIContentExtensionStore.h:81:  make_error_code is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKUserContentExtensionStore.mm:125:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKUserContentExtensionStore.mm:138:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 10 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

Comment on attachment 304008 [details]
Patch

If this is just the patch that Geoff already looked up plus the rename, then rubberstamp=me

[Brady Eidson]

(In reply to comment #17)
> Comment on attachment 304008 [details]
> Patch
> 
> If this is just the patch that Geoff already looked up plus the rename, then
> rubberstamp=me

Oh - And, it has to compile, too.

[Alex Christensen]

Created attachment 304010 [details]
Patch

[Alex Christensen]

Created attachment 304011 [details]
Patch

[WebKit Commit Bot]

Attachment 304011 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/APIContentExtensionStore.cpp:192:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/APIContentExtensionStore.cpp:200:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/APIContentExtensionStore.cpp:207:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/APIContentExtensionStore.cpp:213:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/APIContentExtensionStore.cpp:224:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/APIContentExtensionStore.cpp:409:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/APIContentExtensionStore.cpp:414:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/APIContentExtensionStore.h:81:  make_error_code is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKUserContentExtensionStore.mm:125:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKUserContentExtensionStore.mm:138:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 10 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Created attachment 304016 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r213696