1223634 - globs are not supported as MatchPatterns

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Deekshith]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20151110004043

Steps to reproduce:

I was trying to use exclude_globs and exclude_matches in Webextension manifest but it is not working. It is working in Chrome and Opera browsers.
"exclude_globs": ["*.pdf",
                        "http*://www.messenger.com/*",
                        "http*://docs.google.com/*",
                        "http*://mail.google.com/*",
                        "http*://www.mediafire.com/*"]

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

exclude_globals is not yet supported, but exclude_matches is supposed to be supported. Can you give an example of the exclude_matches patterns you tried?

Comment 2

[Deekshith]

Gotcha, (In reply to Bill McCloskey (:billm) from comment #1)
> exclude_globals is not yet supported, but exclude_matches is supposed to be
> supported. Can you give an example of the exclude_matches patterns you tried?

Got it. exclude_globs was working in other browsers but not in Firefox but I tried exclude_matches did not work in other browsers too so it might be my fault.

Here is the pattern I tried:

"content_scripts": [
    {
      "matches": ["<all_urls>"],
      "exclude_globs": ["*.pdf",
                        "http*://*.facebook.com/*",
                        "http*://www.messenger.com/*",
                        "http*://mail.google.com/*",
                        "http*://www.mediafire.com/*"],
      "js": ["js/127774.user.js"]
    }
]

I wanted it to inject on all URLs except www.facebook.com and other websites but it does not work.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yes, these aren't valid match patterns. See https://developer.chrome.com/extensions/match_patterns for the syntax. For the first one you'll have to do something like "*://*/*.pdf" and for the others "*://<domain>/*". The * in the scheme part matches only http and https. If you want to match file or ftp, you need to include them separately.

I'm going to repurpose this bug to be about missing support for globs. We don't have a bug about that yet.

Comment 4

[Deekshith]

(In reply to Bill McCloskey (:billm) from comment #3)
> Yes, these aren't valid match patterns. See
> https://developer.chrome.com/extensions/match_patterns for the syntax. For
> the first one you'll have to do something like "*://*/*.pdf" and for the
> others "*://<domain>/*". The * in the scheme part matches only http and
> https. If you want to match file or ftp, you need to include them separately.
> 
> I'm going to repurpose this bug to be about missing support for globs. We
> don't have a bug about that yet.

Thats very helpful. Thank you for the information.

Comment 5

[Tom S [:evilpie]]

Attachment: Implement include_globs/exclude_globs

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8730177 [details] [diff] [review]
Implement include_globs/exclude_globs

Review of attachment 8730177 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Tom. I'm a bit busy, but Kris should be able to look at this.

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8730177 [details] [diff] [review]
Implement include_globs/exclude_globs

Review of attachment 8730177 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Tom, this is great!
Follows a preliminary feedback (Kris will look at it for the final review):

The "test_MatchGlobs.js" test |MatchGlob| as a unit, I'd like an additional test case which
checks that the MatchGlob is correctly used for the content scripts through the "include_globs"/"exclude_globs"
manifest properties

::: toolkit/modules/addons/MatchPattern.jsm
@@ +184,5 @@
> +
> +MatchGlobs.prototype = {
> +  matches(uri) {
> +    for (let regexp of this.regexps) {
> +      dump(regexp.toSource());

This |dump| (probably used to check the regexps during the prototyping) has to be removed.

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8730177 [details] [diff] [review]
Implement include_globs/exclude_globs

Review of attachment 8730177 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks. This looks good aside from a few nits.

I think we need some additional tests for the manifest properties, though.

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +159,2 @@
>  
>    // TODO: Support glob patterns.

This TODO can be removed now.

@@ +170,5 @@
>      if (this.exclude_matches_.matches(uri)) {
>        return false;
>      }
>  
> +    if (!this.include_globs_.matches(uri)) {

This shouldn't apply if `include_globs` wasn't set in the manifest.

::: toolkit/modules/addons/MatchPattern.jsm
@@ +185,5 @@
> +MatchGlobs.prototype = {
> +  matches(uri) {
> +    for (let regexp of this.regexps) {
> +      dump(regexp.toSource());
> +      if (regexp.test(uri.spec)) {

We should probably extract `uri.spec` at the head of the function. XPConnect property access is still pretty expensive.

::: toolkit/modules/tests/xpcshell/test_MatchGlobs.js
@@ +6,5 @@
> +Components.utils.import("resource://gre/modules/MatchPattern.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +function test(url, pattern)
> +{

We place the opening curly brace on the same line as the function statement in WebExtension code.

@@ +14,5 @@
> +}
> +
> +function pass({url, pattern})
> +{
> +  do_check_true(test(url, pattern), `Expected match: ${JSON.stringify(pattern)}, ${url}`);

Please use `ok` rather than `do_check_true` and `do_check_false`

Comment 9

[Tom S [:evilpie]]

Attachment: v2 Implement include_globs/exclude_globs

Great. Adding the manifest test first would have caught the options bug ^^

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8730676 [details] [diff] [review]
v2 Implement include_globs/exclude_globs

Review of attachment 8730676 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Thanks.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41042ab2e4bc

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5948b646910e

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1897f0b4eb3

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5948b646910e
https://hg.mozilla.org/mozilla-central/rev/e1897f0b4eb3

Comment 15

[Will Bamberg [:wbamberg]]

I've added these to:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/content_scripts

...and also added a new little section on matching here:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/content_scripts#Matching_URL_patterns

Let me know if this covers it!

Comment 16

[Tom S [:evilpie]]

Yes, I think so. In my mind the most important bits are covered: 1. They are optional. 2. The only refine something already matched with "matches". 3. Short description of the regexp like syntax.

Thanks!