Bug 142519 – Extend URL filter's Term definition to support groups/subpatterns

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

[Benjamin Poulain]

Extend URL filter's Term definition to support groups/subpatterns

[Benjamin Poulain]

Created attachment 248306 [details]
Patch

[Alex Christensen]

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 :)

[Benjamin Poulain]

(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 :(

[Benjamin Poulain]

(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().

[Benjamin Poulain]

Committed r181341: <http://trac.webkit.org/changeset/181341>