Last Comment Bug 742098 - refactor observe() into a set of distinct methods
: refactor observe() into a set of distinct methods
Status: RESOLVED FIXED
[good first bug][mentor=zpao,dietrich...
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 14
Assigned To: Max Li [:maxli]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-03 16:40 PDT by Dietrich Ayala (:dietrich)
Modified: 2012-04-12 10:26 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (25.13 KB, patch)
2012-04-08 17:37 PDT, Max Li [:maxli]
paul: feedback+
Details | Diff | Review
Patch (v2) (41.67 KB, patch)
2012-04-09 17:43 PDT, Max Li [:maxli]
paul: feedback+
Details | Diff | Review
Patch (v3) (41.68 KB, patch)
2012-04-10 15:57 PDT, Max Li [:maxli]
paul: review+
Details | Diff | Review

Description Dietrich Ayala (:dietrich) 2012-04-03 16:40:32 PDT
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 reading much bugmail, email directly) 2012-04-05 15:01:16 PDT
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] 2012-04-08 17:37:11 PDT
Created attachment 613210 [details] [diff] [review]
Patch

As per comment 1, I only refactored observe().
Comment 3 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-04-09 12:10:08 PDT
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] 2012-04-09 17:43:46 PDT
Created attachment 613453 [details] [diff] [review]
Patch (v2)

Fixed the switch styling
Comment 5 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-04-10 12:33:14 PDT
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] 2012-04-10 15:57:23 PDT
Created attachment 613807 [details] [diff] [review]
Patch (v3)

Fixed nit
Comment 7 Mozilla RelEng Bot 2012-04-10 19:45:57 PDT
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/poshannessy@mozilla.com-6daf0d9a4d49
Comment 8 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-04-11 12:47:35 PDT
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 reading much bugmail, email directly) 2012-04-11 13:12:46 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d83a00be8a0
Comment 10 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-12 10:26:21 PDT
https://hg.mozilla.org/mozilla-central/rev/6d83a00be8a0

Note You need to log in before you can comment on or make changes to this bug.