Add MatchPattern support to WebRequest module

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla41
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 8614968 [details] [diff] [review]
patch

This patch adds support for Chrome's match patterns [1] rather than regexps for URL filtering, which is what we're using now. I also fixed a random bug.

[1] https://developer.chrome.com/extensions/match_patterns
Attachment #8614968 - Flags: review?(dtownsend)
Comment on attachment 8614968 [details] [diff] [review]
patch

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

This really needs some MatchPattern tests before it should land.

::: toolkit/modules/addons/MatchPattern.jsm
@@ +9,5 @@
> +const PERMITTED_SCHEMES = ["http", "https", "file", "ftp"];
> +
> +// This function converts a glob pattern (containing * and possibly ?
> +// as wildcards) to a regular expression.
> +function globToRegexp(pat, allowQuestion)

allowQuestion is never true. Do we need it?

@@ +20,5 @@
> +  } else {
> +    pat = pat.replace(/\?/g, "\\?");
> +  }
> +  pat = pat.replace(/\*/g, ".*");
> +  return new RegExp(pat);

new RegExp("^" + pat + "$")

@@ +34,5 @@
> +    this.path = new RegExp('.*');
> +  } else if (!pat) {
> +    this.scheme = [];
> +  } else {
> +    let re = new RegExp("^(http|https|file|ftp|\\*):\\/\\/(\\*|\\*\\.[^*/]+|[^*/]+)/(.*)$");

You don't need to escape the / characters in here

@@ +42,5 @@
> +      this.scheme = [];
> +    }
> +
> +    if (match[1] == '*') {
> +      this.scheme = PERMITTED_SCHEMES;

This doesn't match Chrome's behaviour which only allows https or http for '*'

@@ +46,5 @@
> +      this.scheme = PERMITTED_SCHEMES;
> +    } else {
> +      this.scheme = [match[1]];
> +    }
> +    this.host = globToRegexp(match[2], false);

The alternative to generating a full regexp for the host is that it's always either an exact string match or a suffix match. Maybe not worth changing since you had to write this code for the path case anyway.

@@ +57,5 @@
> +    if (this.scheme.indexOf(uri.scheme) == -1) {
> +      return false;
> +    }
> +
> +    if (!this.host.exec(uri.host)) {

The Chrome docs aren't specific about what to do in the case of ports being present. Maybe hostPort is a better choice here?

@@ +61,5 @@
> +    if (!this.host.exec(uri.host)) {
> +      return false;
> +    }
> +
> +    if (!this.path.exec(uri.path)) {

Here and about, test is quicker.

@@ +81,5 @@
> +  }
> +}
> +
> +MatchPattern.prototype = {
> +  matches(uri) {

Better add a note that this expects an nsIURI

@@ +86,5 @@
> +    for (let matcher of this.matchers) {
> +      if (matcher.matches(uri)) {
> +        return true;
> +      }
> +      return false;

This is only checking the first matcher!
Attachment #8614968 - Flags: review?(dtownsend) → review-
Created attachment 8617604 [details] [diff] [review]
patch v2

Thanks Dave! The allowQuestion bit is to support glob patterns:
https://developer.chrome.com/extensions/content_scripts#match-patterns-globs
I haven't actually implemented this yet, but I'd like to do so. It seems like the code is worth keeping.
Attachment #8614968 - Attachment is obsolete: true
Attachment #8617604 - Flags: review?(dtownsend)
Attachment #8617604 - Flags: review?(dtownsend) → review+
https://hg.mozilla.org/mozilla-central/rev/f8f4a6c06549
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Keywords: dev-doc-needed
I've updated the examples in WebRequest.jsm to use MatchPatterns, and added a little section on how to construct match patterns, and pointed to the match patterns doc for details of the string syntax.

I'll mark this dev-doc-complete, but let me know if you see anything else that needs adding.
Flags: needinfo?(wmccloskey)
Keywords: dev-doc-needed → dev-doc-complete
Thanks!
Flags: needinfo?(wmccloskey)
You need to log in before you can comment on or make changes to this bug.