Last Comment Bug 743336 - Settings API: Add service
: Settings API: Add service
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Gregor Wagner [:gwagner]
:
Mentors:
Depends on: 747581 758735
Blocks: 729440 737153 741862 744308
  Show dependency treegraph
 
Reported: 2012-04-06 13:19 PDT by Gregor Wagner [:gwagner]
Modified: 2012-05-25 12:12 PDT (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WiP (21.48 KB, patch)
2012-04-16 03:48 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
Version 2 (19.77 KB, patch)
2012-04-17 02:02 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
Patch (19.33 KB, patch)
2012-04-17 21:59 PDT, Gregor Wagner [:gwagner]
fabrice: feedback+
Details | Diff | Review
patch (19.52 KB, patch)
2012-04-23 11:51 PDT, Gregor Wagner [:gwagner]
fabrice: feedback+
Details | Diff | Review
Tests (5.73 KB, patch)
2012-04-23 17:41 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
patch (15.91 KB, patch)
2012-04-26 16:40 PDT, Gregor Wagner [:gwagner]
fabrice: review+
Details | Diff | Review
Fix (1.50 KB, patch)
2012-04-27 15:05 PDT, Gregor Wagner [:gwagner]
fabrice: review+
Details | Diff | Review
Tests (5.18 KB, patch)
2012-05-03 11:12 PDT, Gregor Wagner [:gwagner]
fabrice: review+
Details | Diff | Review

Description Gregor Wagner [:gwagner] 2012-04-06 13:19:09 PDT

    
Comment 1 Gregor Wagner [:gwagner] 2012-04-08 19:08:37 PDT
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) PTO Until July 5th 2012-04-09 10:30:07 PDT
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] 2012-04-09 10:48:25 PDT
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) PTO Until July 5th 2012-04-09 12:33:07 PDT
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] 2012-04-09 23:54:58 PDT
(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] 2012-04-10 00:00:38 PDT
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) PTO Until July 5th 2012-04-10 09:35:22 PDT
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] 2012-04-12 19:38:12 PDT
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] 2012-04-16 03:48:54 PDT
Created attachment 615293 [details] [diff] [review]
WiP

Settings Service for c++ chrome code.
Comment 10 Gregor Wagner [:gwagner] 2012-04-16 19:37:10 PDT
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) 2012-04-17 00:48:26 PDT
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] 2012-04-17 01:32:21 PDT
(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] 2012-04-17 02:02:24 PDT
Created attachment 615631 [details] [diff] [review]
Version 2

Another version with untyped get/set using jsvals and an additional setString.
Comment 14 Gregor Wagner [:gwagner] 2012-04-17 03:08:35 PDT
dhylands: do you have a JS context when you want to use this API for bug 737153?
Comment 15 Gregor Wagner [:gwagner] 2012-04-17 21:59:18 PDT
Created attachment 616012 [details] [diff] [review]
Patch

I changed the API to jsvals. We should now be able to store and retrieve objects as well.
Comment 16 [:fabrice] Fabrice Desré 2012-04-23 09:50:45 PDT
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] 2012-04-23 09:59:01 PDT
(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] 2012-04-23 11:51:38 PDT
Created attachment 617572 [details] [diff] [review]
patch
Comment 19 Gregor Wagner [:gwagner] 2012-04-23 17:41:01 PDT
Created attachment 617713 [details] [diff] [review]
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) 2012-04-24 02:25:31 PDT
(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 (PTO, back in 6/27) Yoshi Huang[:allstars.chh] 2012-04-25 20:29:00 PDT
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] 2012-04-25 20:37:31 PDT
(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é 2012-04-26 14:03:34 PDT
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] 2012-04-26 15:21:44 PDT
> > +
> > +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] 2012-04-26 16:40:03 PDT
Created attachment 618853 [details] [diff] [review]
patch

Removed SettingsServiceCallback from JS files.
This patch is based on Bug 746933
Comment 26 [:fabrice] Fabrice Desré 2012-04-27 13:38:04 PDT
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] 2012-04-27 14:29:07 PDT
(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] 2012-04-27 15:05:28 PDT
Created attachment 619190 [details] [diff] [review]
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 30 Ryan VanderMeulen [:RyanVM] 2012-04-29 14:50:27 PDT
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] 2012-04-30 10:06:53 PDT
(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] 2012-05-03 11:12:34 PDT
Created attachment 620788 [details] [diff] [review]
Tests
Comment 33 Gregor Wagner [:gwagner] 2012-05-08 11:29:48 PDT
Tests: https://hg.mozilla.org/integration/mozilla-inbound/rev/a0ec06f6c00d
Comment 34 Ryan VanderMeulen [:RyanVM] 2012-05-08 18:50:16 PDT
https://hg.mozilla.org/mozilla-central/rev/a0ec06f6c00d

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