Gregor Wagner [:gwagner] Assignee
	Description • 13 years ago

      No description provided.

	Gregor Wagner [:gwagner] Assignee
	Comment 1 • 13 years ago

Now the question is if we should leave the current SettingsManager.js implementation untouched and duplicate the functionality in the service or if we should change the current implementation to use the new service.
The obvious solution would be to move functionality to the service but it might be hard to keep the current behavior.
If we use a similar approach like the contacts api uses (with frame messenger) we would have a hard time to ensure the correct execution order given by the lock order.

Jonas, any thoughts?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 2 • 13 years ago

Do we need to expose the ability to set settings through the service too? If not, then it might make sense to *not* have the service work as a backend for the API. Instead the service could expose a more C++-y interface which operated on strings rather than JSON objects, and where you could pass in an array of settings which you wanted to read, and get back array of values for those settings.

	Gregor Wagner [:gwagner] Assignee
	Comment 3 • 13 years ago

So the use-case where we need a service right now is reading settings from the DB during startup.
I guess we could come up with a scenario where we want to put information into the DB during startup. Lets say we want to put a model number or some sim card info into the DB.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 4 • 13 years ago

I see.

I don't have a strong preference. Ultimately what we care about is code simplicity. If making the API sit on top of the service makes both the API and service implementation too complex, then "duplicating" the functionality might be the better option.

However I do think that we should make sure that the C++ API is able to do multiple write operations as a single atomic operation. I.e. that if the C++ code needs to do 3 set operations, then we should be sure that no web app can run a request against the settings while only one of those three set operations have run.

	Philipp von Weitershausen [:philikon]
	Comment 5 • 13 years ago

