[Content Extensions] Move compiling of content extensions to the UIProcess
Created attachment 248092 [details] Patch
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.
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?
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.
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
(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.
(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.
Created attachment 248106 [details] Patch
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.
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.
Committed r181200: <http://trac.webkit.org/changeset/181200>