Closed Bug 1299053 Opened 3 years ago Closed 3 years ago

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

Categories

(WebExtensions :: Untriaged, defect, P3)

51 Branch
defect

Tracking

(firefox51 affected, firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox51 --- affected
firefox55 --- fixed

People

(Reporter: robwu, Assigned: robwu)

Details

(Whiteboard: triaged)

Attachments

(4 files)

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.
Priority: -- → P3
Whiteboard: triaged
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 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 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 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+
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).
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)
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
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.