[Session Restore] Remove calls to |_ensureInitialized()| and no longer throw a "Session Store is not initialized." error

RESOLVED FIXED in Firefox 34

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: smacleod, Assigned: tyler.colgan, Mentored)

Tracking

Trunk
Firefox 34
Points:
1
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Bug 918024 Removed the synchronous start up fallback for sessions store. We left the calls to |_ensureInitialized()| so we would throw an error if someone attempted to trigger the removed code path.

Since Bug 918024 landed, we haven't seen reports of these errors and we should eventually remove this code if everything stays quiet.
(Reporter)

Updated

4 years ago
Flags: firefox-backlog?
Whiteboard: [good first bug][mentor=smacleod][lang=js]
Flags: firefox-backlog? → firefox-backlog+
Mentor: smacleod
Whiteboard: [good first bug][mentor=smacleod][lang=js] → [good first bug][lang=js]

Updated

4 years ago
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js] p=1
(Assignee)

Comment 1

4 years ago
Created attachment 8465594 [details] [diff] [review]
Remove calls to _ensureInitialized() (version 1)

If I'm interpreting this correctly, it sounds like we want to remove _ensureInitialized() and any calls to it. I went ahead and did that in this patch; let me know if I'm on the right track.

Assuming that this is the right approach, it looks like doRestore() would no longer have a possibility of throwing an error, so I removed its |@throws| line as well.
Attachment #8465594 - Flags: feedback?(smacleod)
(Reporter)

Comment 2

4 years ago
Comment on attachment 8465594 [details] [diff] [review]
Remove calls to _ensureInitialized() (version 1)

Review of attachment 8465594 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Tyler, this looks great. Thanks for the patch! I've pushed it to Try for you[1].

You should consider applying for Level 1 Commit access[2] so that you can push
to Try as well. Tim or I would vouch for you.

Please let me know if you'd like any help finding more stuff to work on.

Thanks!

[1] https://tbpl.mozilla.org/?tree=Try&rev=5a00ca10c01c
[2] https://www.mozilla.org/hacking/commit-access-policy/
Attachment #8465594 - Flags: feedback?(smacleod) → review+
(Reporter)

Comment 3

4 years ago
If you'd like to apply for commit access you should also take a look at https://www.mozilla.org/hacking/committer/
(Reporter)

Comment 4

4 years ago
Try looks fine.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/b8672944145a
Assignee: nobody → tyler.colgan
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b8672944145a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34

Updated

4 years ago
Iteration: --- → 34.1
Points: --- → 1
QA Whiteboard: [qa?]
Whiteboard: [good first bug][lang=js] p=1 → [good first bug][lang=js]
(Reporter)

Updated

4 years ago
QA Whiteboard: [qa?] → [qa-]
You need to log in before you can comment on or make changes to this bug.