1320306 - Implement sessions.onChanged WebExtensions API

Status: RESOLVED FIXED

(WebExtensions :: Compatibility, defect, P2)

Description

[Bob Silverberg [:bsilverberg]]

This was split off from Bug #1308061, so that that can land independently.

Implement browser.sessions.onChanged event, which is documented at https://developer.chrome.com/extensions/sessions#event-onChanged.

Comment 1

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1320306 - Implement sessions.onChanged WebExtensions API,

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

Comment 2

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8814395 [details]
Bug 1320306 - Implement sessions.onChanged WebExtensions API,

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

Comment 3

[Bob Silverberg [:bsilverberg]]

Dependency 1308061 landed on autoland and looks like it will be good to land on central as well. Andrew, could you please see if you can find time to review this? It would be great to land before Hawaii.

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 8814395 [details]
Bug 1320306 - Implement sessions.onChanged WebExtensions API,

https://reviewboard.mozilla.org/r/95672/#review96070

::: browser/components/extensions/ext-sessions.js:18
(Diff revision 2)
> +let observer = {
> +  observe: function() {
> +    this.emit("changed");
> +  },
> +};
> +EventEmitter.decorate(observer);

is this really buying us anything?  why not just attach an observer directly down in the event manager below?

::: browser/components/extensions/test/browser/browser_ext_sessions_restore.js:15
(Diff revision 2)
> +                                  "resource:///modules/sessionstore/TabStateFlusher.jsm");
>  
>  add_task(function* test_sessions_restore() {
>    function background() {
> +    let notificationCount = 0;
> +    browser.sessions.onChanged.addListener(function listener() {

why not use an arrow function?

::: browser/components/extensions/test/browser/browser_ext_sessions_restore.js:18
(Diff revision 2)
> +      if (notificationCount == 10) {
> +        browser.sessions.onChanged.removeListener(listener);
> +      }

this isn't actually necessary...

::: browser/components/extensions/test/browser/browser_ext_sessions_restore.js:54
(Diff revision 2)
>        permissions: ["sessions", "tabs"],
>      },
>      background,
>    });
>  
> +  function assertNotificationCount(expected, actual) {

roll the awaitMessage() call into this function?

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8814395 [details]
Bug 1320306 - Implement sessions.onChanged WebExtensions API,

https://reviewboard.mozilla.org/r/95672/#review96098

Eh, mozreview fail again, mean to say: just a few nits, your choice whether to change it or not.

Comment 6

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8814395 [details]
Bug 1320306 - Implement sessions.onChanged WebExtensions API,

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

Comment 7

[Bob Silverberg [:bsilverberg]]

Try looks good. Requesting check in.

Comment 8

[Bob Silverberg [:bsilverberg]]

(In reply to Andrew Swan [:aswan] from comment #5)
 
> Eh, mozreview fail again, mean to say: just a few nits, your choice whether
> to change it or not.

Thanks Andrew. Your suggestions were good. I addressed them and submitted to try and everything looks good. Feel free to take a final look if you like, but I pretty much just made all of the changes you suggested.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a44959def71
Implement sessions.onChanged WebExtensions API, r=aswan

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3a44959def71

Comment 11

[Will Bamberg [:wbamberg]]

-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sessions/onChanged