(In reply to Gregor Wagner [:gwagner] from comment #3)
> So the use-case where we need a service right now is reading settings from
> the DB during startup.
> I guess we could come up with a scenario where we want to put information
> into the DB during startup. Lets say we want to put a model number or some
> sim card info into the DB.

Indeed.

That said, I would like to offer a different point of view. You're proposing to make the settings DB the one source of truth and everybody has to be feed into it. My original impression of the settings API was that -- while the settings API would look uniform on the DOM side -- settings could live in whatever backend provided them. For instance, settings that are read from the SIM card would simply be provided by the RIL. So each Gecko component can register itself as a "settings provider" and manage its own set of settings somehow.

This would avoid having to duplicate data from e.g. the SIM card into the database at startup -- which would have to have to happen every single time. Instead we'd lazily fetch this information when needed directly from the source of truth.

	Philipp von Weitershausen [:philikon]
	Comment 6 • 13 years ago

Btw, I think for a lot of Gecko services, a "prefs settings provider" that simply maps settings to Gecko prefs that could then be read by whatever Gecko component needs them would a sufficient and pretty simple solution.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 7 • 13 years ago

I'm absolutely ok with using other backends in addition to IndexedDB. We also talked about this for the wifi settings where the deamon we are using wants some settings to be in a configuration file iirc.

One complication is that the current implementation of the settings API actually implement a sort of transaction. I.e. if something goes wrong we roll back all changes done inside the settings lock. This is definitely cool, but certainly not critical. It would be a bit complex to implement correctly if we keep settings outside of the database since we can't roll back changes outside of the database if we crash for example.

	Gregor Wagner [:gwagner] Assignee
	Comment 8 • 13 years ago

I don't think we can use the API from C++ side the same way we do from JS. Philikon had the idea to introduce typed methods as used in the desktop settings service: setBool, setString, setObject and similar with getBool, getString...

	Gregor Wagner [:gwagner] Assignee
	Comment 9 • 13 years ago

Settings Service for c++ chrome code.

	Gregor Wagner [:gwagner] Assignee
	Comment 10 • 13 years ago

The first version only supports primitive types for get/set from C++.
We have to think more about passing objects around.
We also can't use the DOMRequest approach because we don't have a window. I added a nsISettingsServiceCallback that provides a handle function for the user to implement.

The signature for getBool for example:
void getBool(in string aName, in nsISettingsServiceCallback aCallback);

The Callback interface looks like:
  [implicit_jscontext]
  void handle(in DOMString aName, in jsval aResult);
  [implicit_jscontext]
  void handleError(in DOMString aName);

	Mounir Lamouri (:mounir)
	Comment 11 • 13 years ago

In my opinion, it would be better to have a 1:1 match with the API and use a method taking a jsval in C++ instead of having a method per type.

	Gregor Wagner [:gwagner] Assignee
	Comment 12 • 13 years ago

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #11)
> In my opinion, it would be better to have a 1:1 match with the API and use a
> method taking a jsval in C++ instead of having a method per type.

That works fine for bool, int and doubles but for strings we need a context to get a JSString* for STRING_TO_JSVAL.

	Gregor Wagner [:gwagner] Assignee
	Comment 13 • 13 years ago

Another version with untyped get/set using jsvals and an additional setString.

	Gregor Wagner [:gwagner] Assignee
	Comment 14 • 13 years ago

dhylands: do you have a JS context when you want to use this API for bug 737153?

	Gregor Wagner [:gwagner] Assignee
	Comment 15 • 13 years ago

I changed the API to jsvals. We should now be able to store and retrieve objects as well.

	[:fabrice] Fabrice Desré
	Comment 16 • 13 years ago

Comment on attachment 616012 [details] [diff] [review]
Patch

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

::: dom/interfaces/settings/nsISettingsService.idl
@@ +16,5 @@
> +[scriptable, uuid(3ab3cbc0-8513-11e1-b0c4-0800200c9a66)]
> +interface nsISettingsServiceLock : nsISupports
> +{
> +  void set(in string aName, in jsval aValue, in nsISettingsServiceCallback aCallback);
> +  void get(in string aName, in nsISettingsServiceCallback aCallback);

why not use a DOMRequest, and set req.result to {name: aName, value: aResult} when firing onsuccess ?

::: dom/settings/SettingsDB.jsm
@@ +42,5 @@
> +    let self = this;
> +    debug("try to open database:" + SETTINGSDB_NAME + " " + SETTINGSDB_VERSION + " " + this.db);
> +    let req = aGlobal.mozIndexedDB.open(SETTINGSDB_NAME, SETTINGSDB_VERSION);
> +    debug("req: " + req);
> +    req.onsuccess = function (event) {

nit: name anonymous functions

	Gregor Wagner [:gwagner] Assignee
	Comment 17 • 13 years ago

(In reply to Fabrice Desré [:fabrice] from comment #16)
> Comment on attachment 616012 [details] [diff] [review]
> Patch
> 
> Review of attachment 616012 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/interfaces/settings/nsISettingsService.idl
> @@ +16,5 @@
> > +[scriptable, uuid(3ab3cbc0-8513-11e1-b0c4-0800200c9a66)]
> > +interface nsISettingsServiceLock : nsISupports
> > +{
> > +  void set(in string aName, in jsval aValue, in nsISettingsServiceCallback aCallback);
> > +  void get(in string aName, in nsISettingsServiceCallback aCallback);
> 
> why not use a DOMRequest, and set req.result to {name: aName, value:
> aResult} when firing onsuccess ?


DOMRequest needs a window and we don't always have a window when we use this API.

	Gregor Wagner [:gwagner] Assignee
	Comment 19 • 13 years ago

I don't know if it makes much sense to land the tests as well. Can we do binary tests for B2G?

	Mounir Lamouri (:mounir)
	Comment 20 • 13 years ago

(In reply to Gregor Wagner [:gwagner] from comment #19)
> Created attachment 617713 [details] [diff] [review]
> Tests
> 
> I don't know if it makes much sense to land the tests as well.

It always makes sense to have tests! :)

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 21 • 13 years ago

Comment on attachment 617572 [details] [diff] [review]
patch

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

::: dom/settings/SettingsService.js
@@ +59,5 @@
> +          req.onsuccess = function() {
> +            debug("set on success");
> +            lock._open = true;
> +            if (callback)
> +              callback.handle("SET" + name, value);

Hi, gwagner
  I got a question to ask, is the "SET" added on purpose?
  If so, should we add "GET" likewise in get?
thanks

	Gregor Wagner [:gwagner] Assignee
	Comment 22 • 13 years ago

(In reply to Yoshi Huang[:yoshi] from comment #21)
> Comment on attachment 617572 [details] [diff] [review]
> patch
> 
> Review of attachment 617572 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/settings/SettingsService.js
> @@ +59,5 @@
> > +          req.onsuccess = function() {
> > +            debug("set on success");
> > +            lock._open = true;
> > +            if (callback)
> > +              callback.handle("SET" + name, value);
> 
> Hi, gwagner
>   I got a question to ask, is the "SET" added on purpose?
>   If so, should we add "GET" likewise in get?
> thanks

Oh that was just for debug purpose. Thanks for catching it!

	[:fabrice] Fabrice Desré
	Comment 23 • 13 years ago

Comment on attachment 617572 [details] [diff] [review]
patch

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

::: dom/settings/SettingsService.js
@@ +182,5 @@
> +const SETTINGSSERVICECALLBACK_CONTRACTID = "@mozilla.org/settingsServiceCallback;1";
> +const SETTINGSSERVICECALLBACK_CID        = Components.ID("{83d67430-8516-11e1-b0c4-0800200c9a66}");
> +const nsISettingsServiceCallback         = Ci.nsISettingsServiceCallback;
> +
> +function SettingsServiceCallback() {debug("SSC constructor")};

nit:  {
 debug(...)
}

and blank line before .prototype

@@ +195,5 @@
> +                                     interfaces: [nsISettingsServiceCallback],
> +                                     flags: nsIClassInfo.DOM_OBJECT})
> +}
> +
> +const NSGetFactory = XPCOMUtils.generateNSGetFactory([SettingsService, SettingsServiceLock, SettingsServiceCallback])

SettingsServiceCallback is exported there, but not in the .manifest.

Do we really need it?

	Gregor Wagner [:gwagner] Assignee
	Comment 24 • 13 years ago

> > +
> > +const NSGetFactory = XPCOMUtils.generateNSGetFactory([SettingsService, SettingsServiceLock, SettingsServiceCallback])
> 
> SettingsServiceCallback is exported there, but not in the .manifest.
> 
> Do we really need it?

Right, I tried to write tests for it in JS but it wasn't very useful.

	Gregor Wagner [:gwagner] Assignee
	Comment 25 • 13 years ago

Removed SettingsServiceCallback from JS files.
This patch is based on Bug 746933

	[:fabrice] Fabrice Desré
	Comment 26 • 13 years ago

Comment on attachment 618853 [details] [diff] [review]
patch

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

r=me pending answer to comment

::: dom/settings/SettingsManager.js
@@ +35,5 @@
>  
>    process: function process() {
>      let lock = this;
>      lock._open = false;
> +    let store = lock._transaction.objectStore(SETTINGSSTORE_NAME);

SETTINGSSTORE_NAME is defined in SettingDB.jsm - how do you get it in scope here?

	Gregor Wagner [:gwagner] Assignee
	Comment 27 • 13 years ago

(In reply to Fabrice Desré [:fabrice] from comment #26)
> Comment on attachment 618853 [details] [diff] [review]
> patch
> 
> Review of attachment 618853 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me pending answer to comment
> 
> ::: dom/settings/SettingsManager.js
> @@ +35,5 @@
> >  
> >    process: function process() {
> >      let lock = this;
> >      lock._open = false;
> > +    let store = lock._transaction.objectStore(SETTINGSSTORE_NAME);
> 
> SETTINGSSTORE_NAME is defined in SettingDB.jsm - how do you get it in scope
> here?

Hm I did following in SettingsDB.jsm:
let EXPORTED_SYMBOLS = ["SettingsDB", "SETTINGSDB_NAME", "SETTINGSSTORE_NAME"];

	Gregor Wagner [:gwagner] Assignee
	Comment 28 • 13 years ago

The problem was that SettingsDB.jsm was not included.
Fabrice, great job finding it! I hope we will have some automated testing for that soon.

	Gregor Wagner [:gwagner] Assignee
	Comment 29 • 13 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 12 years ago

http://hg.mozilla.org/mozilla-central/rev/df3acd833280

I don't see a changeset for the tests. Did they ever land?

	Gregor Wagner [:gwagner] Assignee
	Comment 31 • 12 years ago

(In reply to Ryan VanderMeulen from comment #30)
> http://hg.mozilla.org/mozilla-central/rev/df3acd833280
> 
> I don't see a changeset for the tests. Did they ever land?

It depends on bug 747581.

	Gregor Wagner [:gwagner] Assignee
	Comment 33 • 12 years ago

Tests: https://hg.mozilla.org/integration/mozilla-inbound/rev/a0ec06f6c00d

	Ryan VanderMeulen [:RyanVM]
	Comment 34 • 12 years ago

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