753862 - Settings API: electrolysis support

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gregor Wagner [:gwagner]]

We need to send setting-changes across processes.

Comment 1

[Gregor Wagner [:gwagner]]

Attachment: WiP

Some basic setups and basic control flow. now working version.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: basic implementation to send setting changes across processes

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: WiP: implementation to send setting changes across processes

Comment 4

[Gregor Wagner [:gwagner]]

Comment on attachment 628380 [details] [diff] [review]
WiP: implementation to send setting changes across processes


>+
>+  receiveMessage: function(aMessage) {
>+    debug("receiveMessage " + aMessage.name);
>+    let msg = aMessage.json;
>+    switch (aMessage.name) {
>+      case "Settings:Changed":
>+        debug("Settings:Changed!");
>+        ppmm.sendAsyncMessage("Settings:Change:Return:OK", {key: msg.key, value: msg.value});

We also have to inform listeners in the parent process.
So we need the Services.obs.... 


>+        break;
>+      default: 
>+        debug("Wrong message: " + aMessage.name);
>+    }
>+  }
>+}
>+
>+SettingsChangeNotifier.init();
>diff -r 65fb8b9ea0b7 dom/settings/SettingsManager.js
>--- a/dom/settings/SettingsManager.js	Wed May 16 12:03:18 2012 -0700
>+++ b/dom/settings/SettingsManager.js	Wed May 30 10:32:04 2012 -0700
> 
> Cu.import("resource://gre/modules/SettingsQueue.jsm");
> Cu.import("resource://gre/modules/SettingsDB.jsm");
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> Cu.import("resource://gre/modules/Services.jsm");
> 
>+XPCOMUtils.defineLazyGetter(Services, "DOMRequest", function() {
>+  return Cc["@mozilla.org/dom/dom-request-service;1"].getService(Ci.nsIDOMRequestService);
>+});

Is this needed?


>@@ -215,21 +226,45 @@ SettingsManager.prototype = {
>     this._settingsDB.ensureDB(
>       function() { lock.createTransactionAndProcess(); },
>       function() { dump("ensureDB error cb!\n"); },
>       myGlobal );
>     this.nextTick(function() { this._open = false; }, lock);
>     return lock;
>   },
> 
>+  receiveMessage: function(aMessage) {
>+    debug("Settings::receiveMessage: " + aMessage.name);
>+    let msg = aMessage.json;
>+    let contacts = msg.contacts;

^ remove :)

Otherwise looks good. Time to remove the debug code.

Comment 5

[Gregor Wagner [:gwagner]]

Alright we figured out the c++ chrome code notification broadcast. 
cjones and mrbkap think that's enough to just do observer-service notification in the parent process.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: updated the patch following advice from the last comment

Comment 7

[Gregor Wagner [:gwagner]]

Comment on attachment 628591 [details] [diff] [review]
updated the patch following advice from the last comment


>+
>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+Cu.import("resource://gre/modules/Services.jsm");
>+
>+XPCOMUtils.defineLazyGetter(this, "ppmm", function() {
>+  return Cc["@mozilla.org/parentprocessmessagemanager;1"].getService(Ci.nsIFrameMessageManager);
>+});
>+
>+let myGlobal = this;

^ remove


>@@ -66,16 +70,17 @@ SettingsLock.prototype = {
> 
>             req.onsuccess = function() { 
>               lock._open = true;
>               Services.DOMRequest.fireSuccess(request, 0);
>               Services.obs.notifyObservers(lock, "mozsettings-changed", JSON.stringify({
>                 key: key,
>                 value: info.settings[key]
>               }));
>+              cpmm.sendAsyncMessage("Settings:Changed", {key: key, value: info.settings[key]});
>               lock._open = false;
>             };
> 
>             req.onerror = function() { Services.DOMRequest.fireError(request, 0) };
>           }
>           break;
>         case "get":
>           if (info.name == "*") {

You also need to remove the registered messages for the child.
Hopefully the leak goes away with that.
And Services.obs.notifyObservers should be removed from the child.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: updated patch following advice from last comment

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: uploaded the wrong diff in last comment

Comment 10

[Gregor Wagner [:gwagner]]

Comment on attachment 628834 [details] [diff] [review]
uploaded the wrong diff in last comment

>+let SettingsChangeNotifier = {
>+  init: function() {
>+    this._messages = ["Settings:Changed"];
>+    this._messages.forEach((function(msgName) {
>+      ppmm.addMessageListener(msgName, this);
>+    }).bind(this));
>+
>+    Services.obs.addObserver(this, "profile-before-change", false);
>+  },
>+
>+  observe: function(aSubject, aTopic, aData) {
>+    myGlobal = null;

^ remove


>-      let data = JSON.parse(aData);
>-      debug('data:' + data.key + ':' + data.value + '\n');
>-
>-      let event = new this._window.MozSettingsEvent("settingchanged", {
>-        settingName: data.key,
>-        settingValue: data.value
>-      });
>-
>-      this._onsettingchange.handleEvent(event);
>-    }
>+    } 

^ whitespace

Looks good to me but I shouldn't review this because I submitted the first version.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Attachment: updated patch following advice from last comment

Comment 13

[Gregor Wagner [:gwagner]]

Comment on attachment 628881 [details] [diff] [review]
updated patch following advice from last comment

There was some misunderstanding. Christoph deleted too many lines :)

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Attachment: put the accidentally deleted lines of code back in the patch

Comment 15

[[:fabrice] Fabrice Desré]

Comment on attachment 629216 [details] [diff] [review]
put the accidentally deleted lines of code back in the patch

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

That's almost ready! The remaining issue is setting the message listener in SettingsManager.js (unless I missed the obvious)

