Bug 144833 – [Content Extensions] Support domain-specific rules and exceptions

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

[Alex Christensen]

Domain-specific rules are needed.  Someone might want to block something on every page except one page where something looking like something that needs to be blocked actually needs to be let through for the page to function properly.  Someone might also want to block something common-looking that would break many websites but on one specific website it really needs to be blocked.

I added domain-specific whitelists and blacklists.

[Alex Christensen]

Created attachment 252787 [details]
Patch

[Alex Christensen]

Created attachment 252805 [details]
Patch

[Darin Adler]

Comment on attachment 252805 [details]
Patch

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

> Source/WebCore/contentextensions/CombinedURLFilters.cpp:136
> +    prependDot.append(canonicalDotStar);

uncheckedAppend

> Source/WebCore/contentextensions/CombinedURLFilters.cpp:137
> +    prependDot.append(Term('.', true));

uncheckedAppend; the boolean here is ugly

> Source/WebCore/contentextensions/CombinedURLFilters.cpp:142
> +        prependDot.append(Term(domain[i], true));

uncheckedAppend; the boolean here is ugly

> Source/WebCore/contentextensions/CombinedURLFilters.cpp:143
> +        prependBeginningOfLine.append(Term(domain[i], true));

uncheckedAppend

> Source/WebCore/contentextensions/CombinedURLFilters.cpp:145
> +    prependDot.append(Term::EndOfLineAssertionTerm);

uncheckedAppend

> Source/WebCore/contentextensions/CombinedURLFilters.cpp:146
> +    prependBeginningOfLine.append(Term::EndOfLineAssertionTerm);

uncheckedAppend

> Source/WebCore/contentextensions/ContentExtensionParser.cpp:59
> +        // FIXME: JSObject::getIndex should be marked as const.

Seems like a strange place for this FIXME.

> Source/WebCore/contentextensions/ContentExtensionParser.cpp:61
> +        if (exec.hadException() || !value || !value.isString())

Do we really need to check !value here? I don’t think getIndex can return that unless it raises an exception.

> Source/WebCore/contentextensions/ContentExtensionParser.cpp:65
> +        String domain = value.toWTFString(&exec);

The code above already checked value.isString, so this should use JSString::value, not JSValue::toWTFString.

> Source/WebCore/contentextensions/ContentExtensionParser.cpp:72
> +        for (unsigned i = 0; i < domain.length(); ++i) {
> +            UChar c = domain.at(i);
> +            if (!isASCII(domain[i]) || isASCIIUpper(c))
> +                return ContentExtensionError::JSONDomainNotLowerCaseASCII;
> +        }

I think this can be done much more efficiently. One obvious mistake here is that this does domain[i] twice and puts one into a local variable. I suggest putting containsOnlyASCIIWithNoUppercase into a helper function rather than having the loop in line in this function.

> Source/WebCore/contentextensions/ContentExtensionParser.cpp:141
> +    if (ifDomain && !exec.hadException() && ifDomain.isObject()) {

No need to null check ifDomain; it can’t return null unless there is an exception.

> Source/WebCore/contentextensions/ContentExtensionParser.cpp:142
> +        auto ifDomainError = getDomainList(exec, ifDomain.toObject(&exec), trigger.domains);

Should use asObject rather than toObject since we already checked isObject.

> Source/WebCore/contentextensions/ContentExtensionParser.cpp:152
> +    if (unlessDomain && !exec.hadException() && unlessDomain.isObject()) {

No need to null check unlessDomain; it can’t return null unless there is an exception.

> Source/WebCore/contentextensions/ContentExtensionParser.cpp:155
> +        auto unlessDomainError = getDomainList(exec, unlessDomain.toObject(&exec), trigger.domains);

Should use asObject rather than toObject since we already checked isObject.

> Source/WebCore/contentextensions/ContentExtensionRule.h:49
> +    bool ifDomain { false }; // If this is false, that means domains is a list of unless-domains if it is not empty.

The name "if domain" is really confusing. Can we find a way to express this in idiomatic English? Maybe “applies only if domain matches” but I guess that’s not what it means (see the confusion?). Maybe an enum?

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:150
>              for (auto actionLocation : universalActions) {

Should be auto&, not auto.

[Alex Christensen]

Comment on attachment 252805 [details]
Patch

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

Addressed comments.  Uploading patch for landing.

>> Source/WebCore/contentextensions/ContentExtensionParser.cpp:59
>> +        // FIXME: JSObject::getIndex should be marked as const.
> 
> Seems like a strange place for this FIXME.

I didn't want to touch JavaScriptCore to make this patch easier to land/merge.  I'll look into this in a separate patch.

[Alex Christensen]

Created attachment 252881 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 252881 [details]
Patch

Clearing flags on attachment: 252881

Committed r184116: <http://trac.webkit.org/changeset/184116>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Myles C. Maxfield]

Build fix in r184121

[Alex Christensen]

rdar://problem/20822592