Bug 146241 - [Content Extensions] Add a way to match a domain but not subdomains
Summary: [Content Extensions] Add a way to match a domain but not subdomains
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-23 11:26 PDT by Alex Christensen
Modified: 2015-06-25 17:58 PDT (History)
1 user (show)

See Also:


Attachments
Patch (10.38 KB, patch)
2015-06-23 11:31 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (13.51 KB, patch)
2015-06-23 11:39 PDT, Alex Christensen
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2015-06-23 11:26:33 PDT
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.
Comment 1 Alex Christensen 2015-06-23 11:31:18 PDT
Created attachment 255418 [details]
Patch
Comment 2 Alex Christensen 2015-06-23 11:39:17 PDT
Created attachment 255419 [details]
Patch
Comment 3 Darin Adler 2015-06-23 13:49:47 PDT
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.
Comment 4 Benjamin Poulain 2015-06-23 14:02:51 PDT
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.
Comment 5 Alex Christensen 2015-06-25 17:53:07 PDT
(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.
Comment 6 Alex Christensen 2015-06-25 17:58:27 PDT
http://trac.webkit.org/changeset/185973