Closed Bug 1223634 Opened 4 years ago Closed 4 years ago

globs are not supported as MatchPatterns

Categories

(WebExtensions :: Untriaged, defect)

44 Branch
Unspecified
macOS
defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: dkh.hyd, Assigned: evilpie)

Details

(Keywords: dev-doc-complete, Whiteboard: [berlin][good first bug])

Attachments

(1 file, 1 obsolete file)

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/*"]
OS: Unspecified → Mac OS X
Component: Untriaged → WebExtensions
Product: Firefox → Toolkit
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?
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.
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.
Summary: exclude_globs and exclude_matches not working in Webextensions manifest → globs are not supported as MatchPatterns
(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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [berlin]
Assignee: nobody → evilpies
Attachment #8730177 - Flags: review?(wmccloskey)
Whiteboard: [berlin] → [berlin][good first bug]
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.
Attachment #8730177 - Flags: review?(wmccloskey) → review?(kmaglione+bmo)
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 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`
Attachment #8730177 - Flags: review?(kmaglione+bmo)
Great. Adding the manifest test first would have caught the options bug ^^
Attachment #8730177 - Attachment is obsolete: true
Attachment #8730676 - Flags: review?(kmaglione+bmo)
Comment on attachment 8730676 [details] [diff] [review]
v2 Implement include_globs/exclude_globs

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

Looks good. Thanks.
Attachment #8730676 - Flags: review?(kmaglione+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/5948b646910e
https://hg.mozilla.org/mozilla-central/rev/e1897f0b4eb3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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!
Flags: needinfo?(evilpies)
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!
Flags: needinfo?(evilpies)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.