Settings API: Add service

RESOLVED FIXED in mozilla15

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gwagner, Assigned: gwagner)

Tracking

Trunk
mozilla15
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 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?
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.
(Assignee)

Comment 3

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

Updated

5 years ago
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.
Blocks: 741862
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
Blocks: 744308
(Assignee)

Comment 8

5 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...
(Assignee)

Comment 9

5 years ago
Created attachment 615293 [details] [diff] [review]
WiP

Settings Service for c++ chrome code.
(Assignee)

Comment 10

5 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);
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 12

5 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.
(Assignee)

Comment 13

5 years ago
Created attachment 615631 [details] [diff] [review]
Version 2

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

Comment 14

5 years ago
dhylands: do you have a JS context when you want to use this API for bug 737153?
(Assignee)

Comment 15

5 years ago
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.
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+
(Assignee)

Comment 17

5 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.
(Assignee)

Comment 18

5 years ago
Created attachment 617572 [details] [diff] [review]
patch
Attachment #616012 - Attachment is obsolete: true
Attachment #617572 - Flags: review?(fabrice)
(Assignee)

Comment 19

5 years ago
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?
(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
(Assignee)

Comment 22

5 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!
Blocks: 729440
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+
(Assignee)

Comment 24

5 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.
(Assignee)

Comment 25

5 years ago
Created attachment 618853 [details] [diff] [review]
patch

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+
(Assignee)

Comment 27

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

Comment 28

5 years ago
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.
Attachment #619190 - Flags: review?(fabrice)
Attachment #619190 - Flags: review?(fabrice) → review+
(Assignee)

Comment 29

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/df3acd833280
http://hg.mozilla.org/mozilla-central/rev/df3acd833280

I don't see a changeset for the tests. Did they ever land?
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
(Assignee)

Comment 31

5 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.
Depends on: 747581
(Assignee)

Comment 32

5 years ago
Created attachment 620788 [details] [diff] [review]
Tests
Attachment #617713 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #620788 - Flags: review?(fabrice)
Attachment #620788 - Flags: review?(fabrice) → review+
(Assignee)

Comment 33

5 years ago
Tests: https://hg.mozilla.org/integration/mozilla-inbound/rev/a0ec06f6c00d
https://hg.mozilla.org/mozilla-central/rev/a0ec06f6c00d
Flags: in-testsuite? → in-testsuite+
Depends on: 758735
You need to log in before you can comment on or make changes to this bug.