Bug 142402 – [Content Extensions] Move compiling of content extensions to the UIProcess

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

[Sam Weinig]

[Content Extensions] Move compiling of content extensions to the UIProcess

[Sam Weinig]

Created attachment 248092 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/page/UserContentController.cpp:182:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WebCore/page/UserContentController.h:84:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 2 in 43 files


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

[Benjamin Poulain]

Comment on attachment 248092 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        Convert CompiledContentExtension to be an abstract base class so that we can back it however
> +        we like at the WebKit level. Since it doesn't necessarily use Vectors for its backing store
> +        any more, change the interface to use pointer/length.

I would have gone an other way:

Give the compiler an abstract "writer/reader" class to manipulate the bytecode. There would be 2 kinds of writer: one backed by a file, the other backed by a vector. That way, we would avoid having the entire DFA in memory (at the cost of having to read back from the disk to link).

> Source/WebCore/contentextensions/CompiledContentExtension.h:42
> +struct CompiledContentExtensionData {
> +    Vector<DFABytecode> bytecode;
> +    Vector<SerializedActionByte> actions;
> +};

This could be in ContentExtensionCompiler.h

> Source/WebCore/contentextensions/ContentExtensionRule.h:70
>      static Action deserialize(const Vector<SerializedActionByte>&, unsigned location);

Isn't this dead?

> Source/WebCore/contentextensions/DFABytecodeInterpreter.cpp:40
>      return *reinterpret_cast<const IntType*>(&bytecode[index]);

ASSERT(index + sizeof(IntType) <= m_bytecodeLength)

> Source/WebCore/contentextensions/DFABytecodeInterpreter.cpp:54
> +        switch (static_cast<DFABytecodeInstruction>(m_bytecode[programCounter])) {

ASSERT(programCounter <= m_bytecodeLength)

> Source/WebCore/contentextensions/DFABytecodeInterpreter.h:46
> +        UNUSED_PARAM(m_bytecodeLength);

ASSERT(bytecode);
ASSERT(m_bytecodeLength)?

> Source/WebCore/page/UserContentController.cpp:182
> -void UserContentController::addUserContentFilter(const String& name, const String& ruleList)
> +void UserContentController::addUserContentExtension(const String& name, RefPtr<ContentExtensions::CompiledContentExtension> contentExtension)

Yeaah, I never liked the "Filter" :)

