849764 - Fix removeObserver() calls with three params

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Shriram (irc: Mavericks)]

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()

Comment 1

[Xin]

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.

Comment 2

[Xin]

Attachment: Replace removeObserver() calls with three params with two

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(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 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 5

[Xin]

Attachment: addon-sdk/

Comment 6

[Xin]

Attachment: b2g/

Comment 7

[Xin]

Attachment: browser/

Comment 8

[Xin]

Attachment: image/

Comment 9

[Xin]

Attachment: mobile/

Comment 10

[Xin]

Attachment: storage/

Comment 11

[Xin]

Attachment: testing/

Comment 12

[Xin]

Attachment: toolkit/

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 16

[Xin]

Attachment: storage/

Comment 17

[Xin]

Attachment: b2g/

Comment 18

[Xin]

Attachment: image/

Comment 19

[Xin]

Attachment: mobile/

Comment 20

[Xin]

Attachment: storage/

Comment 21

[Xin]

Attachment: testing/

Comment 22

[Xin]

Attachment: toolkit/

Comment 23

[Xin]

Attachment: addon-sdk/

Leave pref observer as is.

Comment 24

[Xin]

Attachment: browser/

Leave pref observer as is.

Comment 25

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 26

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 733722 [details] [diff] [review]
browser/

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

Thanks!

Comment 27

[Xin]

Attachment: browser/

Comment 28

[[github robot]]

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 29

[Dave Townsend [: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.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/79131be6fd64
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc8070070b06
https://hg.mozilla.org/integration/mozilla-inbound/rev/af01ad72d579
https://hg.mozilla.org/integration/mozilla-inbound/rev/125d104b58be
https://hg.mozilla.org/integration/mozilla-inbound/rev/e69b67564187
https://hg.mozilla.org/integration/mozilla-inbound/rev/3736eec7dd73
https://hg.mozilla.org/integration/mozilla-inbound/rev/be3be63607b0

Thanks for the patches! Does this really need [leave open] on it anymore?

Comment 31

[Xin]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #30)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/79131be6fd64
> https://hg.mozilla.org/integration/mozilla-inbound/rev/bc8070070b06
> https://hg.mozilla.org/integration/mozilla-inbound/rev/af01ad72d579
> https://hg.mozilla.org/integration/mozilla-inbound/rev/125d104b58be
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e69b67564187
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3736eec7dd73
> https://hg.mozilla.org/integration/mozilla-inbound/rev/be3be63607b0
> 
> Thanks for the patches! Does this really need [leave open] on it anymore?

No.

Comment 32

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/79131be6fd64
https://hg.mozilla.org/mozilla-central/rev/bc8070070b06
https://hg.mozilla.org/mozilla-central/rev/af01ad72d579
https://hg.mozilla.org/mozilla-central/rev/125d104b58be
https://hg.mozilla.org/mozilla-central/rev/e69b67564187
https://hg.mozilla.org/mozilla-central/rev/3736eec7dd73
https://hg.mozilla.org/mozilla-central/rev/be3be63607b0

Comment 33

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Dave, look like you may not have noticed my note about the possible typo in the add-on sdk patch in comment 25

Comment 34

[Dave Townsend [:mossop]]

(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.