Extend URL filter's Term definition to support groups/subpatterns
Created attachment 248306 [details] Patch
Comment on attachment 248306 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248306&action=review A few small comments to be addressed, but it looks good. > Source/WebCore/contentextensions/URLFilterParser.cpp:161 > + ASSERT_WITH_SECURITY_IMPLICATION(m_termType == TermType::Group); > + if (m_termType != TermType::Group) > + return; Does ASSERT_WITH_SECURITY_IMPLICATION assert in release builds? If so, this is redundant. If not, why not just use RELEASE_ASSERT here? > Source/WebCore/contentextensions/URLFilterParser.cpp:291 > + ASSERT_NOT_REACHED(); > + return -1; These two lines are not necessary. > Source/WebCore/contentextensions/URLFilterParser.cpp:571 > + m_openGroups.append(Term(Term::GroupTerm)); Is the explicit constructor call necessary? > Tools/TestWebKitAPI/Tests/WebCore/ContentExtensions.cpp:119 > +const char* patternsStartingWithGroupFilter = "[{\"action\":{\"type\":\"block\"},\"trigger\":{\"url-filter\":\"(http://whatwg\\\\.org/)?webkit\134\134.org\"}}]"; I think \134\134 should be \\\\ > Tools/TestWebKitAPI/Tests/WebCore/ContentExtensions.cpp:121 > +TEST_F(ContentExtensionTest, PatternStartingWithGroup) Hooray! Test cases :)
(In reply to comment #2) > A few small comments to be addressed, but it looks good. Will fix, thanks for the review! > > Source/WebCore/contentextensions/URLFilterParser.cpp:161 > > + ASSERT_WITH_SECURITY_IMPLICATION(m_termType == TermType::Group); > > + if (m_termType != TermType::Group) > > + return; > > Does ASSERT_WITH_SECURITY_IMPLICATION assert in release builds? If so, this > is redundant. If not, why not just use RELEASE_ASSERT here? That's up to each port really, but by default ASSERT_WITH_SECURITY_IMPLICATION is a debug assert, not a release one. > > Source/WebCore/contentextensions/URLFilterParser.cpp:291 > > + ASSERT_NOT_REACHED(); > > + return -1; > > These two lines are not necessary. They are for GCC :(
(In reply to comment #3) > > > Source/WebCore/contentextensions/URLFilterParser.cpp:291 > > > + ASSERT_NOT_REACHED(); > > > + return -1; > > > > These two lines are not necessary. > > They are for GCC :( Ahah, no my bad, those two are useless. Without opening the patch I thought those were faster the switch().
Committed r181341: <http://trac.webkit.org/changeset/181341>