Implement sessions.onChanged WebExtensions API

RESOLVED FIXED in Firefox 53

Status

()

Toolkit
WebExtensions: Compatibility
P2
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

({dev-doc-complete})

unspecified
mozilla53
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [sessions]triaged)

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year ago
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 4

a year ago
mozreview-review
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 5

a year ago
mozreview-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.

Updated

a year ago
Flags: needinfo?(aswan)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

a year ago
Try looks good. Requesting check in.
Keywords: checkin-needed
(Assignee)

Comment 8

a year ago
(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

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3a44959def71
Implement sessions.onChanged WebExtensions API, r=aswan
Keywords: checkin-needed

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3a44959def71
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Updated

a year ago
Keywords: dev-doc-needed
-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sessions/onChanged
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.