1242522 - Implement webNavigation event filtering

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

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

The webNavigation API addListener, as described in the "Filtered events" of the "events"[1] doc page, should support "url filters" as an optional parameter (which should contains an array of "UrlFilter"[2] objects as value of its "url" property):

    let urlFilter = {
      url: [{ hostSuffix: 'example.net', pathContains: '/subpath/' }, ...]
    };
    chrome.webNavigation.onDOMContentLoaded.addListener((...) => {
      ...
    }, urlFilter);


[1]: https://developer.chrome.com/extensions/events 
[2]: https://developer.chrome.com/extensions/events#type-UrlFilter

Comment 1

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

I don't think we should implement this. The Chrome developers told me that no one really uses it, and it seems extremely complex.

Comment 2

[Andy McKay]

What do you think we should do here luca?

Comment 3

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

(In reply to Andy McKay [:andym] from comment #2)
> What do you think we should do here luca?

I definitely agree with Bill on this:

The usage of this feature seems to be pretty low indeed, and it seems that
filters have been introduced as an "optimization for event pages", e.g. looking into the
"Event Pages - Best Practices when using event pages"[1] section from the Chrome Extensions docs
for event pages:

   ...
   4. Use event filters to restrict your event notifications to the cases you care about.
      For example, if you listen to the tabs.onUpdated event, try using the
      webNavigation.onCompleted event with filters instead (the tabs API does not support filters).
      That way, your event page will only be loaded for events that interest you.
   ...

As we do not currently support event pages (which are basically non-persistent background pages), 
then implementing a feature to optimize them doesn't make sense neither, so I opened this issue 
mostly to check what a good cross-browser best practice could be here 
(and if new documentation can be helpful, then plan additions to the mdn doc pages with Will).

Here is how I'd like to proceed on this:

- do not implement this feature (given the above reasons)

- find and document the best cross-browser strategy (e.g. how Firefox reacts to the presence of the
  filter parameter? how is better to write the event handler to support both Chromium and Firefox with the same add-on js sourcecode?)

- close the issue (and re-evaluate it in a second time if we decide to implement the "event pages" feature).

[1]: https://developer.chrome.com/extensions/event_pages#best-practices

Comment 5

[nico.schloemer]

I'm using urlFilters in a Chrome extension [1] and stumbled upon this issue here when trying to translate the extension to Firefox.

Chrome's example page [2] recommends to replace
```
if (hasHostSuffix(e.url, 'google.com') ||
            hasHostSuffix(e.url, 'google.com.au')) {

```
by url filters
> which makes the listening code more declarative and efficient - an event page page need not be woken up to handle events it doesn't care about.

I'm using a persistent background page for now, so for efficiency I guess it doesn't really matter now; it's just a code clarity thing.

Having filters implemented in FF, even when just being transpiled to the above `if (hasHostSuffix...)` snippet, would have saved me some trouble here.


[1] https://github.com/paperhive/paperhive-chrome-extension/blob/master/src/scripts/background.js#L184
[2] https://developer.chrome.com/extensions/events#filtered

Comment 6

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

Attachment: Bug 1242522 - [webext] Add WebNavigation's UrlFilter to the json schema.

web_navigation.json was using a custom "filter" attribute which seems to add an implicit
optional extra parameter.

As we already support a similar feature for the web_request.json, which declares the
optional event filter as an explicit "extraParameters", which is already supported,
this patch adds the "events.UrlFilter" definition and turns all the "filter" attributes
into "extraParameters".

Review commit: https://reviewboard.mozilla.org/r/47157/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47157/

Comment 7

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

Attachment: Bug 1242522 - [webext] Implement optional UrlFilter on WebNavigation event listeners.

Review commit: https://reviewboard.mozilla.org/r/47159/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47159/

Comment 8

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

Attachment: Bug 1242522 - [webext] Test UrlFilter on WebNavigation event listeners.

Review commit: https://reviewboard.mozilla.org/r/47161/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47161/

Comment 9

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

Comment on attachment 8742366 [details]
Bug 1242522 - [webext] Test UrlFilter on WebNavigation event listeners.

This patch adds the webnavigations tests for the event filtering, it is still incomplete (more scenarios have to be added), but in the mean time a preliminary feedback on it can be useful.

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8742364 [details]
Bug 1242522 - [webext] Add WebNavigation's UrlFilter to the json schema.

https://reviewboard.mozilla.org/r/47157/#review43977

::: toolkit/components/extensions/schemas/web_navigation.json:21
(Diff revision 1)
>          }]
>        }
>      ]
>    },
>    {
> +    "namespace": "events",

This should go in events.json

::: toolkit/components/extensions/schemas/web_navigation.json:131
(Diff revision 1)
> +            "description": "Matches if the port of the URL is contained in any of the specified port lists. For example <code>[80, 443, [1000, 1200]]</code> matches all requests on port 80, 443 and in the range 1000-1200.",
> +            "optional": true,
> +            "items": {
> +              "choices": [
> +                {"type": "integer", "description": "A specific port."},
> +                {"type": "array", "items": {"type": "integer"}, "description": "A pair of integers identiying the start and end (both inclusive) of a port range."}

This should have `"minItems": 2, "maxItems": 2`

::: toolkit/components/extensions/schemas/web_navigation.json:155
(Diff revision 1)
>          "id": "TransitionQualifier",
>          "type": "string",
>          "enum": ["client_redirect", "server_redirect", "forward_back", "from_address_bar"]
> +      },
> +      {
> +        "id": "UrlFilter",

Can we call this something like "Filters"?

::: toolkit/components/extensions/schemas/web_navigation.json:288
(Diff revision 1)
>              }
>            }
> +        ],
> +        "extraParameters": [
> +          {
> +            "name": "filter",

"filters"

::: toolkit/components/extensions/schemas/web_navigation.json:291
(Diff revision 1)
> +        "extraParameters": [
> +          {
> +            "name": "filter",
> +            "$ref": "UrlFilter",
> +            "description": "Conditions that the URL being navigated to must satisfy. The 'schemes' and 'ports' fields of UrlFilter are ignored for this event."
> +          }

Shouldn't this be optional?

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8742365 [details]
Bug 1242522 - [webext] Implement optional UrlFilter on WebNavigation event listeners.

https://reviewboard.mozilla.org/r/47159/#review43979

::: toolkit/components/extensions/ext-webNavigation.js:78
(Diff revision 1)
>  }
>  
>  // Similar to WebRequestEventManager but for WebNavigation.
>  function WebNavigationEventManager(context, eventName) {
>    let name = `webNavigation.${eventName}`;
> -  let register = callback => {
> +  let register = (callback, {url: filters} = {}) => {

The destructuring and default value in the arguments is a bit hard to read. Can we please move the destrucutring into the body?

::: toolkit/modules/addons/MatchPattern.jsm:211
(Diff revision 1)
> +    let uri = NetUtil.newURI(url);
> +    let uriURL;
> +    try {
> +      uriURL = uri.QueryInterface(Components.interfaces.nsIURL);
> +    } catch (e) {
> +      // some schemes doesn't support QueryInterface(nsIURL) (e.g. about)

Please always capitalize comments.

"Some schemes don't implement nsIURL"

::: toolkit/modules/addons/MatchPattern.jsm:232
(Diff revision 1)
> +          return uri.port;
> +        } catch (e) {
> +          // 'uri.port' throws an exception with some uri schemes (e.g. about)
> +          return null;
> +        }
> +      },

These getters are expensive. Can we just store the results instead?

::: toolkit/modules/addons/MatchPattern.jsm:238
(Diff revision 1)
> +      url: uri.spec,
> +      path: uriURL.filePath,
> +      query: uriURL.query,
> +    };
> +
> +    // If any of the filter matches, matches returns true.

*filters

::: toolkit/modules/addons/MatchPattern.jsm:245
(Diff revision 1)
> +      if (this.matchURLFilter({filter, data})) {
> +        return true;
> +      }
> +    }
> +
> +    return false;

You could also do `return this.filters.some(filter => this.matchURLFilter({filter, data})`

::: toolkit/modules/addons/MatchPattern.jsm:247
(Diff revision 1)
> +      }
> +    }
> +
> +    return false;
> +  },
> +  matchURLFilter({filter, data}) {

Please add empty line.

::: toolkit/modules/addons/MatchPattern.jsm:294
(Diff revision 1)
> +
> +    // UrlMatches should test with a RE2 string the url without the ref
> +    // components.
> +    if (filter.urlMatches) {
> +      let url = data.uri.specIgnoringRef;
> +      if (!url || !url.match(filter.urlMatches)) {

`!url` will never be true here.

::: toolkit/modules/addons/MatchPattern.jsm:302
(Diff revision 1)
> +    }
> +
> +    // UrlMatches should test with a RE2 string the url without the query and
> +    // the ref components.
> +    if (filter.urlAndPathMatches) {
> +      let url = data.uri.resolve(data.path);

This is confusing. `data.path` is the same as `uri.filePath` rather than `uri.path`, which makes me read this as an error.

::: toolkit/modules/addons/MatchPattern.jsm:311
(Diff revision 1)
> +    }
> +
> +    // ports test for exact port matching or included in a range of ports.
> +    if (filter.ports) {
> +      // Return false if at least of scheme is verified
> +      let port = data.port === -1 ?

I'd find this easier to read without the ternery.

    let port = data.port;
    if (port === -1) {
        port = this.getDefaultPort(scheme);
    }

::: toolkit/modules/addons/MatchPattern.jsm:313
(Diff revision 1)
> +    // ports test for exact port matching or included in a range of ports.
> +    if (filter.ports) {
> +      // Return false if at least of scheme is verified
> +      let port = data.port === -1 ?
> +            this.getDefaultPortForScheme(data.uri.scheme) : data.port;
> +      let matchPort = filter.ports.some((filterPort) => {

`return filter.ports.some(...`

::: toolkit/modules/addons/MatchPattern.jsm:316
(Diff revision 1)
> +      let port = data.port === -1 ?
> +            this.getDefaultPortForScheme(data.uri.scheme) : data.port;
> +      let matchPort = filter.ports.some((filterPort) => {
> +        if (Array.isArray(filterPort)) {
> +          let [lower, upper] = filterPort;
> +          if (port >= lower &&

`return port >= lower && port <= upper`

::: toolkit/modules/addons/MatchPattern.jsm:334
(Diff revision 1)
> +      }
> +    }
> +
> +    return true;
> +  },
> +  getDefaultPortForScheme(scheme) {

Please add blank line.

::: toolkit/modules/addons/MatchPattern.jsm:335
(Diff revision 1)
> +    }
> +
> +    return true;
> +  },
> +  getDefaultPortForScheme(scheme) {
> +    switch (scheme) {

`Services.io.getProtocolHandler(scheme).defaultPort`

::: toolkit/modules/addons/MatchPattern.jsm:344
(Diff revision 1)
> +      return 443;
> +    default:
> +      return -1;
> +    }
> +  },
> +  serialize() {

Please add blank line.

::: toolkit/modules/addons/WebNavigation.jsm:54
(Diff revision 1)
>      }
>      let listeners = this.listeners.get(type);
>      listeners.add(listener);
> +    if (filter) {
> +      this.filters.set(listener, filter);
> +    }

Can we just make `listeners` a map, with the listener as the key, and the filter in the value?

Comment 12

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/47161/#review43989

::: toolkit/components/extensions/test/mochitest/test_ext_webnavigation_filters.html:74
(Diff revision 1)
> +
> +  let received = [];
> +  let completedResolve;
> +  let waitingURL, waitingEvent;
> +
> +  function loadAndWait(win, event, url, script) {

s/script/callback/

`win` also appears to be unused.

::: toolkit/components/extensions/test/mochitest/test_ext_webnavigation_filters.html:136
(Diff revision 1)
> +
> +  function* runTestScenario({event, url, filters}) {
> +   for (let {okFilter, failFilter} of filters) {
> +     extension.sendMessage("test-filter", "onCompleted", {url: okFilter}, {url: failFilter});
> +
> +     yield loadAndWait(win, "onCompleted", url, () => { win.location = url; });

Can we just set `win.location` and then call `waitForLoad` rather than `loadAndWait`?

Comment 13

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

Comment on attachment 8742364 [details]
Bug 1242522 - [webext] Add WebNavigation's UrlFilter to the json schema.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47157/diff/1-2/

Comment 14

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

Comment on attachment 8742365 [details]
Bug 1242522 - [webext] Implement optional UrlFilter on WebNavigation event listeners.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47159/diff/1-2/

Comment 15

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

Comment on attachment 8742366 [details]
Bug 1242522 - [webext] Test UrlFilter on WebNavigation event listeners.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47161/diff/1-2/

Comment 16

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

https://reviewboard.mozilla.org/r/47157/#review43977

> This should go in events.json

I moved this into its own schema file (events.json) and in its own bugzilla issue ([Bug 1242561](https://bugzilla.mozilla.org/show_bug.cgi?id=1242561))

> This should have `"minItems": 2, "maxItems": 2`

Added in the new events.json file (https://reviewboard.mozilla.org/r/47703/diff/1#2)

> Can we call this something like "Filters"?

Renamed to EventUrlFilters.

> Shouldn't this be optional?

added '"optional": true' to all the filters extraParameters.

Comment 17

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

https://reviewboard.mozilla.org/r/47157/#review44497

::: toolkit/components/extensions/schemas/web_navigation.json:172
(Diff revision 2)
> +        "extraParameters": [
> +          {
> +            "name": "filters",
> +            "optional": true,
> +            "$ref": "EventUrlFilters",
> +            "description": "Conditions that the URL being navigated to must satisfy. The 'schemes' and 'ports' fields of UrlFilter are ignored for this event."

All the webNavigation EventUrlFilters' description attributes says that:

- The 'schemes' and 'ports' fields of UrlFilter are ignored for this event.

I'm not currently ignoring the schemes and ports fields on the UrlFilter, 
I'm wondering: why we shouldn't support "schemes and ports" based filtering on the webNavigation events?

Comment 18

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

https://reviewboard.mozilla.org/r/47159/#review43979

> This is confusing. `data.path` is the same as `uri.filePath` rather than `uri.path`, which makes me read this as an error.

In the last pushed version I separated the data object (used to easily reuse the "Contains", "Prefix", "Suffix" "Equals" logic on the "host", "path", "query" and "url" parts), from our nsIURI and its nsIURL interface.
It should help to read it more easily.

Comment 19

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

Comment on attachment 8742365 [details]
Bug 1242522 - [webext] Implement optional UrlFilter on WebNavigation event listeners.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47159/diff/2-3/

Comment 20

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

Comment on attachment 8742366 [details]
Bug 1242522 - [webext] Test UrlFilter on WebNavigation event listeners.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47161/diff/2-3/

Comment 21

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

Comment on attachment 8742364 [details]
Bug 1242522 - [webext] Add WebNavigation's UrlFilter to the json schema.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47157/diff/2-3/

Comment 22

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

Comment on attachment 8742365 [details]
Bug 1242522 - [webext] Implement optional UrlFilter on WebNavigation event listeners.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47159/diff/3-4/

Comment 23

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

Comment on attachment 8742366 [details]
Bug 1242522 - [webext] Test UrlFilter on WebNavigation event listeners.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47161/diff/3-4/

Comment 24

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

https://reviewboard.mozilla.org/r/47161/#review43989

> Can we just set `win.location` and then call `waitForLoad` rather than `loadAndWait`?

The `loadAndWait` helper comes from the other existent webnavigation mochitest-plain test file (`test_ext_webnavigation.html`), but besides the above notes I didn't like too much how I was using it to test the event filters.

In the last pushed version I've reworked the extension which I'm using to orchestrate the tests on the various filtering scenarios and now the loadAndWait method is not needed anymore (and I've completely removed it from the last version for the `test_ext_webnavigation_filters.html`).

Comment 25

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

https://reviewboard.mozilla.org/r/47155/#review44935

::: toolkit/components/extensions/test/mochitest/test_ext_webnavigation_filters.html:219
(Diff revisions 3 - 4)
> +  // TODO: add additional specific tests for the other webNavigation events:
> +  // onErrorOccurred, onHistoryStateUpdated, onReferenceFragmentUpdated
> +  // (and onCreatedNavigationTarget on supported)

In the last version of this test case, all the available filters should now be provided of their own test scenario, for most of the common webnavigation events.

I'm going to cover the remaining ones asap.

::: toolkit/modules/addons/MatchPattern.jsm:13
(Diff revisions 3 - 4)
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
>                                    "resource://gre/modules/NetUtil.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",

Fixed missing Services (used by the "port"-based filtering).

::: toolkit/modules/addons/MatchPattern.jsm:307
(Diff revisions 3 - 4)
>      }
>  
> -    // ports test for exact port matching or included in a range of ports.
> -    if (filter.ports) {
> -      // Return false if at least of scheme is verified
> -      let port = data.port;
> +    return true;
> +  },
> +
> +  testMatchOnURLComponent({urlComponent: key, data, filter}) {

I moved the code that handle the "Contains/Equals/Suffix/Prefix" filtering into a new helper method (which reduces the method complexity and makes eslint more "happy" about it).

Comment 26

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

(In reply to Luca Greco [:rpl] from comment #23)
> Comment on attachment 8742366 [details]
> MozReview Request: Bug 1242522 - [webext] Test UrlFilter on WebNavigation
> event listeners. r?kmag
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/47161/diff/3-4/

I'm in the process of preparing more tests around this change:

- an xpcshell test in "toolkit/modules/test/xpcshell" to fastly test more "MatchURLFilters" scenarios 
- a mochitest-plain (or a mochitest-chrome) that errors related to the schema validation of the filters are logged as expected
- test the filters applied on the remaining WebNavigation events (onHistoryStateUpdates, onReferenceFragmentUpdated, onErrorOccurred)

Comment 27

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

Attachment: Bug 1242522 - [webext] Import missing events API schema file.

Review commit: https://reviewboard.mozilla.org/r/48723/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48723/

Comment 28

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

Attachment: Bug 1242522 - [webext] Add MatchURLFilters xpcshell-test.

Review commit: https://reviewboard.mozilla.org/r/48725/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48725/

Comment 29

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

Comment on attachment 8742364 [details]
Bug 1242522 - [webext] Add WebNavigation's UrlFilter to the json schema.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47157/diff/3-4/

Comment 30

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

Comment on attachment 8742365 [details]
Bug 1242522 - [webext] Implement optional UrlFilter on WebNavigation event listeners.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47159/diff/4-5/

Comment 31

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

Comment on attachment 8742366 [details]
Bug 1242522 - [webext] Test UrlFilter on WebNavigation event listeners.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47161/diff/4-5/

Comment 32

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

In the last push to mozreview:

- moved from Bug 1242561 the patch that adds the events.json schema
- added more scenarios into the mochitest-plain test_ext_webNavigation_filtering.html
  (test coverage of the onReferenceFragmentUpdated, onHistoryStateUpdated scenarios)
- added xpcshell-test which test unit the MatchURLFilter helper with more scenarios
- fix MatchURLFilters accordingly to the issues found using the new xpcshell-test
  (e.g. handling empty filters, empty string in url based filters and handling of special schemes
  which cannot be loaded from a mochitest-plain test)

Comment 33

[Michail Frolov]

We're also using this parameter in our internal (unpublished) Chrome extension.  Converting to Firefox WebExtensions currently breaks on webNavigation.onDOMContentLoaded.addListener with filter parameter set to:

{ url: [{ urlEquals: "<my url>" }] }

with error: "Incorrect argument types for webNavigation.onDOMContentLoaded."

The first parameter is function(details){}

I am using nightly 49.0a1 (2016-04-27)

Comment 34

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

Comment on attachment 8744910 [details]
Bug 1242522 - [webext] Import missing events API schema file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48723/diff/1-2/

Comment 35

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

Comment on attachment 8742364 [details]
Bug 1242522 - [webext] Add WebNavigation's UrlFilter to the json schema.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47157/diff/4-5/

Comment 36

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

Comment on attachment 8742365 [details]
Bug 1242522 - [webext] Implement optional UrlFilter on WebNavigation event listeners.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47159/diff/5-6/

Comment 37

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

Comment on attachment 8742366 [details]
Bug 1242522 - [webext] Test UrlFilter on WebNavigation event listeners.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47161/diff/5-6/

Comment 38

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

Comment on attachment 8744911 [details]
Bug 1242522 - [webext] Add MatchURLFilters xpcshell-test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48725/diff/1-2/

Comment 39

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

The last push is a rebase (on a recent tip) of the patches as already submitted to mozreview.

Comment 40

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/47157/#review51272

Comment 41

[Kris Maglione [:kmag]]

Comment on attachment 8742365 [details]
Bug 1242522 - [webext] Implement optional UrlFilter on WebNavigation event listeners.

https://reviewboard.mozilla.org/r/47159/#review45275

::: toolkit/modules/addons/MatchPattern.jsm:211
(Diff revision 4)
> +    }
> +
> +    let uri = NetUtil.newURI(url);
> +    let uriURL;
> +    try {
> +      uriURL = uri.QueryInterface(Components.interfaces.nsIURL);

This object isn't necessary. Just `QueryInterface` `uri`. You can even avoid the try-catch if you just do `uri instanceof Ci.nsIURL`

::: toolkit/modules/addons/MatchPattern.jsm:222
(Diff revision 4)
> +    let host;
> +    try {
> +      host = uri.host;
> +    } catch (e) {
> +      // 'uri.host' throws an exception with some uri schemes (e.g. about).
> +      host = null;

Just initialize to null above. Same for the others.

::: toolkit/modules/addons/MatchPattern.jsm:237
(Diff revision 4)
> +
> +    let data = {
> +      host,
> +      port,
> +      url: uri.spec,
> +      path: uriURL.filePath,

Can you please name this property `filePath` to avoid confusion?

::: toolkit/modules/addons/MatchPattern.jsm:199
(Diff revision 6)
>    },
>  };
> +
> +// Match WebNavigation URL Filters.
> +this.MatchURLFilters = function(filters) {
> +  if (filters && !Array.isArray(filters)) {

Is there any reason to accept null here?

::: toolkit/modules/addons/MatchPattern.jsm:200
(Diff revision 6)
>  };
> +
> +// Match WebNavigation URL Filters.
> +this.MatchURLFilters = function(filters) {
> +  if (filters && !Array.isArray(filters)) {
> +    throw Error("Invalid format: filters should be an array");

`throw new TypeError("`filters` must be an array")`

::: toolkit/modules/addons/MatchPattern.jsm:207
(Diff revision 6)
> +  this.filters = filters;
> +};
> +
> +MatchURLFilters.prototype = {
> +  matches(url) {
> +    if (!this.filters || this.filters.length == 0) {

Shouldn't a zero-length array always return false?

::: toolkit/modules/addons/MatchPattern.jsm:214
(Diff revision 6)
> +    }
> +
> +    let uri = NetUtil.newURI(url);
> +    let uriURL;
> +    try {
> +      uriURL = uri.QueryInterface(Components.interfaces.nsIURL);

This object isn't necessary. Just `QueryInterface` `uri`. You can even avoid the try-catch if you just do `uri instanceof Ci.nsIURL`

::: toolkit/modules/addons/MatchPattern.jsm:225
(Diff revision 6)
> +    let host;
> +    try {
> +      host = uri.host;
> +    } catch (e) {
> +      // 'uri.host' throws an exception with some uri schemes (e.g. about).
> +      host = "";

Just initialize to a null string above. Same for the others.

::: toolkit/modules/addons/MatchPattern.jsm:240
(Diff revision 6)
> +
> +    let data = {
> +      host,
> +      port,
> +      url,
> +      path: uriURL.filePath,

Can you please name this property `filePath` to avoid confusion?

::: toolkit/modules/addons/MatchPattern.jsm:259
(Diff revision 6)
> +      }
> +    }
> +
> +    // Test for exact port matching or included in a range of ports.
> +    if (filter.ports) {
> +      // Return false if at least of scheme is verified

I don't understand this comment. It doesn't seem accurate.

::: toolkit/modules/addons/MatchPattern.jsm:264
(Diff revision 6)
> +      // Return false if at least of scheme is verified
> +      let port = data.port;
> +      if (port === -1) {
> +        // NOTE: currently defaultPort for "resource" and "chrome" schemes defaults to -1,
> +        // for "about", "data" and "javascript" schemes defaults to undefined.
> +        port = ["resource", "chrome"].includes(uri.scheme) ?

Please expand this into a multi-line `if` statement.

::: toolkit/modules/addons/MatchPattern.jsm:287
(Diff revision 6)
> +      if (!this.testMatchOnURLComponent({urlComponent, data, filter})) {
> +        return false;
> +      }
> +    }
> +
> +    // urlMatches should test with a RE2 string the url without the ref

What is an "RE2 string"?

::: toolkit/modules/addons/MatchPattern.jsm:318
(Diff revision 6)
> +
> +  testMatchOnURLComponent({urlComponent: key, data, filter}) {
> +    // Test for equals.
> +    // NOTE: an empty string should not be considered
> +    // a filter to skip
> +    if (typeof filter[`${key}Equals`] == "string") {

Why do you need a `typeof` check here?

::: toolkit/modules/addons/WebNavigation.jsm:20
(Diff revision 6)
>  
>  // TODO:
>  // onCreatedNavigationTarget
>  
>  var Manager = {
> +  // type (string) -> WeakMap<listener,UrlFilter>

Map[string -> Map[listener -> URLFilter]]

::: toolkit/modules/addons/WebNavigation.jsm:152
(Diff revision 6)
>      for (let prop in extra) {
>        details[prop] = extra[prop];
>      }
>  
> -    for (let listener of listeners) {
> +    for (let [listener, filters] of listeners) {
> +      // Call the listener if the listener has not filter or if its filter matches.

s/not/no/

Comment 42

[Kris Maglione [:kmag]]

Comment on attachment 8742366 [details]
Bug 1242522 - [webext] Test UrlFilter on WebNavigation event listeners.

https://reviewboard.mozilla.org/r/47161/#review51274

Comment 43

[Kris Maglione [:kmag]]

Comment on attachment 8744911 [details]
Bug 1242522 - [webext] Add MatchURLFilters xpcshell-test.

https://reviewboard.mozilla.org/r/48725/#review51276

::: toolkit/modules/tests/xpcshell/test_MatchURLFilters.js:10
(Diff revision 2)
> +
> +Components.utils.import("resource://gre/modules/MatchPattern.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +function createTestFilter({url, filters})
> +{

Opening brace should always be on the same line as the last argument.

::: toolkit/modules/tests/xpcshell/test_MatchURLFilters.js:17
(Diff revision 2)
> +  return m.matches(url);
> +}
> +
> +function expectPass({url, filters})
> +{
> +  do_check_true(createTestFilter({url, filters}),

`ok(...)`

::: toolkit/modules/tests/xpcshell/test_MatchURLFilters.js:23
(Diff revision 2)
> +                `Expected match: ${JSON.stringify(filters)}, ${url}`);
> +}
> +
> +function expectFail({url, filters})
> +{
> +  do_check_false(createTestFilter({url, filters}),

`ok(!...)`

::: toolkit/modules/tests/xpcshell/test_MatchURLFilters.js:27
(Diff revision 2)
> +{
> +  do_check_false(createTestFilter({url, filters}),
> +                 `Expected no match: ${JSON.stringify(filters)}, ${url}`);
> +}
> +
> +function expectThrow({url, filters, exceptionMessageContains})

Please use `Assert.throws` instead.

::: toolkit/modules/tests/xpcshell/test_MatchURLFilters.js:43
(Diff revision 2)
> +    do_check_true(String(e).includes(exceptionMessageContains),
> +                  `Check received exception for expected message: ${JSON.stringify(logData)}`);
> +  }
> +}
> +
> +function run_test()

`add_task(...`

::: toolkit/modules/tests/xpcshell/test_MatchURLFilters.js:51
(Diff revision 2)
> +  // Pattern must include trailing slash.
> +  // Protocol not allowed.
> +  // Now try with * in the path.
> +  // Check path stuff.
> +  // Multiple patterns.
> +  const shouldPass = true,

One variable per var/let/const statement.

::: toolkit/modules/tests/xpcshell/test_MatchURLFilters.js:56
(Diff revision 2)
> +  const shouldPass = true,
> +        shouldFail = true,
> +        shouldThrow = true;
> +
> +  var testCases = [
> +    // empty, undefined and null filters

Please capitalize all comments, and end sentences with a full stop.

::: toolkit/modules/tests/xpcshell/test_MatchURLFilters.js:76
(Diff revision 2)
> +    {shouldPass, filters: [{schemes: ["ftp"]}], url: "ftp://fake/ftp/url"},
> +    {shouldPass, filters: [{schemes: ["about"]}], url: "about:blank"},
> +    {shouldPass, filters: [{schemes: ["data"]}], url: "data:,testDataURL"},
> +    {shouldPass, filters: [{schemes: ["javascript"]}], url: "javascript:fake"},
> +    {shouldPass, filters: [{schemes: ["resource"]}], url: "resource://fake/url"},
> +    {shouldPass, filters: [{schemes: ["chrome"]}], url: "chrome://fake/chrome/url"},

We shouldn't add tests for `chrome:`, `resource:`, or `javascript:`. Probably not for `about:` either. They should either be forbidden, or have undefined behavior.

::: toolkit/modules/tests/xpcshell/test_MatchURLFilters.js:94
(Diff revision 2)
> +    // port filter: schemes without a default port
> +    {shouldFail, filters: [{ports: [-1]}], url: "about:blank"},
> +    {shouldFail, filters: [{ports: [-1]}], url: "resource://fake/url"},
> +    {shouldFail, filters: [{ports: [-1]}], url: "chrome://fake/chrome/url"},
> +    {shouldFail, filters: [{ports: [-1]}], url: "data:,testDataURL"},
> +    {shouldFail, filters: [{ports: [-1]}], url: "javascript:fake"},

Same here, and below. No tests for `chrome:`, `resource:`, or `javascript:`

::: toolkit/modules/tests/xpcshell/test_MatchURLFilters.js:126
(Diff revision 2)
> +    {shouldPass, filters: [{hostPrefix: null}], url: "https://mozilla.org"},
> +
> +    {shouldPass, filters: [{hostSuffix: ".org"}], url: "https://mozilla.org"},
> +    {shouldFail, filters: [{hostSuffix: "moz"}], url: "https://mozilla.org"},
> +    {shouldPass, filters: [{hostSuffix: ""}], url: "https://mozilla.org"},
> +    {shouldPass, filters: [{hostSuffix: null}], url: "https://mozilla.org"},

Please test `hostEquals` and `hostSuffix` filters against URLs with ports.

::: toolkit/modules/tests/xpcshell/test_MatchURLFilters.js:131
(Diff revision 2)
> +    {shouldPass, filters: [{hostSuffix: null}], url: "https://mozilla.org"},
> +
> +    // TODO: the following chrome and resource urls has "fake" as the host part,
> +    // should be prevent it to be identified as a valid host by MatchURLFilters?
> +    {shouldPass, filters: [{hostEquals: "fake"}], url: "chrome://fake/chrome/url"},
> +    {shouldPass, filters: [{hostEquals: "fake"}], url: "resource://fake/url"},

No tests for these, please.

::: toolkit/modules/tests/xpcshell/test_MatchURLFilters.js:152
(Diff revision 2)
> +    {shouldPass, filters: [{pathEquals: null}], url: "https://mozilla.org/test/path"},
> +    {shouldPass, filters: [{pathEquals: "/test/path"}], url: "https://mozilla.org/test/path"},
> +    {shouldFail, filters: [{pathEquals: "/wrong/path"}], url: "https://mozilla.org/test/path"},
> +    // NOTE: trying at least once another valid protocol
> +    {shouldPass, filters: [{pathEquals: "/test/path"}], url: "ftp://mozilla.org/test/path"},
> +    {shouldFail, filters: [{pathEquals: "/wrong/path"}], url: "ftp://mozilla.org/test/path"},

Please test `pathEquals` and `pathSuffix` filters against URLs qith queries and ref fragments.

::: toolkit/modules/tests/xpcshell/test_MatchURLFilters.js:158
(Diff revision 2)
> +
> +    {shouldPass, filters: [{pathContains: "st/"}], url: "https://mozilla.org/test/path"},
> +    {shouldPass, filters: [{pathContains: "/test"}], url: "https://mozilla.org/test/path"},
> +    {shouldFail, filters: [{pathContains: "org"}], url: "https://mozilla.org/test/path"},
> +    {shouldPass, filters: [{pathContains: ""}], url: "https://mozilla.org/test/path"},
> +    {shouldPass, filters: [{pathContains: null}], url: "https://mozilla.org/test/path"},

Please test that these don't match strings that are only in queries or ref fragments.

::: toolkit/modules/tests/xpcshell/test_MatchURLFilters.js:174
(Diff revision 2)
> +
> +    // queryEquals, queryContains, queryPrefix, querySuffix
> +    {shouldFail, filters: [{queryEquals: ""}], url: "https://mozilla.org/?param=val"},
> +    {shouldPass, filters: [{queryEquals: null}], url: "https://mozilla.org/?param=val"},
> +    {shouldPass, filters: [{queryEquals: "param=val"}], url: "https://mozilla.org/?param=val"},
> +    {shouldFail, filters: [{queryEquals: "?param=val"}], url: "https://mozilla.org/?param=val"},

Please test these against URLs with paths other than "/"

::: toolkit/modules/tests/xpcshell/test_MatchURLFilters.js:241
(Diff revision 2)
> +    {shouldPass, filters: [{originAndPathMatches: ".*://mozilla"}],
> +     url: "https://mozilla.org/?p=v#ref"},
> +    {shouldPass, filters: [{originAndPathMatches: ".*://mozilla"}],
> +     url: "ftp://mozilla.org/?p=v#ref"},
> +    // NOTE: urlMatches should not match the url without the query and the ref.
> +    {shouldFail, filters: [{originAndPathMatches: ".*://.*/?p"}],

This needs to be "\\?" rather than just "?". Same for the ones above.

Comment 44

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

Comment on attachment 8744910 [details]
Bug 1242522 - [webext] Import missing events API schema file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48723/diff/2-3/

Comment 45

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

Comment on attachment 8742364 [details]
Bug 1242522 - [webext] Add WebNavigation's UrlFilter to the json schema.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47157/diff/5-6/

Comment 46

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

Comment on attachment 8742365 [details]
Bug 1242522 - [webext] Implement optional UrlFilter on WebNavigation event listeners.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47159/diff/6-7/

Comment 47

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

Comment on attachment 8742366 [details]
Bug 1242522 - [webext] Test UrlFilter on WebNavigation event listeners.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47161/diff/6-7/

Comment 48

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

Comment on attachment 8744911 [details]
Bug 1242522 - [webext] Add MatchURLFilters xpcshell-test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48725/diff/2-3/

Comment 49

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

https://reviewboard.mozilla.org/r/47159/#review45275

> Can you please name this property `filePath` to avoid confusion?

This is named `path` because it match the name of the components in the webNavigation URL filters, and it is currently used to match against it any `path*` url filter (e.g. `pathContains` etc.).

I'm not absolutely against to rename it, but it worth to notice that after renaming it the part that match the url components (e.g. lines from 281 to 285 and the `testMatchOnURLComponent` method) will need special handling for the path component (and currently the same block of code is reused to process all the `host`, `path`, `query` and `url` components filtering, because most of their logic is shared).

> Is there any reason to accept null here?

On Chromium the url filter can be null or undefined (and it is always verified, like when it is set to an empty array).

But on Firefox the url property is currently enforced by the schema (where it is configured as mandatory), and so I'm going to don't allow empty filters here.

> Shouldn't a zero-length array always return false?

zero-legth array filters are always verified on Chromium, and so I'm incline to keep the same behavior here (at least if we don't see any problem with it)

> What is an "RE2 string"?

That is an extraction of the chrome docs, I pasted it there to remember what that filter it is supposed to check.

The RE2 string refers to their regex library implementation (https://github.com/google/re2/wiki/Syntax)

I'm going to tweak the comment into:

```
// urlMatches is a regular expression string (based on the RE2 library syntaxes on
// Chromium) and it should be applied on the "url without the ref"
```

> Why do you need a `typeof` check here?

The reason is described in the inline comment above that line (which I hope that
is understandable and describe the reason of that typeof):
 
> // NOTE: an empty string should not be considered a filter to skip

Comment 50

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

https://reviewboard.mozilla.org/r/48725/#review51504

::: toolkit/modules/tests/xpcshell/test_MatchURLFilters.js:10
(Diff revision 2)
> +
> +Components.utils.import("resource://gre/modules/MatchPattern.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +function createTestFilter({url, filters})
> +{

ouch! I was following the coding style of the other MatchPattern test file in this directory, my apologies.

::: toolkit/modules/tests/xpcshell/test_MatchURLFilters.js:76
(Diff revision 2)
> +    {shouldPass, filters: [{schemes: ["ftp"]}], url: "ftp://fake/ftp/url"},
> +    {shouldPass, filters: [{schemes: ["about"]}], url: "about:blank"},
> +    {shouldPass, filters: [{schemes: ["data"]}], url: "data:,testDataURL"},
> +    {shouldPass, filters: [{schemes: ["javascript"]}], url: "javascript:fake"},
> +    {shouldPass, filters: [{schemes: ["resource"]}], url: "resource://fake/url"},
> +    {shouldPass, filters: [{schemes: ["chrome"]}], url: "chrome://fake/chrome/url"},

Actually the "chrome:", "data:" and "about:" schemes are supported (and they work as expected) on Chromium, and I'd like to keep the same behavior on Firefox for them (at least if we have no reasons to do otherwise).

But I'm going to remove the "javascript:" scheme from the tested scenarios (and leave it as an undefined behavior) because on Chromium it behaves differently from how it behaves on the other tested url schemes (and on Firefox it works differently from how it behaves the other url schemes and from how it behaves on Chrome).

::: toolkit/modules/tests/xpcshell/test_MatchURLFilters.js:94
(Diff revision 2)
> +    // port filter: schemes without a default port
> +    {shouldFail, filters: [{ports: [-1]}], url: "about:blank"},
> +    {shouldFail, filters: [{ports: [-1]}], url: "resource://fake/url"},
> +    {shouldFail, filters: [{ports: [-1]}], url: "chrome://fake/chrome/url"},
> +    {shouldFail, filters: [{ports: [-1]}], url: "data:,testDataURL"},
> +    {shouldFail, filters: [{ports: [-1]}], url: "javascript:fake"},

As wrote above:
I'm ok to remove "javascript:" from the tested cases, I'd like to keep "chrome:" scheme url supported because it is supported on both Firefox and Chrome and we can easily make Firefox to behave like Chromium here.

I'm a bit unsure about the "resource:" url scheme because it doesn't exist on chromium, but it is supported on Firefox and the webNavigation events related to this url scheme are received from the addon, so the only thing that we are going to prevent by removing its support from the MatchURLFilter helper is the possibility to filter out them.

::: toolkit/modules/tests/xpcshell/test_MatchURLFilters.js:131
(Diff revision 2)
> +    {shouldPass, filters: [{hostSuffix: null}], url: "https://mozilla.org"},
> +
> +    // TODO: the following chrome and resource urls has "fake" as the host part,
> +    // should be prevent it to be identified as a valid host by MatchURLFilters?
> +    {shouldPass, filters: [{hostEquals: "fake"}], url: "chrome://fake/chrome/url"},
> +    {shouldPass, filters: [{hostEquals: "fake"}], url: "resource://fake/url"},

I checked again this on Chromium and this is exactly how it works on it ("fake" becames the hostname of the "chrome:" url scheme)

Comment 51

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

Comment on attachment 8744911 [details]
Bug 1242522 - [webext] Add MatchURLFilters xpcshell-test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48725/diff/3-4/

Comment 52

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/47159/#review45275

> This is named `path` because it match the name of the components in the webNavigation URL filters, and it is currently used to match against it any `path*` url filter (e.g. `pathContains` etc.).
> 
> I'm not absolutely against to rename it, but it worth to notice that after renaming it the part that match the url components (e.g. lines from 281 to 285 and the `testMatchOnURLComponent` method) will need special handling for the path component (and currently the same block of code is reused to process all the `host`, `path`, `query` and `url` components filtering, because most of their logic is shared).

OK, I guess that makes sense.

> zero-legth array filters are always verified on Chromium, and so I'm incline to keep the same behavior here (at least if we don't see any problem with it)

I don't think that makes sense. Are there any add-ons that rely on this? Maybe we should just make the minimum length 1.

> That is an extraction of the chrome docs, I pasted it there to remember what that filter it is supposed to check.
> 
> The RE2 string refers to their regex library implementation (https://github.com/google/re2/wiki/Syntax)
> 
> I'm going to tweak the comment into:
> 
> ```
> // urlMatches is a regular expression string (based on the RE2 library syntaxes on
> // Chromium) and it should be applied on the "url without the ref"
> ```

We don't use the RE2 library. Let's just say regular expression.

> The reason is described in the inline comment above that line (which I hope that
> is understandable and describe the reason of that typeof):
>  
> > // NOTE: an empty string should not be considered a filter to skip

Can we just check `!= null` instead?

Comment 53

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/48725/#review51504

> Actually the "chrome:", "data:" and "about:" schemes are supported (and they work as expected) on Chromium, and I'd like to keep the same behavior on Firefox for them (at least if we have no reasons to do otherwise).
> 
> But I'm going to remove the "javascript:" scheme from the tested scenarios (and leave it as an undefined behavior) because on Chromium it behaves differently from how it behaves on the other tested url schemes (and on Firefox it works differently from how it behaves the other url schemes and from how it behaves on Chrome).

The `chrome:` scheme means something completely different in Firefox than it does in Chrome. In general, we don't allow extensions to access it. The same applies to about:, for the most part. I'm not sure what our behavior should be for them, in the case of web navigation events, so I'm not saying that we should remove support for them. I'm saying that at the moment, the behavior is undefined, and we shouldn't be testing undefined behavior.

Comment 54

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

Comment on attachment 8744910 [details]
Bug 1242522 - [webext] Import missing events API schema file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48723/diff/3-4/

Comment 55

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

Comment on attachment 8742364 [details]
Bug 1242522 - [webext] Add WebNavigation's UrlFilter to the json schema.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47157/diff/6-7/

Comment 56

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

Comment on attachment 8742365 [details]
Bug 1242522 - [webext] Implement optional UrlFilter on WebNavigation event listeners.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47159/diff/7-8/

Comment 57

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

Comment on attachment 8742366 [details]
Bug 1242522 - [webext] Test UrlFilter on WebNavigation event listeners.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47161/diff/7-8/

Comment 58

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

Comment on attachment 8744911 [details]
Bug 1242522 - [webext] Add MatchURLFilters xpcshell-test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48725/diff/4-5/

Comment 59

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

In the last pushed updates (https://reviewboard.mozilla.org/r/47155/diff/8-9/),
I've rebased the patches on a recent fx-team tip and applied the last suggestions from the review comments:

- removed all the test cases related to "chrome:", "resource:" urls [1]
- use `!= null` instead of `typeof ... === "string"` to be able to recognize the "empty string vs. null/undefined" scenarios
- set `minIndex = 1` in the url filter schema (and added a test which ensure that a reasonable error message is logged),
  so that an empty array is not an "always verified filter" anymore
- removed any reference to RE2 from the inline comments  

[1] I'm a bit worried about this, because even if the Chrome Extensions API reference doesn't currently explicitly document their behavior, they are currently supported (e.g. their webNavigation events are currently emitted on both Firefox and Chrome) and if the addon developers start to leverage this behavior to filter out them from the received events, by removing their test cases we are going to discover a regression only when the addon developers start to complain, which can be a lot of times after the regression is actually happened.

Comment 60

[Kris Maglione [:kmag]]

Comment on attachment 8744910 [details]
Bug 1242522 - [webext] Import missing events API schema file.

https://reviewboard.mozilla.org/r/48723/#review55504

Comment 61

[Kris Maglione [:kmag]]

Comment on attachment 8744911 [details]
Bug 1242522 - [webext] Add MatchURLFilters xpcshell-test.

https://reviewboard.mozilla.org/r/48725/#review55506

Comment 62

[Kris Maglione [:kmag]]

Comment on attachment 8742365 [details]
Bug 1242522 - [webext] Implement optional UrlFilter on WebNavigation event listeners.

https://reviewboard.mozilla.org/r/47159/#review55508

Comment 63

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

Comment on attachment 8744910 [details]
Bug 1242522 - [webext] Import missing events API schema file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48723/diff/4-5/

Comment 64

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

Comment on attachment 8742364 [details]
Bug 1242522 - [webext] Add WebNavigation's UrlFilter to the json schema.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47157/diff/7-8/

Comment 65

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

Comment on attachment 8742365 [details]
Bug 1242522 - [webext] Implement optional UrlFilter on WebNavigation event listeners.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47159/diff/8-9/

Comment 66

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

Comment on attachment 8742366 [details]
Bug 1242522 - [webext] Test UrlFilter on WebNavigation event listeners.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47161/diff/8-9/

Comment 67

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

Comment on attachment 8744911 [details]
Bug 1242522 - [webext] Add MatchURLFilters xpcshell-test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48725/diff/5-6/

Comment 68

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

Updated patches pushed on mozreview with the following trivial changes:
- rebased on a recent fx-team tip
- single line change to fix a small eslint error in the test case

Pushed to try:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ba9ca2ea99601e01a706124836623832ed62135

Comment 69

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/1fec68caafb1
[webext] Import missing events API schema file. r=kmag
https://hg.mozilla.org/integration/fx-team/rev/aa3410416e05
[webext] Add WebNavigation's UrlFilter to the json schema. r=kmag
https://hg.mozilla.org/integration/fx-team/rev/75ea4286197e
[webext] Implement optional UrlFilter on WebNavigation event listeners. r=kmag
https://hg.mozilla.org/integration/fx-team/rev/284703b2bb5f
[webext] Test UrlFilter on WebNavigation event listeners. r=kmag
https://hg.mozilla.org/integration/fx-team/rev/0786deb10eef
[webext] Add MatchURLFilters xpcshell-test. r=kmag

Comment 70

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1fec68caafb1
https://hg.mozilla.org/mozilla-central/rev/aa3410416e05
https://hg.mozilla.org/mozilla-central/rev/75ea4286197e
https://hg.mozilla.org/mozilla-central/rev/284703b2bb5f
https://hg.mozilla.org/mozilla-central/rev/0786deb10eef

Comment 71

[Will Bamberg [:wbamberg]]

I think all that's needed here is to update the compat data for webNavigation, and I have done that: https://github.com/mdn/browser-compat-data/pull/31/files.

I also had a go at improving the page on urlFilter a bit: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/events/UrlFilter.

But let me know if I need anything else!

Comment 72

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

Thanks Will!

In the MDN page linked above, we should remove any reference to RE2 (which is the regular expression implementation which is used internally in Chrome by the webNavigation event filtering) and change them into links to our own RegExp docs on MDN (which is what we use internally in our implementation)

About Chrome incompatibilities related to this part of the API, which, at least in my opinion, worth a mention:

On Chrome an empty array used as a webNavigation events filter behaves like an "always matched filter", on the contrary Firefox will raise an exception if the array for UrlFilters is empty, e.g.:

> Type error for parameter filters (Error processing url: 
> Array requires at least 1 items; you have 0) 
> for webNavigation.onCompleted.

Besides the above notes, the MDN doc page linked in Comment 71 looks great.

The updated compat data has been updated correctly (but, even it is not related to the UrlFilter but to the webNavigation API in general, I'm going to explicitly test the behavior of the webNavigation API on Android, so that we can add special notes related to its behavior on Android if needed).

Finally, I'm creating a new example for the "mdn/webextensions-examples" repo with usage of both the webNavigation API and the UrlFilter, as a companion of these docs:

- https://github.com/rpl/webextensions-examples/tree/webnav/navigation-stats

(which is going to become a pull request asap, so that we can review and discuss it there)

Comment 73

[Will Bamberg [:wbamberg]]

Thanks Luca!

(In reply to Luca Greco [:rpl] from comment #72)
> Thanks Will!
> 
> In the MDN page linked above, we should remove any reference to RE2 (which
> is the regular expression implementation which is used internally in Chrome
> by the webNavigation event filtering) and change them into links to our own
> RegExp docs on MDN (which is what we use internally in our implementation)
> 
> About Chrome incompatibilities related to this part of the API, which, at
> least in my opinion, worth a mention:
> 
> On Chrome an empty array used as a webNavigation events filter behaves like
> an "always matched filter", on the contrary Firefox will raise an exception
> if the array for UrlFilters is empty, e.g.:
> 
> > Type error for parameter filters (Error processing url: 
> > Array requires at least 1 items; you have 0) 
> > for webNavigation.onCompleted.
> 

Yes, we should document this, but which is the "standard" from which the other one deviates?

Comment 74

[Will Bamberg [:wbamberg]]

> > On Chrome an empty array used as a webNavigation events filter behaves like
> > an "always matched filter", on the contrary Firefox will raise an exception
> > if the array for UrlFilters is empty, e.g.:


I've added compat notes for this.