Bug 142257 – Add basic support for character sets to the URL Filter parser

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

Add basic support for character sets to the URL Filter parser

[Benjamin Poulain]

Created attachment 247834 [details]
Patch

[Build Bot]

Comment on attachment 247834 [details]
Patch

Attachment 247834 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6528593282727936

New failing tests:
http/tests/usercontentfilter/basic-filter.html

[Build Bot]

Created attachment 247840 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Benjamin Poulain]

Created attachment 247896 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/contentextensions/URLFilterParser.cpp:71:  static_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/contentextensions/URLFilterParser.cpp:73:  static_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/contentextensions/URLFilterParser.cpp:75:  static_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/contentextensions/URLFilterParser.cpp:77:  static_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 2 files


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

[Alex Christensen]

Comment on attachment 247896 [details]
Patch

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

I need a bit more time to look at this, but I already have a few questions:

> Source/WebCore/contentextensions/URLFilterParser.cpp:58
>      ZeroToMany = 0x2000,
>      OneToMany = 0x4000

I think these should be named ZeroOrMore and OneOrMore.

> Source/WebCore/contentextensions/URLFilterParser.cpp:261
> +        // Nothing to do here. The character set atom may have a quantifier, we sink the atom on lazily.

"atom on lazily" -> "atom lazily"
Why are we sinking the atom lazily instead of sinking it here?

> Source/WebCore/contentextensions/URLFilterParser.cpp:266
> +        fail(ASCIILiteral("Buildins character class atoms are not supported yet."));

Buildins

> Source/WebCore/contentextensions/URLFilterParser.cpp:499
> +    BitVector m_pendingCharacterSet { 128 };

A BitVector this big would always allocate a buffer.  It would be more efficient to use 2 uint64_t's.  I wish we had a uint128_t.  A lot of SIMD registers have 128 bits.  Could we eventually use those?

[Alex Christensen]

Comment on attachment 247896 [details]
Patch

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

> Source/WebCore/contentextensions/URLFilterParser.cpp:70
> +    switch (trivialAtom & 0xf000) {

static_cast<AtomQuantifier>(trivialAtom & 0xf00) here to avoid static_cast<unsigned> at each case.  Then the switch will complain if we miss an enum value.

[Benjamin Poulain]

(In reply to comment #7)
> Comment on attachment 247896 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247896&action=review
> 
> > Source/WebCore/contentextensions/URLFilterParser.cpp:70
> > +    switch (trivialAtom & 0xf000) {
> 
> static_cast<AtomQuantifier>(trivialAtom & 0xf00) here to avoid
> static_cast<unsigned> at each case.  Then the switch will complain if we
> miss an enum value.

That is semantically worse though. I like to make sure enum class never take incorrect values by design.

[Benjamin Poulain]

Created attachment 247916 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/contentextensions/URLFilterParser.cpp:71:  static_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/contentextensions/URLFilterParser.cpp:73:  static_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/contentextensions/URLFilterParser.cpp:75:  static_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/contentextensions/URLFilterParser.cpp:77:  static_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 2 files


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

[Alex Christensen]

Comment on attachment 247916 [details]
Patch

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

This looks good to me.  I don't completely understand sinkPendingAtomIfNecessary, but the rest looks good.  Lambdas calling different generateTransition functions confused me a little, but I figured out what was going on.

I really think this needs unit tests.  Tests should have some regular expressions, some strings they match, and some strings they do not match, as well as some invalid regular expressions that fail in each possible way to fail.

> Source/WebCore/contentextensions/URLFilterParser.cpp:255
> +        for (unsigned i = a; i <= b; ++i)

This seems to have correct behavior.  [z-a] should not match anything.

> Source/WebCore/contentextensions/URLFilterParser.cpp:381
> +    void generateTransition(BitVector characterSet, bool isInverted, unsigned source, unsigned target)

A lot of these should probably be const BitVector&.  Here and elsewhere.

> Source/WebCore/contentextensions/URLFilterParser.cpp:386
> +                char character = static_cast<char>(characterIterator);

Is the characterIterator just an integer that counts from 0 to 127 if that bit is set in the BitVector?

> Source/WebCore/contentextensions/URLFilterParser.cpp:422
> +        case BuildMode::PrefixTree: {

The only thing I don't understand is the difference between PrefixTree and DirectGeneration.

[Alex Christensen]

Comment on attachment 247916 [details]
Patch

r=me

This needs some serious unit testing, but we can land that in a separate patch.

[Benjamin Poulain]

Created attachment 248010 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/contentextensions/URLFilterParser.cpp:71:  static_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/contentextensions/URLFilterParser.cpp:73:  static_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/contentextensions/URLFilterParser.cpp:75:  static_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/contentextensions/URLFilterParser.cpp:77:  static_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 7 files


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

[Benjamin Poulain]

Comment on attachment 248010 [details]
Patch

Clearing flags on attachment: 248010

Committed r181107: <http://trac.webkit.org/changeset/181107>

[Benjamin Poulain]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

This patch is causing failures on EFL, Gtk, Mavericks, Yosemite and Windows test bots <http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fusercontentfilter%2Fcharacter-set-basic-support.html>.

Stephanie checked in a Skip of this <https://trac.webkit.org/changeset/181129>, but we should not be landing tests that cause all the bots to fail.

[Benjamin Poulain]

(In reply to comment #17)
> This patch is causing failures on EFL, Gtk, Mavericks, Yosemite and Windows
> test bots
> <http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=http%2Ftests%2Fusercontentfilter%2Fcharacter-set-
> basic-support.html>.
> 
> Stephanie checked in a Skip of this
> <https://trac.webkit.org/changeset/181129>, but we should not be landing
> tests that cause all the bots to fail.

What happened is Alex moved the tests in a new repository before I landed this: http://trac.webkit.org/changeset/181031

As a result, this test was not run as it should have been.