Implement browser.extension.getViews (+tabId option, -viewtype notifications)

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: Untriaged
P3
normal
RESOLVED FIXED
a year ago
3 months ago

People

(Reporter: robwu, Assigned: robwu)

Tracking

51 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox51 affected, firefox55 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

a year ago
extension.json defines view type "notifications". This is not supported by Firefox nor Chrome, and Chrome has also removed it: https://chromium.googlesource.com/chromium/src/+/6aa796d3ff416b6f41dd41a62ab57bab10f51dc6%5E%21/

Also, the extension.getViews API now includes the tabId key to filter out views:
https://chromium.googlesource.com/chromium/src/+/15fd52bd9893d7b72fda22e24d74c145b29221d1%5E%21/

This is very easy to implement, but let's not immediately jump to a patch because I'm in the process of restructuring the extension context and tabs/extensions implementation.

Updated

11 months ago
Priority: -- → P3
Whiteboard: triaged
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

4 months ago
mozreview-review
Comment on attachment 8860727 [details]
Bug 1299053 - Remove unsupported "notification" type.

https://reviewboard.mozilla.org/r/132654/#review135890
Attachment #8860727 - Flags: review?(aswan) → review+

Comment 6

4 months ago
mozreview-review
Comment on attachment 8860728 [details]
Bug 1299053 - Refactor test/browser/browser_ext_getViews

https://reviewboard.mozilla.org/r/132656/#review135896
Attachment #8860728 - Flags: review?(aswan) → review+

Comment 7

4 months ago
mozreview-review
Comment on attachment 8860729 [details]
Bug 1299053 - support tabId in browser.extension.getViews

https://reviewboard.mozilla.org/r/132658/#review135898

::: browser/components/extensions/test/browser/browser_ext_getViews.js:115
(Diff revision 1)
>    let winId2 = windowTracker.getId(win2);
>  
>    function* openTab(winId) {
>      extension.sendMessage("background-open-tab", winId);
>      yield extension.awaitMessage("tab-ready");
> +    return yield extension.awaitMessage("opened-tab");

no need for the yield (i thought eslint was checking for this...)

::: browser/components/extensions/test/browser/browser_ext_getViews.js:134
(Diff revision 1)
>      let count = yield extension.awaitMessage("getViews-count");
>      is(count, expectedCount, `count for ${JSON.stringify(filter)} correct`);
>    }
>  
>    yield checkViews("background", 0, 0, 0);
> +  yield checkViewsWithFilter({tabId: -1}, 1);

Is this really intentional and something we want to test?
Attachment #8860729 - Flags: review?(aswan) → review+

Comment 8

4 months ago
mozreview-review
Comment on attachment 8860730 [details]
Bug 1299053 - Ensure that background pages have windowId -1 in browser.extension.getViews

https://reviewboard.mozilla.org/r/132660/#review135900

::: commit-message-c5f60:1
(Diff revision 1)
> +Bug 1299053 - Background pages must have windowId -1 in browser.extension.getViews

Can you rephrase this to something like "ensure that background pages have ..."
Attachment #8860730 - Flags: review?(aswan) → review+
(Assignee)

Comment 9

4 months ago
mozreview-review-reply
Comment on attachment 8860729 [details]
Bug 1299053 - support tabId in browser.extension.getViews

https://reviewboard.mozilla.org/r/132658/#review135898

> no need for the yield (i thought eslint was checking for this...)

The linter on the trybot does warn about this. Perhaps I should update my local eslint package.

> Is this really intentional and something we want to test?

Yes this is intentional. -1 indicates no tab (and is also the value of browser.tabs.TAB_ID_NONE). The ability to query all non-tabs seems useful to me (and it is also supported by Chrome).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

3 months ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 018b28f96ec6 -d 25ddfb854eec: rebasing 395759:018b28f96ec6 "Bug 1299053 - Remove unsupported "notification" type. r=aswan"
rebasing 395760:0764c1352987 "Bug 1299053 - Refactor test/browser/browser_ext_getViews r=aswan"
merging browser/components/extensions/test/browser/browser_ext_getViews.js
warning: conflicts while merging browser/components/extensions/test/browser/browser_ext_getViews.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

3 months ago
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/a4b4d19f8c6c
Remove unsupported "notification" type. r=aswan
https://hg.mozilla.org/integration/autoland/rev/7452f4215289
Refactor test/browser/browser_ext_getViews r=aswan
https://hg.mozilla.org/integration/autoland/rev/31a271c3020c
support tabId in browser.extension.getViews r=aswan
https://hg.mozilla.org/integration/autoland/rev/fcf6042ae2ed
Ensure that background pages have windowId -1 in browser.extension.getViews r=aswan

Comment 21

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a4b4d19f8c6c
https://hg.mozilla.org/mozilla-central/rev/7452f4215289
https://hg.mozilla.org/mozilla-central/rev/31a271c3020c
https://hg.mozilla.org/mozilla-central/rev/fcf6042ae2ed
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.