Closed Bug 1329507 Opened 3 years ago Closed 2 years ago

tabs.onUpdated should have a filter template like webRequest.onBeforeRequest

Categories

(WebExtensions :: General, defect, P3)

defect

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: 21Naown, Assigned: mixedpuppy)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [design-decision-approved][tabs][triaged])

Attachments

(1 file)

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.
Whiteboard: [design-decision-needed]
Whiteboard: [design-decision-needed] → [design-decision-needed][tabs]
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
Hello, I will not be able to join the meeting you mentioned. Thank you for proposing to me!
Flags: needinfo?(amckay)
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.
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Flags: needinfo?(amckay)
Resolution: --- → WONTFIX
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.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WONTFIX → ---
Priority: -- → P2
Whiteboard: [design-decision-needed][tabs] → [design-decision-approved][tabs]
Whiteboard: [design-decision-approved][tabs] → [design-decision-approved][tabs][triaged]
Component: WebExtensions: Untriaged → WebExtensions: General
Priority: P2 → P3
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.
Attachment #8955375 - Flags: feedback?(kmaglione+bmo)
Assignee: nobody → mixedpuppy
tabs.onUpdated has come up a few times recently, so I decided to give it a quick spin.
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.
Attachment #8955375 - Flags: feedback?(kmaglione+bmo) → feedback+
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"]
Attachment #8955375 - Flags: review?(kmaglione+bmo) → review+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2c03c9417c77
add filtering to tabs.onUpdated, r=kmag
https://hg.mozilla.org/mozilla-central/rev/2c03c9417c77
Status: REOPENED → RESOLVED
Closed: 3 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Thank you very much!
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.
Flags: needinfo?(mixedpuppy)
has tests.
Flags: needinfo?(mixedpuppy) → qe-verify-
Keywords: dev-doc-needed
Depends on: 1454406
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.
Flags: needinfo?(mixedpuppy)
See Also: → 1461693
Depends on: 1461695
(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
Flags: needinfo?(mixedpuppy)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.