Closed Bug 944375 Opened 9 years ago Closed 9 years ago

Register per-browser event handlers from a const array of event names

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: ttaubert, Assigned: chrishood)

Details

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

Attachments

(1 file, 4 obsolete files)

In SessionStore.jsm, we currently register all per-browser event handlers (load, SwapDocShells, UserTypedValueChanged) with a line for each handler. The same thing happens for removing those event listeners.

Just like for notifications (see OBSERVING) and messages (see MESSAGES) we should have a constant that defines an array of per-browser event names that we listen for. Whenever a tab is added/removed we iterate the list of even names and add/remove event listeners.
Hi I'd be interested in taking this on as my first bug. 

It looks like you want to start by putting NOTIFY_WINDOWS_RESTORED, NOTIFY_BROWSER_STATE_RESTORED and NOTIFY_LAST_SESSION_CLEARED into a constant array as apposed to having them be their own constants.  Where should I look in terms of adding and removing event listeners?
Flags: needinfo?(ttaubert)
Hey Chris, great to hear you want to pick this up. I was not talking about the constants we define for notification topics though. ss.onTabAdd() and ss.onTabRemove() have a bunch of addEventListener() and removeEventListener() calls. These should be implemented like the |OBSERVING| and |MESSAGES| arrays.

I assigned this bug to you, please ask if you have any questions. Here or on IRC.
Assignee: nobody → chrishood
Status: NEW → ASSIGNED
Flags: needinfo?(ttaubert)
What should I call the new const array?  Right now I'm going with TAB_OBSERVING, is this fine or do we want to call it something else?
Flags: needinfo?(ttaubert)
Attached patch bug-944375-fix.patch (obsolete) — Splinter Review
Attachment #8342548 - Flags: review?(ttaubert)
Comment on attachment 8342548 [details] [diff] [review]
bug-944375-fix.patch

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

Looks great, thanks! We should be good to go after adjusting the const name. Did you run sessionstore tests yet to check if they still pass?

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +78,5 @@
>    "TabUnpinned"
>  ];
>  
> +// Tab notifications observed.
> +const TAB_OBSERVING = [

Similar to TAB_EVENTS above maybe BROWSER_EVENTS?
Attachment #8342548 - Flags: review?(ttaubert) → feedback+
Flags: needinfo?(ttaubert)
I just ran them and all tests in SessionStore passed.  I'll upload a new patch containing the revisions later today.
Great, thank you!
Attached patch bug-944375-fix-rev1.patch (obsolete) — Splinter Review
Changed the name of the const to BROWSER_EVENTS.
Attachment #8342548 - Attachment is obsolete: true
Attachment #8344758 - Flags: review?(ttaubert)
Attached patch bug-944375-fix-rev1.patch (obsolete) — Splinter Review
Forgot to alter patch notes to reflect revision.
Attachment #8344758 - Attachment is obsolete: true
Attachment #8344758 - Flags: review?(ttaubert)
Attachment #8344763 - Flags: review?(ttaubert)
Comment on attachment 8344763 [details] [diff] [review]
bug-944375-fix-rev1.patch

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

Great, thank you! This is ready for checkin-needed with the small comment fix.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +77,5 @@
>    "TabOpen", "TabClose", "TabSelect", "TabShow", "TabHide", "TabPinned",
>    "TabUnpinned"
>  ];
>  
> +// Tab notifications observed.

Nit: Browser events observed.
Attachment #8344763 - Flags: review?(ttaubert) → review+
Ah, forgot to change that to reflect the new const name.
Thanks for the review, is there anything else I need to do to get it commited?
Attached patch bug-944375-fix-rev2.patch (obsolete) — Splinter Review
Fixed the comment for BROWSER_EVENTS.
Attachment #8344763 - Attachment is obsolete: true
Attachment #8344835 - Flags: checkin?(ttaubert)
Comment on attachment 8344835 [details] [diff] [review]
bug-944375-fix-rev2.patch

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

Thanks for fixing the comment! To get this landed you need to upload a new patch (that is commonly named "patch for checkin" or the like) that has "r=ttaubert" at the end of the patch description. Once you did that you can add the "checkin-needed" keyword to the keywords input field at the top of the bug. We have sheriffs that will then pick up the patch and land it for you.
Attachment #8344835 - Flags: checkin?(ttaubert)
Keywords: checkin-needed
Attachment #8344835 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/28b5d7bdb2bb

Should we have tests for this or are the existing sessionstore tests enough?
Flags: in-testsuite?
Keywords: checkin-needed
Whiteboard: [good first bug][mentor=ttaubert][lang=js] → [good first bug][mentor=ttaubert][lang=js][fixed-in-fx-team]
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #15)
> Should we have tests for this or are the existing sessionstore tests enough?

The existing ones are enough. This functionality is so basic that 95+ percent (wild guess) of our tests would fail. Thanks for checking though!
https://hg.mozilla.org/mozilla-central/rev/28b5d7bdb2bb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=ttaubert][lang=js][fixed-in-fx-team] → [good first bug][mentor=ttaubert][lang=js]
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.