Implement sessions.onChanged WebExtensions API

RESOLVED FIXED in Firefox 53

Status

P2
normal
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

({dev-doc-complete})

unspecified
mozilla53
dev-doc-complete
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [sessions]triaged)

Attachments

(1 attachment)

(Assignee)

Description

2 years 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

2 years 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

2 years 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

2 years 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.
Flags: needinfo?(aswan)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

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

Comment 8

2 years 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

2 years 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

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

Updated

2 years ago
Keywords: dev-doc-needed

Updated

9 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.