Closed Bug 743336 Opened 8 years ago Closed 8 years ago

Settings API: Add service

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: gwagner, Assigned: gwagner)

References

Details

Attachments

(3 files, 5 obsolete files)

No description provided.
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?
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.
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.
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.
Assignee: nobody → anygregor
(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.
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.
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
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.
Blocks: 737153
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...
Attached patch WiP (obsolete) — Splinter Review
Settings Service for c++ chrome code.
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);
Attachment #615293 - Flags: feedback?(fabrice)
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.
(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.
Attached patch Version 2 (obsolete) — Splinter Review
Another version with untyped get/set using jsvals and an additional setString.
dhylands: do you have a JS context when you want to use this API for bug 737153?
Attached patch Patch (obsolete) — Splinter Review
I changed the API to jsvals. We should now be able to store and retrieve objects as well.
Attachment #615293 - Attachment is obsolete: true
Attachment #615631 - Attachment is obsolete: true
Attachment #615293 - Flags: feedback?(fabrice)
Attachment #616012 - Flags: feedback?(fabrice)
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
Attachment #616012 - Flags: feedback?(fabrice) → feedback+
(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.
Attached patch patch (obsolete) — Splinter Review
Attachment #616012 - Attachment is obsolete: true
Attachment #617572 - Flags: review?(fabrice)
Attached patch Tests (obsolete) — Splinter Review
I don't know if it makes much sense to land the tests as well. Can we do binary tests for B2G?
(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 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
(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 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?
Attachment #617572 - Flags: review?(fabrice) → feedback+
> > +
> > +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.
Attached patch patchSplinter Review
Removed SettingsServiceCallback from JS files.
This patch is based on Bug 746933
Attachment #617572 - Attachment is obsolete: true
Attachment #618853 - Flags: review?(fabrice)
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?
Attachment #618853 - Flags: review?(fabrice) → review+
(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"];
Attached patch FixSplinter Review
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.
Attachment #619190 - Flags: review?(fabrice)
Attachment #619190 - Flags: review?(fabrice) → review+
http://hg.mozilla.org/mozilla-central/rev/df3acd833280

I don't see a changeset for the tests. Did they ever land?
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
(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.
Depends on: 747581
Attached patch TestsSplinter Review
Attachment #617713 - Attachment is obsolete: true
Attachment #620788 - Flags: review?(fabrice)
Attachment #620788 - Flags: review?(fabrice) → review+
Depends on: 758735
You need to log in before you can comment on or make changes to this bug.