21Naown Reporter
	Description • 7 years ago

If you are not restricted by Chrome's specs, what about add a filter similarly to https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/onBeforeRequest#Parameters ?
To raise only event(s) we need.

	Caitlin Neiman [:caitmuenster]
	Comment 1 • 7 years ago

Hi @21Naown, this has been added to the agenda for the May 9 WebExtensions triage meeting. Would you be able to join us? 

Wiki: https://wiki.mozilla.org/Add-ons/Contribute/Triage

Agenda: https://docs.google.com/document/d/1q7UD3DxsT5z0lO3EbOyF9Iln32Wg_e9LYMdHZ80BEb4/edit#heading=h.34n4lirhljve

	21Naown Reporter
	Comment 2 • 7 years ago

Hello, I will not be able to join the meeting you mentioned. Thank you for proposing to me!

	Andy McKay
	Comment 3 • 7 years ago

In the tabs.onUpdated event you can examine the tabs and decide which tab to process and which tab to ignore. It might make it a few lines of code shorter, perhaps, but then there's a whole filter syntax we have to maintain long term when a few lines of JS would work.

It might save a little bit of time because the WebExtension code no longer needs to call the extension. But that seems like a very minimal win for something that doesn't get called as much as, say the webRequest API.

Since this can already be done and doesn't offer any performance benefit but does increase code maintainence this doesn't seem worth it.

	Kris Maglione [:kmag]
	Comment 4 • 7 years ago

This will actually help us a lot with performance, since we wind up sending huge numbers of these events even when most of them aren't needed, so I'd like us to implement it.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 6 • 6 years ago

Comment on attachment 8955375 [details]
Bug 1329507 add filtering to tabs.onUpdated,

I haven't written tests for the new filtering, but the existing onUpdated tests pass.  Just getting feedback.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 7 • 6 years ago

tabs.onUpdated has come up a few times recently, so I decided to give it a quick spin.

	Kris Maglione [:kmag]
	Comment 8 • 6 years ago
mozreview-review

Comment on attachment 8955375 [details]
Bug 1329507 add filtering to tabs.onUpdated,

https://reviewboard.mozilla.org/r/224554/#review230508

::: browser/components/extensions/ext-tabs.js:98
(Diff revision 1)
> +        if (!perms.overlapsAll(filter.urls)) {
> +          Cu.reportError("The proxy.addListener filter doesn't overlap with host permissions.");
> +        }

`proxy.addListener`?

In any case, the overlapping permissions check is not necessary. All that matters here is that they have the "tabs" permission.

