Closed Bug 849764 Opened 7 years ago Closed 7 years ago

Fix removeObserver() calls with three params

Categories

(Core :: XPCOM, defect, minor)

Other
All
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: kshriram18, Assigned: magicxinzhang)

References

()

Details

(Whiteboard: [good-first-bug][mentor=mak][lang=js])

Attachments

(8 files, 11 obsolete files)

880 bytes, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
1.05 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
6.41 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
2.16 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
1.09 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
5.69 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
3.05 KB, patch
mossop
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
39.12 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
As mentioned by philor on #developers,
removeObserver calls take two parameters only and as seen at 
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIObserverService#removeObserver()
Whiteboard: [good-first-bug][mentor=mak][lang=js]
Hi, I'd like to work on this.  I did quick search and saw some removeObserver() calls also take one parameter.  I guess I will just leave those.
Attachment #725047 - Flags: review?(mak77)
(In reply to Xin from comment #1)
> Hi, I'd like to work on this.  I did quick search and saw some
> removeObserver() calls also take one parameter.  I guess I will just leave
> those.

Some of those may not be implementation of nsIObserver, other interfaces have similarly named methods.
Comment on attachment 725047 [details] [diff] [review]
Replace removeObserver() calls with three params with two

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

To simplify both the review and the landing process, could you please split the patch by srcdir folders?
Something like browser, toolkit, xpcom... and attach one patch for each folder (you can use Mercurial queues to simplify the process)

These are the removeObserver methods in interfaces:
http://mxr.mozilla.org/mozilla-central/search?string=removeObserver%28&find=\.idl&findi=\.idl%24&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
None of them is taking a bool as third param, so I think that fixing all of the 106 matches reported by the mxr query in url of this bug should be safe.
Attachment #725047 - Flags: review?(mak77)
Attached patch addon-sdk/ (obsolete) — Splinter Review
Attachment #725047 - Attachment is obsolete: true
Attachment #725177 - Flags: review?(mak77)
Attached patch b2g/ (obsolete) — Splinter Review
Attachment #725178 - Flags: review?(mak77)
Attached patch browser/ (obsolete) — Splinter Review
Attachment #725179 - Flags: review?(mak77)
Attached patch image/ (obsolete) — Splinter Review
Attachment #725180 - Flags: review?(mak77)
Attached patch mobile/ (obsolete) — Splinter Review
Attachment #725181 - Flags: review?(mak77)
Attached patch storage/ (obsolete) — Splinter Review
Attachment #725182 - Flags: review?(mak77)
Attached patch testing/ (obsolete) — Splinter Review
Attachment #725183 - Flags: review?(mak77)
Attached patch toolkit/ (obsolete) — Splinter Review
Attachment #725184 - Flags: review?(mak77)
Attachment #725184 - Flags: review?(mak77) → review+
Summary: Remove removeObserver() calls with three params → Fix removeObserver() calls with three params
Attachment #725183 - Flags: review?(mak77) → review+
Comment on attachment 725182 [details] [diff] [review]
storage/

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

::: storage/test/unit/test_vacuum.js
@@ -108,5 @@
>    print("\n*** Test that a VACUUM correctly happens and all notifications are fired.");
>    // Wait for VACUUM begin.
>    let beginVacuumReceived = false;
>    Services.obs.addObserver(function (aSubject, aTopic, aData) {
> -    Services.obs.removeObserver(arguments.callee, aTopic, false);

arguments.callee is deprecated, while here could you please name the function (something like onVacuum)and use the function name instead of arguments callee?

@@ -117,5 @@
>    let heavyIOTaskBeginReceived = false;
>    let heavyIOTaskEndReceived = false;
>    Services.obs.addObserver(function (aSubject, aTopic, aData) {
>      if (heavyIOTaskBeginReceived && heavyIOTaskEndReceived) {
> -      Services.obs.removeObserver(arguments.callee, aTopic, false);

ditto

@@ -130,5 @@
>    }, "heavy-io-task", false);
>  
>    // Wait for VACUUM end.
>    Services.obs.addObserver(function (aSubject, aTopic, aData) {
> -    Services.obs.removeObserver(arguments.callee, aTopic, false);

ditto
Attachment #725182 - Flags: review?(mak77) → feedback+
Attachment #725181 - Flags: review?(mak77) → review+
Attachment #725180 - Flags: review?(mak77) → review+
Attachment #725178 - Flags: review?(mak77) → review+
Comment on attachment 725177 [details] [diff] [review]
addon-sdk/

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

::: addon-sdk/source/lib/sdk/preferences/event-target.js
@@ +55,5 @@
>    off(this);
>  
>    // stop listening to preference changes
>    let branch = prefTargetNS(this).branch;
> +  branch.removeObserver('', prefTargetNS(this).observer);

this bug is only about nsIObserver, this is a pref observer, if you want to fix these please file a bug apart.
Attachment #725177 - Flags: review?(mak77) → feedback+
Comment on attachment 725179 [details] [diff] [review]
browser/

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

::: browser/components/preferences/aboutPermissions.js
@@ +428,5 @@
>     * Called on page unload.
>     */
>    cleanUp: function() {
>      if (this._observersInitialized) {
> +      Services.prefs.removeObserver("signon.rememberSignons", this);

please file a bug apart for prefservice observers

::: browser/components/privatebrowsing/test/browser/perwindow/browser_privatebrowsing_certexceptionsui.js
@@ +28,5 @@
>      };
>      function testCheckbox() {
>        win.removeEventListener("load", testCheckbox, false);
>        Services.obs.addObserver(function (aSubject, aTopic, aData) {
> +        Services.obs.removeObserver(arguments.callee, "cert-exception-ui-ready");

arguments callee is deprecated, while here please name the function (something like onCertUI) and replace arguments.callee with the function name
Attachment #725179 - Flags: review?(mak77) → feedback+
Attached patch storage/ (obsolete) — Splinter Review
Attachment #725182 - Attachment is obsolete: true
Attachment #728825 - Flags: review?(mak77)
Attachment #728825 - Flags: review?(mak77) → review+
Assignee: nobody → magicxinzhang
Status: NEW → ASSIGNED
Keywords: checkin-needed
Whiteboard: [good-first-bug][mentor=mak][lang=js] → [good-first-bug][mentor=mak][lang=js][leave open]
Attached patch b2g/Splinter Review
Attachment #725178 - Attachment is obsolete: true
Attachment #733689 - Flags: checkin?
Attached patch image/Splinter Review
Attachment #725180 - Attachment is obsolete: true
Attachment #733690 - Flags: checkin?
Attached patch mobile/Splinter Review
Attachment #725181 - Attachment is obsolete: true
Attachment #733691 - Flags: checkin?
Attached patch storage/Splinter Review
Attachment #728825 - Attachment is obsolete: true
Attachment #733694 - Flags: checkin?
Attached patch testing/Splinter Review
Attachment #725183 - Attachment is obsolete: true
Attachment #733695 - Flags: checkin?
Attached patch toolkit/Splinter Review
Attachment #725184 - Attachment is obsolete: true
Attachment #733697 - Flags: checkin?
Attached patch addon-sdk/Splinter Review
Leave pref observer as is.
Attachment #725177 - Attachment is obsolete: true
Attachment #733709 - Flags: review?(mak77)
Attached patch browser/ (obsolete) — Splinter Review
Leave pref observer as is.
Attachment #725179 - Attachment is obsolete: true
Attachment #733722 - Flags: review?(mak77)
Comment on attachment 733709 [details] [diff] [review]
addon-sdk/

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

::: addon-sdk/source/doc/module-source/sdk/platform/xpcom.md
@@ +47,5 @@
>        register: function register() {
>          observerService.addObserver(this, this.topic, false);
>        },
>        unregister: function() {
> +        addObserver.removeObserver(this, this.topic);

this addObserver.removeObserver to me looks a typo... I guess I will forward the review to Dave who knows this part better.
Attachment #733709 - Flags: review?(mak77) → review?(dtownsend+bugmail)
Comment on attachment 733722 [details] [diff] [review]
browser/

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

Thanks!
Attachment #733722 - Flags: review?(mak77) → review+
Attached patch browser/Splinter Review
Attachment #733722 - Attachment is obsolete: true
Attachment #733966 - Flags: checkin?
Attachment #733709 - Flags: review?(dtownsend+bugmail) → review+
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/45a1b15895acfe2bed4859edca152d854f7158e2
Bug 849764: Replace removeObserver() calls with three params with two in addon-sdk dir. r=Mossop
Comment on attachment 733709 [details] [diff] [review]
addon-sdk/

Thanks for the patch, I've landed these changes in the add-on sdk repo and they'll make their way to m-c with our next uplift next week.
Attachment #733709 - Flags: checkin+
Attachment #733689 - Flags: checkin? → checkin+
Attachment #733690 - Flags: checkin? → checkin+
Attachment #733691 - Flags: checkin? → checkin+
Attachment #733694 - Flags: checkin? → checkin+
Attachment #733695 - Flags: checkin? → checkin+
Attachment #733697 - Flags: checkin? → checkin+
Attachment #733966 - Flags: checkin? → checkin+
Whiteboard: [good-first-bug][mentor=mak][lang=js][leave open] → [good-first-bug][mentor=mak][lang=js]
Dave, look like you may not have noticed my note about the possible typo in the add-on sdk patch in comment 25
Flags: needinfo?(dtownsend+bugmail)
(In reply to Marco Bonardo [:mak] from comment #33)
> Dave, look like you may not have noticed my note about the possible typo in
> the add-on sdk patch in comment 25

Forgot about this sorry, filed bug 862085 with a fix.
Flags: needinfo?(dtownsend+bugmail)
You need to log in before you can comment on or make changes to this bug.