Closed Bug 1320306 Opened 5 years ago Closed 5 years ago

Implement sessions.onChanged WebExtensions API

Categories

(WebExtensions :: Compatibility, defect, P2)

defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Iteration:
53.2 - Dec 12
Tracking Status
firefox53 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [sessions]triaged)

Attachments

(1 file)

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.
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.
Flags: needinfo?(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?
Attachment #8814395 - Flags: review?(aswan) → review+
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.
Flags: needinfo?(aswan)
Try looks good. Requesting check in.
Keywords: checkin-needed
(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.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3a44959def71
Implement sessions.onChanged WebExtensions API, r=aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3a44959def71
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Keywords: dev-doc-needed
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.