> Source/WebKit2/UIProcess/API/APIUserContentExtension.h:34
> +class UserContentFilter final : public ObjectImpl<Object::Type::UserContentFilter> {

Still using the name Filter here?

> Source/WebKit2/UIProcess/API/APIUserContentExtension.h:41
> +    UserContentFilter(const WTF::String& name, const WTF::String& serializedRules);

Shouldn't this be private?

[Alex Christensen]

Comment on attachment 248092 [details]
Patch

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

Patch needs a little refining, but the concept is good.

> Source/WebCore/contentextensions/ContentExtensionCompiler.cpp:157
> +    return { WTF::move(bytecode), WTF::move(actions) };

I feel like you have redundant WTF::move calls, but better too many than too few.

> Source/WebCore/contentextensions/DFABytecodeInterpreter.h:55
> +    const size_t m_bytecodeLength;

I used unsigned for the index in the byte code many times to keep the byte code small.  My jumps are only 4 bytes long.  Should we limit the size of byte code and actions to 2^32 bytes?

> Source/WebKit2/Shared/WebCompiledContentExtensionData.cpp:38
> +    encoder << bytecode;
> +    encoder << actions;

Does this copy the whole vector across IPC?  I hope this is temporary.

> Source/WebKit2/UIProcess/API/APIUserContentExtension.cpp:31
> +UserContentFilter::UserContentFilter(const WTF::String& name, const WTF::String& serializedRules)

Shouldn't we get rid of this?  Why is this file in the patch twice?

> Source/WebKit2/UIProcess/API/APIUserContentExtension.h:34
> +class UserContentFilter final : public ObjectImpl<Object::Type::UserContentFilter> {

Same here.  Get rid of this file.

> Source/WebKit2/UIProcess/API/C/WKUserContentFilterRef.cpp:41
>  WKUserContentFilterRef WKUserContentFilterCreate(WKStringRef nameRef, WKStringRef serializedRulesRef)

Change this also to WKUserContentExtensionCreate

> Tools/TestWebKitAPI/Tests/WebCore/ContentExtensions.cpp:46
> +        return os << "ContentFilterAction::CSSDisplayNone";

This is just copying the file, but in a follow-up patch we should print out the selector in this case.

[Benjamin Poulain]

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

>> Source/WebCore/contentextensions/CompiledContentExtension.h:42
>> +};
> 
> This could be in ContentExtensionCompiler.h

Ok, you use it outside WebCore, ignore my previous comment.

> Source/WebKit2/WebProcess/UserContent/WebUserContentController.messages.in:37
> +    AddUserContentExtensions(Vector<std::pair<String, WebKit::WebCompiledContentExtensionData>> userContentFilters);

userContentFilters -> userContentExtensions

[Sam Weinig]

(In reply to comment #3)
> Comment on attachment 248092 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248092&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        Convert CompiledContentExtension to be an abstract base class so that we can back it however
> > +        we like at the WebKit level. Since it doesn't necessarily use Vectors for its backing store
> > +        any more, change the interface to use pointer/length.
> 
> I would have gone an other way:
> 
> Give the compiler an abstract "writer/reader" class to manipulate the
> bytecode. There would be 2 kinds of writer: one backed by a file, the other
> backed by a vector. That way, we would avoid having the entire DFA in memory
> (at the cost of having to read back from the disk to link).

I completely agree this is the right way to go.  I was avoiding doing that in this patch as it was already getting to big. It will be part of follow up patches.

> 
> > Source/WebCore/contentextensions/CompiledContentExtension.h:42
> > +struct CompiledContentExtensionData {
> > +    Vector<DFABytecode> bytecode;
> > +    Vector<SerializedActionByte> actions;
> > +};
> 
> This could be in ContentExtensionCompiler.h

Done.

> 
> > Source/WebCore/contentextensions/ContentExtensionRule.h:70
> >      static Action deserialize(const Vector<SerializedActionByte>&, unsigned location);
> 
> Isn't this dead?

Yup, bad merge on my part.

> 
> > Source/WebCore/contentextensions/DFABytecodeInterpreter.cpp:40
> >      return *reinterpret_cast<const IntType*>(&bytecode[index]);
> 
> ASSERT(index + sizeof(IntType) <= m_bytecodeLength)

Done.

> 
> > Source/WebCore/contentextensions/DFABytecodeInterpreter.cpp:54
> > +        switch (static_cast<DFABytecodeInstruction>(m_bytecode[programCounter])) {
> 
> ASSERT(programCounter <= m_bytecodeLength)

Done.

> 
> > Source/WebCore/contentextensions/DFABytecodeInterpreter.h:46
> > +        UNUSED_PARAM(m_bytecodeLength);
> 
> ASSERT(bytecode);
> ASSERT(m_bytecodeLength)?

I don't think this adds much.

> 
> > Source/WebCore/page/UserContentController.cpp:182
> > -void UserContentController::addUserContentFilter(const String& name, const String& ruleList)
> > +void UserContentController::addUserContentExtension(const String& name, RefPtr<ContentExtensions::CompiledContentExtension> contentExtension)
> 
> Yeaah, I never liked the "Filter" :)
> 
> > Source/WebKit2/UIProcess/API/APIUserContentExtension.h:34
> > +class UserContentFilter final : public ObjectImpl<Object::Type::UserContentFilter> {
> 
> Still using the name Filter here?

No, weird diff thing.

> 
> > Source/WebKit2/UIProcess/API/APIUserContentExtension.h:41
> > +    UserContentFilter(const WTF::String& name, const WTF::String& serializedRules);
> 
> Shouldn't this be private?

It can't be due to ObjC bridging stuff.

[Sam Weinig]

(In reply to comment #4)
> Comment on attachment 248092 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248092&action=review
> 
> Patch needs a little refining, but the concept is good.
> 
> > Source/WebCore/contentextensions/ContentExtensionCompiler.cpp:157
> > +    return { WTF::move(bytecode), WTF::move(actions) };
> 
> I feel like you have redundant WTF::move calls, but better too many than too
> few.
> 
> > Source/WebCore/contentextensions/DFABytecodeInterpreter.h:55
> > +    const size_t m_bytecodeLength;
> 
> I used unsigned for the index in the byte code many times to keep the byte
> code small.  My jumps are only 4 bytes long.  Should we limit the size of
> byte code and actions to 2^32 bytes?

Done.

> 
> > Source/WebKit2/Shared/WebCompiledContentExtensionData.cpp:38
> > +    encoder << bytecode;
> > +    encoder << actions;
> 
> Does this copy the whole vector across IPC?  I hope this is temporary.

Yes. As mentioned in the ChangeLog. This will move to being shared/file back.

> 
> > Source/WebKit2/UIProcess/API/APIUserContentExtension.cpp:31
> > +UserContentFilter::UserContentFilter(const WTF::String& name, const WTF::String& serializedRules)
> 
> Shouldn't we get rid of this?  Why is this file in the patch twice?

This is just svn-create-patch behavior. It's renaming and modifying the file.

> 
> > Source/WebKit2/UIProcess/API/APIUserContentExtension.h:34
> > +class UserContentFilter final : public ObjectImpl<Object::Type::UserContentFilter> {
> 
> Same here.  Get rid of this file.

See above.

> 
> > Source/WebKit2/UIProcess/API/C/WKUserContentFilterRef.cpp:41
> >  WKUserContentFilterRef WKUserContentFilterCreate(WKStringRef nameRef, WKStringRef serializedRulesRef)
> 
> Change this also to WKUserContentExtensionCreate

I'll have to do this later to avoid breaking Safari.

> 
> > Tools/TestWebKitAPI/Tests/WebCore/ContentExtensions.cpp:46
> > +        return os << "ContentFilterAction::CSSDisplayNone";
> 
> This is just copying the file, but in a follow-up patch we should print out
> the selector in this case.

See above.

[Sam Weinig]

Created attachment 248106 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/page/UserContentController.cpp:182:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WebCore/page/UserContentController.h:84:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WebKit2/CMakeLists.txt:242:  Alphabetical sorting problem. "UIProcess/API/APIFrameInfo.cpp" should be before "UIProcess/WebsiteData/unix/WebsiteDataStoreUnix.cpp".  [list/order] [5]
Total errors found: 3 in 44 files


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

[Benjamin Poulain]

Comment on attachment 248106 [details]
Patch

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

> Source/WebCore/contentextensions/ContentExtensionRule.cpp:44
>  {
>      switch (static_cast<ActionType>(actions[location])) {

ASSERT(location <= actionsLength) since we no longer have the Vector's Release assert.

[Sam Weinig]

Committed r181200: <http://trac.webkit.org/changeset/181200>