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

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Rob Wu [:robwu]]

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.

Comment 1

[Rob Wu [:robwu]]

Attachment: Bug 1299053 - Remove unsupported "notification" type.

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

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1299053 - Refactor test/browser/browser_ext_getViews

I want to add more tests, based on tabId.
Adding more positional arguments makes the code less readable,
so I moved the check for windowId to a separate method.

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

Comment 3

[Rob Wu [:robwu]]

Attachment: Bug 1299053 - support tabId in browser.extension.getViews

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

Comment 4

[Rob Wu [:robwu]]

Attachment: Bug 1299053 - Ensure that background pages have windowId -1 in browser.extension.getViews

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

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8860727 [details]
Bug 1299053 - Remove unsupported "notification" type.

https://reviewboard.mozilla.org/r/132654/#review135890

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8860728 [details]
Bug 1299053 - Refactor test/browser/browser_ext_getViews

https://reviewboard.mozilla.org/r/132656/#review135896

Comment 7

[Andrew Swan [:aswan]]

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?

Comment 8

[Andrew Swan [:aswan]]

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 ..."

Comment 9

[Rob Wu [:robwu]]

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 10

[Rob Wu [:robwu]]

Comment on attachment 8860729 [details]
Bug 1299053 - support tabId in browser.extension.getViews

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

Comment 11

[Rob Wu [:robwu]]

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

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

Comment 12

[Rob Wu [:robwu]]

Comment on attachment 8860728 [details]
Bug 1299053 - Refactor test/browser/browser_ext_getViews

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

Comment 13

[Rob Wu [:robwu]]

Comment on attachment 8860729 [details]
Bug 1299053 - support tabId in browser.extension.getViews

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

Comment 14

[Rob Wu [:robwu]]

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

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

Comment 15

[Mozilla Autoland]

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 16

[Rob Wu [:robwu]]

Comment on attachment 8860727 [details]
Bug 1299053 - Remove unsupported "notification" type.

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

Comment 17

[Rob Wu [:robwu]]

Comment on attachment 8860728 [details]
Bug 1299053 - Refactor test/browser/browser_ext_getViews

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

Comment 18

[Rob Wu [:robwu]]

Comment on attachment 8860729 [details]
Bug 1299053 - support tabId in browser.extension.getViews

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

Comment 19

[Rob Wu [:robwu]]

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

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

[Ryan VanderMeulen [:RyanVM]]

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