Closed Bug 1359806 Opened 8 years ago Closed 8 years ago

sessions.restore returns an array instead of an object

Categories

(WebExtensions :: General, defect, P1)

defect

Tracking

(firefox54 fixed, firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

(Keywords: addon-compat, Whiteboard: [sessions]triaged)

Attachments

(2 files)

browser.sessions.restore should return an object as per [1] and [2], but it currently returns an array containing a single object. This should be fixed. [1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sessions/restore [2] http://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/sessions.json#153
Blocks: 1322060
Comment on attachment 8861951 [details] Bug 1359806 - Fix sessions.restore to return an object instead of an array, https://reviewboard.mozilla.org/r/133958/#review136884
Attachment #8861951 - Flags: review?(mixedpuppy) → review+
That's a bummer. Do you think this is used enough that some announcement should be made out to the mailing lists? Or is it used so little someone just commented on the problem?
(In reply to Shane Caraveo (:mixedpuppy) from comment #3) > That's a bummer. Do you think this is used enough that some announcement > should be made out to the mailing lists? Or is it used so little someone > just commented on the problem? Yes, I am concerned about that too. Either nobody is using it, or people are ignoring the documentation and schema because it doesn't work as advertised. I discussed it with Andy and he mentioned that we can search DXR to see if any extensions are currently using it. I guess we can either reach out to individual authors, or make a general announcement, based on those findings. He also suggested that Jorge might be the person to make that determination, so I am needinfoing him here as well. Oh, and thanks for the review. :) Note that this API has been available since 52.
Flags: needinfo?(jorge)
We'll need to look into DXR (when it's working) to assess the impact and decide what to do about it. What about Chrome compatibility? Is this bug going to make the API compatible with Chrome?
Flags: needinfo?(jorge)
Keywords: addon-compat
(In reply to Jorge Villalobos [:jorgev] from comment #5) > We'll need to look into DXR (when it's working) to assess the impact and > decide what to do about it. > > What about Chrome compatibility? Is this bug going to make the API > compatible with Chrome? Yes
Shane, is this ready for checkin?
Flags: needinfo?(mixedpuppy)
(In reply to Kevin Jones from comment #7) > Shane, is this ready for checkin? I've already landed it, Kevin. It's on autoland now.
Flags: needinfo?(mixedpuppy)
Pushed by bsilverberg@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ff2157da9673 Fix sessions.restore to return an object instead of an array, r=mixedpuppy
(In reply to Bob Silverberg [:bsilverberg] from comment #8) > (In reply to Kevin Jones from comment #7) > > Shane, is this ready for checkin? > > I've already landed it, Kevin. It's on autoland now. Thanks Bob. Sorry I was addressing the wrong person :-/
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Attached patch Uplift to beta (deleted) — Splinter Review
Approval Request Comment [Feature/Bug causing the regression]: 1308060 [User impact if declined]: Users of the sessions.restore API will receive data in an unexpected format which is incorrect. [Is this code covered by automated tests?]: Yes, a test exists at browser/components/extensions/test/browser/browser_ext_sessions_restore.js and was updated to cover this change. A try run has been started at https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed0d0f219d94b2e07dec9fda4f2542acce1a0a62 [Has the fix been verified in Nightly?]: No [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: This is a two-line change to a single API method and it is covered by tests. [String changes made/needed]: None
Attachment #8862624 - Flags: approval-mozilla-beta?
Comment on attachment 8862624 [details] [diff] [review] Uplift to beta Fix a session.restore issue and include test. Beta54+. Should be in 54 beta 4.
Attachment #8862624 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Jorge, this will land in 54. Can I assume that your adding the addon-compat keyword is enough to keep this on your radar for communication and possible outreach?
Flags: needinfo?(jorge)
(In reply to Bob Silverberg [:bsilverberg] from comment #13) > [Is this code covered by automated tests?]: Yes, a test exists at > browser/components/extensions/test/browser/browser_ext_sessions_restore.js > and was updated to cover this change. A try run has been started at > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=ed0d0f219d94b2e07dec9fda4f2542acce1a0a62 > [Has the fix been verified in Nightly?]: No > [Needs manual test from QE? If yes, steps to reproduce]: No Setting qe-verify- based on Bob's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
The compatibility blog post for 54 was published a while ago, as the WebExtensions post for 54. Maybe this warrants an independent post. Anyway, it's on my radar.
Flags: needinfo?(jorge)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: