*** Original post on bio 677 at 2011-02-03 21:15:00 UTC *** From bug 954035 (bio 598): > - Close and reopen the streaming connection when the "track" preference > changes. Do we currently have a way to be notified of account specific > preference changes? Currently I don't think accounts are told about preference changes, but this is probably important for other protocols as well.

*** Original post on bio 677 as attmnt 2260 at 2013-03-09 15:41:00 UTC *** The only thing I was unsure about was whether we need to remove the observer or not? We don't seem to in the buddy list, but I'm always confused about when we leak in these situations.

Comment on attachment 8354025 [details] [diff] [review] Patch *** Original change on bio 677 attmnt 2260 at 2013-03-09 15:50:46 UTC *** > unInit: function() { throw Cr.NS_ERROR_NOT_IMPLEMENTED; }, I would remove the observer here. >+ Components.utils.recomputeWrappers(aTopic + " " + aMsg); This seems unnecessary ;) Thanks for fixing this longstanding bug!

*** Original post on bio 677 at 2013-03-09 15:56:29 UTC *** (In reply to comment #2) > > unInit: function() { throw Cr.NS_ERROR_NOT_IMPLEMENTED; }, > I would remove the observer here. As clokep pointed out, all implementations would then have to call it, so I would suggest adding the observer to the twitter init/uninit instead for now. (I'm assuming there is indeed a leak issue here - so bouncing this to flo).

Comment on attachment 8354025 [details] [diff] [review] Patch *** Original change on bio 677 attmnt 2260 at 2013-03-09 15:56:47 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 677 as attmnt 2261 at 2013-03-09 16:26:00 UTC *** I decided to not prematurely abstract this code to jsProtoHelper, if we have a use case for the other protocols we can move it.

Comment on attachment 8354025 [details] [diff] [review] Patch *** Original change on bio 677 attmnt 2260 at 2013-03-09 16:26:55 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354026 [details] [diff] [review] Patch to just Twitter *** Original change on bio 677 attmnt 2261 at 2013-03-09 16:30:09 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 677 at 2013-03-11 12:41:08 UTC *** Wouldn't this.prefs.addObserver("track", this, false); do what you want, and save you a test? Shouldn't this observer be added when creating a request to the streaming API, and removed when the request is closed? (is that always when the account is disconnected?)

*** Original post on bio 677 at 2013-03-13 01:53:53 UTC *** (In reply to comment #5) > Wouldn't this.prefs.addObserver("track", this, false); do what you want, and > save you a test? I thought so, but I wasn't sure if it did or not. (I'm unsure what you mean by "save you a test" though.) > Shouldn't this observer be added when creating a request to the streaming API, > and removed when the request is closed? (is that always when the account is > disconnected?) We could do that, do you think that's simpler than the current way I have it? (I don't think I'd like this as much as we'd have to remove the observer in multiple places: in onStreamError and cleanUp, which is also where we delete this._streamingRequest.)

*** Original post on bio 677 at 2013-03-13 08:39:04 UTC *** (In reply to comment #6) > (In reply to comment #5) > > Wouldn't this.prefs.addObserver("track", this, false); do what you want, and > > save you a test? > I thought so, but I wasn't sure if it did or not. (I'm unsure what you mean by > "save you a test" though.) I mean you wouldn't need |aMsg != "track"| in the if that causes the early return in the observe method. > > Shouldn't this observer be added when creating a request to the streaming API, > > and removed when the request is closed? (is that always when the account is > > disconnected?) > We could do that, do you think that's simpler than the current way I have it? > (I don't think I'd like this as much as we'd have to remove the observer in > multiple places: in onStreamError and cleanUp, which is also where we delete > this._streamingRequest.) onStreamError calls gotDisconnected that calls cleanUp.

*** Original post on bio 677 as attmnt 2272 at 2013-03-13 10:35:00 UTC *** Taking into account flo's review comments.

Comment on attachment 8354026 [details] [diff] [review] Patch to just Twitter *** Original change on bio 677 attmnt 2261 at 2013-03-13 10:35:38 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354037 [details] [diff] [review] Patch v2 *** Original change on bio 677 attmnt 2272 at 2013-03-13 10:36:01 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 677 as attmnt 2273 at 2013-03-13 10:51:00 UTC *** Per Florian's comment on IRC.

Comment on attachment 8354037 [details] [diff] [review] Patch v2 *** Original change on bio 677 attmnt 2272 at 2013-03-13 10:51:38 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354038 [details] [diff] [review] Patch v3 *** Original change on bio 677 attmnt 2273 at 2013-03-13 10:52:42 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354038 [details] [diff] [review] Patch v3 *** Original change on bio 677 attmnt 2273 at 2013-03-13 10:54:47 UTC *** Looks good by code inspection. Bouncing the review back to aleth to double check. And I think it would be nice to test that the observer still works, and is correctly removed after all these changes :-). Here is what I just said on IRC: 11:51:11 - flo-retina: would be nice to test this (check that the observer is correctly removed when the account is disconnected cleanly, when it's disconnected because of an error on the stream (not sure how to reproduce that :-S), and when the account is deleted) 11:51:46 - flo-retina: possibly also at shutdown, but I would hope the shutdown code path goes the same way as either a normal disconnect or an account deletion

Comment on attachment 8354038 [details] [diff] [review] Patch v3 *** Original change on bio 677 attmnt 2273 at 2013-03-14 12:05:44 UTC *** > if (this._streamTimeout) { > clearTimeout(this._streamTimeout); > delete this._streamTimeout; >+ this.prefs.removeObserver("track", this); > } > if (this._streamingRequest) { > this._streamingRequest.abort(); > delete this._streamingRequest; > } I think there should be a comment explaining why the pref is removed in that particular if clause (since naively it has nothing to do with the timeouts), if the removeObserver call can't be moved outside it (is there actually a problem with removing a pref observer that doe not exist? This would be fine for events.).

*** Original post on bio 677 as attmnt 2276 at 2013-03-14 21:58:00 UTC *** I added a comment, unfortunately aleth isn't on IRC so I can't ask him if the comment is good...if you dislike it, please suggest another one!

Comment on attachment 8354038 [details] [diff] [review] Patch v3 *** Original change on bio 677 attmnt 2273 at 2013-03-14 21:58:23 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354041 [details] [diff] [review] Patch v3 with comment *** Original change on bio 677 attmnt 2276 at 2013-03-14 21:59:21 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354041 [details] [diff] [review] Patch v3 with comment *** Original change on bio 677 attmnt 2276 at 2013-03-15 09:06:03 UTC *** Thanks!

*** Original post on bio 677 at 2013-03-16 12:47:15 UTC *** Checked in as http://hg.instantbird.org/instantbird/rev/0c7e096c7cc7

Comment on attachment 8354041 [details] [diff] [review] Patch v3 with comment *** Original change on bio 677 attmnt 2276 at 2013-03-19 22:57:11 UTC was without comment, so any subsequent comment numbers will be shifted ***