::: dom/settings/SettingsChangeNotifier.jsm
@@ +8,5 @@
> +let DEBUG = false;
> +if (DEBUG)
> +  debug = function (s) { dump("-*- SettingsManager: " + s + "\n"); };
> +else
> +  debug = function (s) {};

just do :
function debug() {
 dump(...);
}

and comment the dump line if needed. Do that also in .js

@@ +29,5 @@
> +    this._messages = ["Settings:Changed"];
> +    this._messages.forEach((function(msgName) {
> +      ppmm.addMessageListener(msgName, this);
> +    }).bind(this));
> +

If you don't plan to add other messages to listen to, just do |ppm.addMessageListener("Settings:Changed", this)|

@@ +30,5 @@
> +    this._messages.forEach((function(msgName) {
> +      ppmm.addMessageListener(msgName, this);
> +    }).bind(this));
> +
> +    Services.obs.addObserver(this, "profile-before-change", false);

can you comment why you're listening for profile-before-change? Since you use it to cleanup, we usually use xpcom-shutdown.

@@ +36,5 @@
> +
> +  observe: function(aSubject, aTopic, aData) {
> +    this._messages.forEach((function(msgName) {
> +      ppmm.removeMessageListener(msgName, this);
> +    }).bind(this));

idem.

@@ +46,5 @@
> +  receiveMessage: function(aMessage) {
> +    let msg = aMessage.json;
> +    switch (aMessage.name) {
> +      case "Settings:Changed":
> +        ppmm.sendAsyncMessage("Settings:Change:Return:OK", {key: msg.key, value: msg.value});

nit: spaces around the json braces.

::: dom/settings/SettingsManager.js
@@ +70,5 @@
>  
>              req.onsuccess = function() { 
>                lock._open = true;
>                Services.DOMRequest.fireSuccess(request, 0);
> +              cpmm.sendAsyncMessage("Settings:Changed", {key: key, value: info.settings[key]});

Nit: spaces in the json object:
{ key:... [key] }

@@ +232,5 @@
> +    debug("Settings::receiveMessage: " + aMessage.name);
> +    let msg = aMessage.json;
> +
> +    switch (aMessage.name) {
> +      case "Settings:Change:Return:OK":

where are you setting the listener for this message?

@@ +280,5 @@
>          this._requests = null;
>          this._window = null;
>          this._innerWindowID = null;
>          this._onsettingchange = null;
>          this._settingsDB.close();

add |cpmm = null;|

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Attachment: updated code following review suggestions

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Fabrice Desré [:fabrice] from comment #15)
> Comment on attachment 629216 [details] [diff] [review]
> put the accidentally deleted lines of code back in the patch
> 
> Review of attachment 629216 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That's almost ready! The remaining issue is setting the message listener in
> SettingsManager.js (unless I missed the obvious)
> 
> ::: dom/settings/SettingsChangeNotifier.jsm
> @@ +8,5 @@
> > +let DEBUG = false;
> > +if (DEBUG)
> > +  debug = function (s) { dump("-*- SettingsManager: " + s + "\n"); };
> > +else
> > +  debug = function (s) {};
> 
> just do :
> function debug() {
>  dump(...);
> }
> 
> and comment the dump line if needed. Do that also in .js
> 
> @@ +29,5 @@
> > +    this._messages = ["Settings:Changed"];
> > +    this._messages.forEach((function(msgName) {
> > +      ppmm.addMessageListener(msgName, this);
> > +    }).bind(this));
> > +
> 
> If you don't plan to add other messages to listen to, just do
> |ppm.addMessageListener("Settings:Changed", this)|

good point - changed that.

> @@ +30,5 @@
> > +    this._messages.forEach((function(msgName) {
> > +      ppmm.addMessageListener(msgName, this);
> > +    }).bind(this));
> > +
> > +    Services.obs.addObserver(this, "profile-before-change", false);
> 
> can you comment why you're listening for profile-before-change? Since you
> use it to cleanup, we usually use xpcom-shutdown.

changed it.

> @@ +36,5 @@
> > +
> > +  observe: function(aSubject, aTopic, aData) {
> > +    this._messages.forEach((function(msgName) {
> > +      ppmm.removeMessageListener(msgName, this);
> > +    }).bind(this));
> 
> idem.
> 
> @@ +46,5 @@
> > +  receiveMessage: function(aMessage) {
> > +    let msg = aMessage.json;
> > +    switch (aMessage.name) {
> > +      case "Settings:Changed":
> > +        ppmm.sendAsyncMessage("Settings:Change:Return:OK", {key: msg.key, value: msg.value});
> 
> nit: spaces around the json braces.
> 
> ::: dom/settings/SettingsManager.js
> @@ +70,5 @@
> >  
> >              req.onsuccess = function() { 
> >                lock._open = true;
> >                Services.DOMRequest.fireSuccess(request, 0);
> > +              cpmm.sendAsyncMessage("Settings:Changed", {key: key, value: info.settings[key]});
> 
> Nit: spaces in the json object:
> { key:... [key] }
> 
> @@ +232,5 @@
> > +    debug("Settings::receiveMessage: " + aMessage.name);
> > +    let msg = aMessage.json;
> > +
> > +    switch (aMessage.name) {
> > +      case "Settings:Change:Return:OK":
> 
> where are you setting the listener for this message?

that was one of the lines I accidentally removed the other day.
> 
> @@ +280,5 @@
> >          this._requests = null;
> >          this._window = null;
> >          this._innerWindowID = null;
> >          this._onsettingchange = null;
> >          this._settingsDB.close();
> 
> add |cpmm = null;|

Comment 18

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7cbb6716a5b

Comment 19

[Gregor Wagner [:gwagner]]

Oh I forgot. Great work :)

Comment 20

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/mozilla-central/rev/a7cbb6716a5b