Bug 146241 – [Content Extensions] Add a way to match a domain but not subdomains

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alex Christensen]

It is currently impossible to have an if-domain apply to sub1.sub2.webkit.org but not sub2.webkit.org because if-domain applies to the domain and any subdomains.

[Alex Christensen]

Created attachment 255418 [details]
Patch

[Alex Christensen]

Created attachment 255419 [details]
Patch

[Darin Adler]

Comment on attachment 255419 [details]
Patch

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

> Source/WebCore/contentextensions/CombinedURLFilters.cpp:125
> +    if (domain.length() && domain[0] == '*') {

WTF::String operator[] returns 0 for characters past the end of the string, so you don’t need the domain.length() here. Not sure I love that design, but there’s no need for the redundant check. Note that WTF::StringView does not work that way.

> Source/WebCore/contentextensions/CombinedURLFilters.cpp:131
> +        prependDot.reserveInitialCapacity(domain.length() + 2);

I suggest putting domain.length() into a local. Not sure it will be CSE eliminated.

[Benjamin Poulain]

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

> Source/WebCore/ChangeLog:10
> +        This patch makes it possible to have a trigger with an if-domain apply to sub2.sub1.webkit.org
> +        but not sub1.webkit.org by making the domains default to only applying to the domain and not subdomains.
> +        To make a domain apply to a domain and any subdomains, the domain must begin with a '*'.

This is backward incompatible. Make sure to have a radar for this patch and fill the documentation section.

> Source/WebCore/contentextensions/CombinedURLFilters.cpp:126
> +        

Extra blank line.

> Source/WebCore/contentextensions/CombinedURLFilters.cpp:130
> +        Vector<Term> prependDot;
> +        Vector<Term> prependBeginningOfLine;

I wonder if we could make those name clearer.

What about:
-Vector<Term> anySubdomains;
-Vector<Term> exactDomain;

Or something like that.

> Source/WebCore/contentextensions/CombinedURLFilters.cpp:151
> +        

Blank line.

[Alex Christensen]

(In reply to comment #3)
> WTF::String operator[] returns 0 for characters past the end of the string, so you don’t need the domain.length() here. Not sure I love that design, but there’s no need for the redundant check. Note that WTF::StringView does not work that way.
That is not a good design.  I'm going to keep this check here in anticipation of us writing a better operator[].
(In reply to comment #4)
> > Source/WebCore/contentextensions/CombinedURLFilters.cpp:130
> > +        Vector<Term> prependDot;
> > +        Vector<Term> prependBeginningOfLine;
> 
> I wonder if we could make those name clearer.
The names are based on what the regular expression syntax with the same functionality would be. The comments indicate what they mean.

[Alex Christensen]

http://trac.webkit.org/changeset/185973