Bug 142519 - Extend URL filter's Term definition to support groups/subpatterns
Summary: Extend URL filter's Term definition to support groups/subpatterns
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-09 18:40 PDT by Benjamin Poulain
Modified: 2015-03-10 13:09 PDT (History)
2 users (show)

See Also:


Attachments
Patch (20.61 KB, patch)
2015-03-09 18:46 PDT, Benjamin Poulain
achristensen: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2015-03-09 18:40:55 PDT
Extend URL filter's Term definition to support groups/subpatterns
Comment 1 Benjamin Poulain 2015-03-09 18:46:03 PDT
Created attachment 248306 [details]
Patch
Comment 2 Alex Christensen 2015-03-09 21:54:57 PDT
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 :)
Comment 3 Benjamin Poulain 2015-03-09 22:02:08 PDT
(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 :(
Comment 4 Benjamin Poulain 2015-03-09 22:03:11 PDT
(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().
Comment 5 Benjamin Poulain 2015-03-10 13:09:54 PDT
Committed r181341: <http://trac.webkit.org/changeset/181341>