Closed Bug 808171 Opened 7 years ago Closed 7 years ago

setting enabled results in double notifications

Categories

(Firefox Graveyard :: SocialAPI, defect)

18 Branch
defect
Not set

Tracking

(firefox17 fixed, firefox18 fixed)

VERIFIED FIXED
Firefox 19
Tracking Status
firefox17 --- fixed
firefox18 --- fixed

People

(Reporter: mixedpuppy, Assigned: markh)

References

Details

(Whiteboard: [qa-])

Attachments

(3 files, 1 obsolete file)

In SocialService.enabled we end up calling _setEnabled twice, since we directly call it as well as calling it from a pref observer.  This results in a bunch of double notifications and various other problems which are more visible when disabling the feature.  We will just remove the direct call to _setEnabled and let the pref observer do that.  (per irc with gavin/jaws)
Attached patch setEnabled fixSplinter Review
Found this double-calling of setEnabled while testing bug 807217.  This issue caused the tests in the patch for bug 807217 to fail.
Assignee: nobody → mixedpuppy
Status: NEW → ASSIGNED
Attachment #677874 - Flags: review?(gavin.sharp)
Attachment #677874 - Flags: review?(gavin.sharp) → review+
Could also add a simple test that toggling notifications are only sent once etc.
Keywords: checkin-needed
That Try run is positively failtastic. Please don't request checkin until your run is finished and verified to be OK.
Keywords: checkin-needed
Comment on attachment 677874 [details] [diff] [review]
setEnabled fix

Doesn't this patch make us dependent on the order of observers?  eg, IIUC, isn't anyone who adds a pref observer before us going to see the wrong value for Social.enabled?
This is an alternative that doesn't have the drawback I mentioned in the previous comment - the pref observer instead just checks if the state needs to be changed.

Note I also swapped the _setEnabled/Services.prefs.setBoolPref calls - by doing the _setEnabled() first, we ensure that all other observers for this pref will see the correct Social.enabled.
Comment on attachment 678702 [details] [diff] [review]
Alternative that has the pref observer check if the state really needs to change.

People who want to observe the social service's state changes should probably do so by watching the social:pref-changed topic, but you're right that this is more robust - I like it.

Why not put the check in _setEnabled, though?
Attachment #678702 - Flags: feedback+
Ah, Shane points out that there is such a check, but not in safe mode - I never really understood that code. Seems like that kind of "override" logic should be in the front-end somewhere.
Comment on attachment 678702 [details] [diff] [review]
Alternative that has the pref observer check if the state really needs to change.

This is the patch I'm using underneath bug 807217
Attachment #678702 - Flags: review?(gavin.sharp)
Attachment #678702 - Flags: review?(gavin.sharp) → review+
Attached patch v3 (obsolete) — Splinter Review
Attachment #678954 - Flags: review?(mhammond)
Comment on attachment 678954 [details] [diff] [review]
v3

The _initialized change is too risky for beta - going with the original.
Attachment #678954 - Attachment is obsolete: true
Attachment #678954 - Attachment is patch: true
Attachment #678954 - Flags: review?(mhammond)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3d534f9a6a3
OS: Mac OS X → All
Hardware: x86 → All
Assignee: mixedpuppy → mhammond
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Social
User impact if declined: required for bug 807217
Testing completed (on m-c, etc.): landed on inbound
Risk to taking this patch (and alternatives if risky): small, avoids double notifications that would confuse tests and potentially real-world scenarios as well
String or UUID changes made by this patch: none

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

Patch for branches, includes the follow-up
Attachment #678986 - Flags: review+
Attachment #678986 - Flags: approval-mozilla-beta?
Attachment #678986 - Flags: approval-mozilla-aurora?
This was part of bug 807217 but was done in a separate bug. Lukas gave approval on #fx-team to land this for the go-to build of beta 5

https://hg.mozilla.org/releases/mozilla-beta/rev/e449c8cd821a
Attachment #678986 - Flags: approval-mozilla-beta?
Attachment #678986 - Flags: approval-mozilla-beta+
Attachment #678986 - Flags: approval-mozilla-aurora?
Attachment #678986 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/e3d534f9a6a3
https://hg.mozilla.org/mozilla-central/rev/e3bec8c880a7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Does this have or need tests?
Flags: in-testsuite?
No tests, but the problems it addresses aren't really user-facing.  It's been working fine so I'll verify.
Status: RESOLVED → VERIFIED
Flags: in-testsuite? → in-testsuite-
Whiteboard: [qa-]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.