742098 - refactor observe() into a set of distinct methods

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Dietrich Ayala (:dietrich)]

they're currently both handling way too many different events & notifications, resulting in code that makes it difficult to see what's going on.

instead, each event & notification should be implemented as a single method following the on* pattern.

handleEvent() and observe() should call those methods for the given event.

this also makes it easier to call each one of those event/notification handlers asynchronously if we want.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

If somebody comes along and wants to take this, observe is implemented here: https://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#499 and handleEvent is a bit lower in the file.

I think handleEvent is actually ok as is. We already call out to specific functions for everything except TabPinned/TabUnpinned, which is only 1 line and calls something else anyway.

Comment 2

[Max Li [:maxli]]

Attachment: Patch

As per comment 1, I only refactored observe().

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 613210 [details] [diff] [review]
Patch

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

Hey Max, Thanks for taking this on!

It looks good already, but I figured while we're here, let's scope creep and fix some style since you started down that path :) feedback+ for now and review should only be a quick second look.

I think we should fix the styling of the switch blocks. You already did it in onPrivateBrowsing, but let's do it in onPrefChanged and observe (and if there are other switches, then those too). Like so:
> switch (foo) {
>   case "bar":
>     code code code;

Comment 4

[Max Li [:maxli]]

Attachment: Patch (v2)

Fixed the switch styling

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 613453 [details] [diff] [review]
Patch (v2)

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

I declare this patch a good example of diffs gone wild. Diffing the changed file & the original with Filemerge makes this much easier to grok :)

This looks great. Just one nit below which I should have caught last time (sorry!) and then I'll r+. I'll push this to try server just to double check nothing got lost in translation.

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +510,5 @@
> +      case "quit-application-granted":
> +        this.onQuitApplicationGranted();
> +        break;
> +      case "browser-lastwindow-close-granted":
> +        this.onBrowserCloseGranted();

Nit: Let's call this onLastWindowCloseGranted. At a glance BrowserCloseGranted looks to be related to <browser> elements but it's not & the important take away is that it's related to the last window.

Comment 6

[Max Li [:maxli]]

Attachment: Patch (v3)

Fixed nit

Comment 7

[Mozilla RelEng Bot]

Try run for 6daf0d9a4d49 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6daf0d9a4d49
Results (out of 23 total builds):
    success: 19
    warnings: 1
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6daf0d9a4d49

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 613807 [details] [diff] [review]
Patch (v3)

Thanks Max! I'll get this checked in today.

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d83a00be8a0

Comment 10

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/6d83a00be8a0