Bug 160365 – Refactor ContentFilterUnblockHandler to allow platform implementations

• Status: NEW
• Component: Platform

[Adrian Perez]

Following up bug 154553:

>> I think we should split the patch, enabling content extensions first, and
>> unskipping the layout tests, and then another patch to add the GTK+ API. It
>> would help a lot to start working on  a patch for epiphany in parallel.
>
> ACK. I have been working on the split, and while doing the first part which
> unskips the layout test I noticed that there was missing some changes to the
> CMake files to have ENABLE_CONTENT_FILTERING defined, and once that was enabled
> it was needed to add more files to the build, and ContentFilterUnblockHandler
> needs refactoring as it's only implemented for Cocoa.
>
> The refactor be a separate patch as well, which will add a port-agnostic
> ContentFilterUnblockHandlerBase, and move the Cocoa-specific implementation
> into WebCore/platform/cocoa/ContentFilterUnblockHandler.{h,mm}. After that
> is done, we can add an implementation for the GTK port.

[Adrian Perez]

Created attachment 284924 [details]
Patch

[Adrian Perez]

Funny story: I was so concerned about not breaking the Cocoa builds that
I went the extra mile to edit the project files with Xcode and do a couple
of build-edit-test cycles until things worked fine... but then I forgot to
do one last build of the GTK+ port. I'll get that fixed and re-upload the
patch :-)

[Adrian Perez]

Created attachment 284940 [details]
Patch

[Andy Estes]

ContentFilterUnblockHandler has nothing to do with content extensions. Are you sure you need to do this?

[Adrian Perez]

(In reply to comment #4)
> ContentFilterUnblockHandler has nothing to do with content extensions. Are
> you sure you need to do this?

After setting ENABLE_CONTENT_FILTERING to ON in the CMake build files, then
ContentFilterUnblockHandler is used from a few places around:

  Source/WebCore/loader/ContentFilter.cpp
  Source/WebCore/loader/FrameLoaderClient.h
  Source/WebKit2/UIProcess/WebFrameProxy.h
  Source/WebCore/testing/MockContentFilter.cpp
  Source/WebCore/testing/MockContentFilterSettings.cpp
  ...

In particular MockContentFilter* are needed to be able to unskip the
contentfiltering/* layout tests, which we would like to have enabled and
passing in the GTK+ port. My plan is to provide the minimal implementation
of ContentFilterUnblockHandler which will just make content filtering work
(e.g. for the moment we are not going to support parental controls).

If you think there might a better course of action, I would be more than
happy to learn about it.

[Andy Estes]

Unfortunately we have two features with confusingly similar names. The feature covered by ENABLE(CONTENT_FILTERING) is to support the Parental Controls website filtering on macOS and iOS. If you're interested in porting content blocking extensions, you want what's covered by ENABLE(CONTENT_EXTENSIONS).

[Adrian Perez]

(In reply to comment #6)
> Unfortunately we have two features with confusingly similar names. The
> feature covered by ENABLE(CONTENT_FILTERING) is to support the Parental
> Controls website filtering on macOS and iOS. If you're interested in porting
> content blocking extensions, you want what's covered by
> ENABLE(CONTENT_EXTENSIONS).

Wow, the names are good candidates to be mixed up :-S

I'll check tomorrow how things go when enabling CONTENT_EXTENSIONS, and if
this patch is not needed I'll close this bug as invalid. Thanks a lot for
pointing in the right direction!

[Darin Adler]

Comment on attachment 284940 [details]
Patch

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

Generally looks OK to me. Most of my comments are about issues in the existing code that is being moved, not new.

> Source/WebCore/platform/ContentFilterUnblockHandlerBase.h:2
> + * Copyright (C) 2016 Igalia S.L.

This copyright line is insufficient. We must not remove the copyright of the authors of the code when moving code to a new file. Shouldn’t add a new copyright unless making significant changes to the code; there’s probably enough change here for that. The usual rule of thumb is roughly “10 lines” although it’s not an exact science.

> Source/WebCore/platform/ContentFilterUnblockHandlerBase.h:27
> +#ifndef ContentFilterUnblockHandlerBase_h
> +#define ContentFilterUnblockHandlerBase_h

Any files we touch should be updated to use #pragma once instead of old style include guards.

> Source/WebCore/platform/ContentFilterUnblockHandlerBase.h:31
> +#include <wtf/text/WTFString.h>

This include is not needed, since URL.h already includes this header.

> Source/WebCore/platform/ContentFilterUnblockHandlerBase.h:57
> +    WEBCORE_EXPORT ContentFilterUnblockHandlerBase(String unblockURLHost, UnblockRequesterFunction unblockRequester)
> +        : m_unblockURLHost { WTFMove(unblockURLHost) }
> +        , m_unblockRequester { WTFMove(unblockRequester) } { }

Types here are a little bit strange. I would have expected String&& and UnblockRequesterFunction&& since we are passing ownership.

> Source/WebCore/platform/ContentFilterUnblockHandlerBase.h:65
> +};
> +
> +
> +template<class Encoder>

Normally we leave a single blank line, not two lines.

> Source/WebCore/platform/cocoa/ContentFilterUnblockHandler.h:27
> +#ifndef ContentFilterUnblockHandler_h
> +#define ContentFilterUnblockHandler_h

Any files we touch should be updated to use #pragma once instead of old style include guards.

> Source/WebCore/platform/cocoa/ContentFilterUnblockHandler.h:33
> +#include "URL.h"
> +#include <functional>

These includes are not needed because they are already included by the base header.

> Source/WebCore/platform/cocoa/ContentFilterUnblockHandler.h:35
> +#include <wtf/text/WTFString.h>

This include is not needed because it is already included by the base header.

> Source/WebCore/platform/cocoa/ContentFilterUnblockHandler.h:50
> +    WEBCORE_EXPORT ContentFilterUnblockHandler(String unblockURLHost, UnblockRequesterFunction);

Types here are a little bit strange. I would have expected either const String& if not passing ownership or String&& if passing ownership. And UnblockRequesterFunction&& since we are passing ownership.

> Source/WebCore/platform/cocoa/ContentFilterUnblockHandler.h:53
> +    ContentFilterUnblockHandler(String unblockURLHost, RetainPtr<WebFilterEvaluator>);

Types here are a little bit strange. I would have expected either const String& if not passing ownership or String&& if passing ownership. And RetainPtr&& since we are passing ownership.

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:1946
> +    if (contentFilterUnblockHandler.encodingRequiresPlatformData()) {

Is this really a runtime thing, or is it fixed at compile time for each platform?

[Adrian Perez]

(In reply to comment #7)
> (In reply to comment #6)
> > Unfortunately we have two features with confusingly similar names. The
> > feature covered by ENABLE(CONTENT_FILTERING) is to support the Parental
> > Controls website filtering on macOS and iOS. If you're interested in porting
> > content blocking extensions, you want what's covered by
> > ENABLE(CONTENT_EXTENSIONS).
> 
> Wow, the names are good candidates to be mixed up :-S
> 
> I'll check tomorrow how things go when enabling CONTENT_EXTENSIONS, and if
> this patch is not needed I'll close this bug as invalid. Thanks a lot for
> pointing in the right direction!

As expected, Andy was right and #defining ENABLE_CONTENT_EXTENSIONS to “1”
does not need this refactoring. Nevertheless, Darin took the time to review
it, so if there is still interest in having it applied, I will be glad to
make it ready for landing: right now we are more interested in supporting
content extensions, but we may be interested in having support for parental
controls in the GTK+ port later on.

[Adrian Perez]

As per the previous comments, this is not blocking bug 154553 anymore.