743336 - Settings API: Add service

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Gregor Wagner [:gwagner]]

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?

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 3

[Gregor Wagner [:gwagner]]

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.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 5

[Philipp von Weitershausen [:philikon]]

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

Comment 6

[Philipp von Weitershausen [:philikon]]

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.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 8

[Gregor Wagner [:gwagner]]

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

Comment 9

[Gregor Wagner [:gwagner]]

Attachment: WiP

Settings Service for c++ chrome code.

Comment 10

[Gregor Wagner [:gwagner]]

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

Comment 11

[Mounir Lamouri (:mounir)]

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.

Comment 12

[Gregor Wagner [:gwagner]]

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

Comment 13

[Gregor Wagner [:gwagner]]

Attachment: Version 2

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

Comment 14

[Gregor Wagner [:gwagner]]

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

Comment 15

[Gregor Wagner [:gwagner]]

Attachment: Patch

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

Comment 16

[[:fabrice] Fabrice Desré]

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

Comment 17

[Gregor Wagner [:gwagner]]

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

Comment 18

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 19

[Gregor Wagner [:gwagner]]

Attachment: Tests

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

Comment 20

[Mounir Lamouri (:mounir)]

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

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 22

[Gregor Wagner [:gwagner]]

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

Comment 23

[[:fabrice] Fabrice Desré]

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?

Comment 24

[Gregor Wagner [:gwagner]]

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

Comment 25

[Gregor Wagner [:gwagner]]

Attachment: patch

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

Comment 26

[[:fabrice] Fabrice Desré]

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?

Comment 27

[Gregor Wagner [:gwagner]]

(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"];

Comment 28

[Gregor Wagner [:gwagner]]

Attachment: Fix

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.

Comment 29

[Gregor Wagner [:gwagner]]

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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

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

Comment 31

[Gregor Wagner [:gwagner]]

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

Comment 32

[Gregor Wagner [:gwagner]]

Attachment: Tests

Comment 33

[Gregor Wagner [:gwagner]]

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

Comment 34

[Ryan VanderMeulen [:RyanVM]]

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