Last Comment Bug 753862 - Settings API: electrolysis support
: Settings API: electrolysis support
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: Christoph Kerschbaumer [:ckerschb (@TPAC)]
:
Mentors:
: 743018 (view as bug list)
Depends on: 762128
Blocks: e10s-device-apis 757614
  Show dependency treegraph
 
Reported: 2012-05-10 10:14 PDT by Gregor Wagner [:gwagner]
Modified: 2012-06-06 10:31 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WiP (8.64 KB, patch)
2012-05-16 15:36 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Splinter Review
basic implementation to send setting changes across processes (9.84 KB, patch)
2012-05-29 22:50 PDT, Christoph Kerschbaumer [:ckerschb (@TPAC)]
no flags Details | Diff | Splinter Review
WiP: implementation to send setting changes across processes (8.71 KB, patch)
2012-05-30 10:34 PDT, Christoph Kerschbaumer [:ckerschb (@TPAC)]
no flags Details | Diff | Splinter Review
updated the patch following advice from the last comment (5.97 KB, patch)
2012-05-30 20:50 PDT, Christoph Kerschbaumer [:ckerschb (@TPAC)]
no flags Details | Diff | Splinter Review
updated patch following advice from last comment (9.54 KB, patch)
2012-05-31 11:32 PDT, Christoph Kerschbaumer [:ckerschb (@TPAC)]
no flags Details | Diff | Splinter Review
uploaded the wrong diff in last comment (8.57 KB, patch)
2012-05-31 11:42 PDT, Christoph Kerschbaumer [:ckerschb (@TPAC)]
no flags Details | Diff | Splinter Review
updated patch following advice from last comment (8.47 KB, patch)
2012-05-31 13:27 PDT, Christoph Kerschbaumer [:ckerschb (@TPAC)]
no flags Details | Diff | Splinter Review
put the accidentally deleted lines of code back in the patch (8.73 KB, patch)
2012-06-01 09:17 PDT, Christoph Kerschbaumer [:ckerschb (@TPAC)]
fabrice: review-
Details | Diff | Splinter Review
updated code following review suggestions (9.08 KB, patch)
2012-06-03 11:03 PDT, Christoph Kerschbaumer [:ckerschb (@TPAC)]
fabrice: review+
Details | Diff | Splinter Review

Description Gregor Wagner [:gwagner] 2012-05-10 10:14:26 PDT
We need to send setting-changes across processes.
Comment 1 Gregor Wagner [:gwagner] 2012-05-16 15:36:58 PDT
Created attachment 624566 [details] [diff] [review]
WiP

Some basic setups and basic control flow. now working version.
Comment 2 Christoph Kerschbaumer [:ckerschb (@TPAC)] 2012-05-29 22:50:15 PDT
Created attachment 628237 [details] [diff] [review]
basic implementation to send setting changes across processes
Comment 3 Christoph Kerschbaumer [:ckerschb (@TPAC)] 2012-05-30 10:34:54 PDT
Created attachment 628380 [details] [diff] [review]
WiP: implementation to send setting changes across processes
Comment 4 Gregor Wagner [:gwagner] 2012-05-30 11:16:36 PDT
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] 2012-05-30 16:55:43 PDT
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 (@TPAC)] 2012-05-30 20:50:46 PDT
Created attachment 628591 [details] [diff] [review]
updated the patch following advice from the last comment
Comment 7 Gregor Wagner [:gwagner] 2012-05-31 09:00:13 PDT
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 (@TPAC)] 2012-05-31 11:32:07 PDT
Created attachment 628830 [details] [diff] [review]
updated patch following advice from last comment
Comment 9 Christoph Kerschbaumer [:ckerschb (@TPAC)] 2012-05-31 11:42:09 PDT
Created attachment 628834 [details] [diff] [review]
uploaded the wrong diff in last comment
Comment 10 Gregor Wagner [:gwagner] 2012-05-31 11:53:56 PDT
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 (@TPAC)] 2012-05-31 13:27:20 PDT
Created attachment 628881 [details] [diff] [review]
updated patch following advice from last comment
Comment 12 Gregor Wagner [:gwagner] 2012-05-31 16:10:10 PDT
*** Bug 743018 has been marked as a duplicate of this bug. ***
Comment 13 Gregor Wagner [:gwagner] 2012-05-31 17:30:23 PDT
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 (@TPAC)] 2012-06-01 09:17:58 PDT
Created attachment 629216 [details] [diff] [review]
put the accidentally deleted lines of code back in the patch
Comment 15 [:fabrice] Fabrice Desré 2012-06-01 12:15:10 PDT
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 (@TPAC)] 2012-06-03 11:03:38 PDT
Created attachment 629624 [details] [diff] [review]
updated code following review suggestions
Comment 17 Christoph Kerschbaumer [:ckerschb (@TPAC)] 2012-06-03 11:07:47 PDT

(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 19 Gregor Wagner [:gwagner] 2012-06-04 16:15:29 PDT
Oh I forgot. Great work :)
Comment 20 Geoff Lankow (:darktrojan) 2012-06-05 06:18:14 PDT
https://hg.mozilla.org/mozilla-central/rev/a7cbb6716a5b

Note You need to log in before you can comment on or make changes to this bug.