::: browser/components/extensions/ext-tabs.js:103
(Diff revision 1)
> +        if (!perms.overlapsAll(filter.urls)) {
> +          Cu.reportError("The proxy.addListener filter doesn't overlap with host permissions.");
> +        }
> +      }
> +      let needsModified = true;
> +      if (!filter.events) {

These aren't events. They're properties.

::: browser/components/extensions/ext-tabs.js:105
(Diff revision 1)
> +        }
> +      }
> +      let needsModified = true;
> +      if (!filter.events) {
> +        // Default is to listen for all events.
> +        filter.events = [

This should be a `Set`.

::: browser/components/extensions/ext-tabs.js:118
(Diff revision 1)
> +          "sharingState",
> +          "status",
> +          "title",
> +        ];
> +      } else {
> +        let allAttrs = ["audible", "favIconUrl", "mutedInfo", "sharingState", "title"];

This should be a globally-declared `Set`.

::: browser/components/extensions/ext-tabs.js:177
(Diff revision 1)
> +        if (filter.tabId != null && tab.id != filter.tabId) {
> +          return;
> +        }
> +        if (filter.windowId != null && tab.windowId != filter.windowId) {
> +          return;
> +        }

There should be an `if (filter)` around this. The overhead will add up in the cases where we don't have a filter.

	Kris Maglione [:kmag]
	Comment 10 • 6 years ago
mozreview-review

Comment on attachment 8955375 [details]
Bug 1329507 add filtering to tabs.onUpdated,

https://reviewboard.mozilla.org/r/224554/#review231416

::: browser/components/extensions/ext-tabs.js:102
(Diff revision 2)
>      return deferred.promise;
>    },
>  };
>  
> +const allAttrs = new Set(["audible", "favIconUrl", "mutedInfo", "sharingState", "title"]);
> +const allProperties = [

`allProperties = new Set(...)`

::: browser/components/extensions/ext-tabs.js:121
(Diff revision 2)
> +  constructor(context, eventName) {
> +    let {extension} = context;
> +    let {tabManager} = extension;
> +
> +    let register = (fire, filterProps) => {
> +      const restricted = ["url", "favIconUrl", "title"];

Let's make this a global Set

::: browser/components/extensions/ext-tabs.js:128
(Diff revision 2)
> +      let filter = {...filterProps};
> +      if (filter.urls) {
> +        filter.urls = new MatchPatternSet(filter.urls);
> +      }
> +      let needsModified = true;
> +      if (!filter.propertyNames) {

:

    if (filter.propertyNames) {
      needsModified = filter.propertyNames.some(p => allAttrs.has(p));
      filter.propertyNames = new Set(filter.propertyNames);
    } else {
      filter.propertyNames = allProperties;
    }

::: browser/components/extensions/ext-tabs.js:145
(Diff revision 2)
> +          if (hasTabs || !restricted.includes(prop)) {
> +            nonempty = true;
> +            result[prop] = changeInfo[prop];
> +          }
> +        }
> +        return [nonempty, result];

Just return null if empty, I think.

::: browser/components/extensions/ext-tabs.js:149
(Diff revision 2)
> +        }
> +        return [nonempty, result];
> +      }
> +
> +      function getWindowID(windowId) {
> +        if (windowId == WINDOW_ID_CURRENT) {

Nit: `===`

::: browser/components/extensions/ext-tabs.js:167
(Diff revision 2)
> +        if (filter.windowId != null && tab.windowId != getWindowID(filter.windowId)) {
> +          return false;
> +        }
> +        if (filter.urls) {
> +          let nativeTab = tabTracker.getTab(tab.id);
> +          return filter.urls.matches(nativeTab.linkedBrowser.currentURI);

We need to check whether the extension has permission to read this tab's URI, otherwise the filters can be used to figure out when specific sites are visited. Please use `tab.uri` here. That automatically checks permissions.

::: browser/components/extensions/schemas/tabs.json:321
(Diff revision 2)
> +            "description": "A list of URLs or URL patterns. Events that cannot match any of the URLs will be filtered out.",
> +            "optional": true,
> +            "items": { "type": "string" },
> +            "minItems": 1
> +          },
> +          "propertyNames": {

Can we just call this "properties"?

::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated_filter.js:24
(Diff revision 2)
> +      browser.tabs.onUpdated.addListener((tabId, changeInfo) => {
> +        browser.test.log(`got onUpdated ${JSON.stringify(changeInfo)}`);
> +        if (changeInfo.status === "complete") {
> +          browser.test.notifyPass("onUpdated");
> +        }
> +      }, {urls: ["*://mochi.test/*"]});

Please also test that an extension without the tabs permission doesn't get events when it has a URL filter unless it has the activeTab permission for the tab being changed.

::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated_filter.js:25
(Diff revision 2)
> +        browser.test.log(`got onUpdated ${JSON.stringify(changeInfo)}`);
> +        if (changeInfo.status === "complete") {
> +          browser.test.notifyPass("onUpdated");
> +        }
> +      }, {urls: ["*://mochi.test/*"]});
> +      browser.test.sendMessage("ready");

Not necessary as long as the background script's initialization does any async work. `startup()` doesn't resolve until the top-level of the background script has finished executing.

::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated_filter.js:59
(Diff revision 2)
> +    background() {
> +      browser.tabs.onUpdated.addListener((tabId, changeInfo) => {
> +        if (changeInfo.status === "complete") {
> +          browser.test.notifyPass("onUpdated");
> +        }
> +      });

Please test this with an actual tab ID. I guess that means opening a tab, getting its ID, adding the listener, and updating the tab.

Also, I winder if we should add some code to automatically cull listeners when the tab or window it has filters for closes...

::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated_filter.js:94
(Diff revision 2)
> +    background() {
> +      browser.tabs.onUpdated.addListener((tabId, changeInfo) => {
> +        if (changeInfo.status === "complete") {
> +          browser.test.notifyPass("onUpdated");
> +        }
> +      }, {windowId: browser.windows.WINDOW_ID_CURRENT});

We should probably test that this works with an explicit window ID, too.

::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated_filter.js:129
(Diff revision 2)
> +        if (Object.keys(changeInfo).some(p => properties.has(p))) {
> +          browser.test.fail(`received unexpected onUpdated event ${JSON.stringify(changeInfo)}`);
> +        }

We should probably just check that Object.keys(changeInfo) is always ["status"]

	Pulsebot
	Comment 13 • 6 years ago

Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2c03c9417c77
add filtering to tabs.onUpdated, r=kmag

	Raul Gurzau (:RaulG)
	Comment 14 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2c03c9417c77

	21Naown Reporter
	Comment 15 • 6 years ago

Thank you very much!

	marius.santa
	Comment 16 • 6 years ago

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 17 • 6 years ago

has tests.

	Will Bamberg [:wbamberg]
	Comment 18 • 6 years ago

I've updated the docs page for this, and added some examples: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/onUpdated

Please let me know if we need anything else here.

A few things came up:

* the "properties" filter wants "isarticle" (https://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/tabs.json#300) but the actual property name is "isArticle" (https://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/tabs.json#). This works but it seems inconsistent.

* I wanted to make the page clearer about exactly which changes cause this event to be fired. Is it basically: changes to any property that can exist in changeInfo? There are some properties in tabs.Tab that are not in changeInfo, but I don't really understand why they are not. For example, isInReaderMode, height, width.

* it looks as if "changeInfo" in tabs.json (https://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/tabs.json#1398) would give me the list of properties that can appear in the changeInfo argument. But this omits a few properties which do appear, namely title, hidden, isArticle.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 19 • 6 years ago

(In reply to Will Bamberg [:wbamberg] from comment #18)
> I've updated the docs page for this, and added some examples:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/onUpdated
> 
> Please let me know if we need anything else here.
> 
> A few things came up:
> 
> * the "properties" filter wants "isarticle"

unfortunate, but not a big deal.  bug 1461695

> * I wanted to make the page clearer about exactly which changes cause this
> event to be fired. Is it basically: changes to any property that can exist
> in changeInfo? There are some properties in tabs.Tab that are not in
> changeInfo, but I don't really understand why they are not. For example,
> isInReaderMode, height, width.

Some of these it's expected to just get it off the tab.  reader mode will fire a url change iirc.  the tests are checking the tab url is about:reader.

> * it looks as if "changeInfo" in tabs.json

bug 1461693