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

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 1

[Chris]

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?

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 3

[Chris]

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?

Comment 4

[Chris]

Attachment: bug-944375-fix.patch

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 6

[Chris]

I just ran them and all tests in SessionStore passed.  I'll upload a new patch containing the revisions later today.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Great, thank you!

Comment 8

[Chris]

Attachment: bug-944375-fix-rev1.patch

Changed the name of the const to BROWSER_EVENTS.

Comment 9

[Chris]

Attachment: bug-944375-fix-rev1.patch

Forgot to alter patch notes to reflect revision.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 11

[Chris]

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?

Comment 12

[Chris]

Attachment: bug-944375-fix-rev2.patch

Fixed the comment for BROWSER_EVENTS.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 14

[Chris]

Attachment: bug-944375-checkin.patch

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/28b5d7bdb2bb

Should we have tests for this or are the existing sessionstore tests enough?

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

(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!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/28b5d7bdb2bb