Last Comment Bug 678695 - Settings API
: Settings API
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Gregor Wagner [:gwagner]
:
Mentors:
https://wiki.mozilla.org/WebAPI/Setti...
Depends on: 722626 739680 742065 743018 750992 754142 758486 764003 772127
Blocks: webapi 714357 712778 b2g-demo-phone 740337
  Show dependency treegraph
 
Reported: 2011-08-12 23:37 PDT by Andreas Gal :gal
Modified: 2013-02-06 02:10 PST (History)
28 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
IDL (3.07 KB, patch)
2012-02-08 15:50 PST, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
WiP: Basic functionality (25.26 KB, patch)
2012-02-09 14:21 PST, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
WiP (31.05 KB, patch)
2012-02-09 16:11 PST, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
JS part (34.05 KB, patch)
2012-02-13 12:55 PST, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
IDL (3.13 KB, patch)
2012-02-29 11:31 PST, Gregor Wagner [:gwagner]
jonas: review+
Details | Diff | Review
WiP: Basic functionality (28.86 KB, patch)
2012-03-02 16:51 PST, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
WiP (33.00 KB, patch)
2012-03-07 17:06 PST, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
Part1: JS + DB (39.37 KB, patch)
2012-03-13 14:41 PDT, Gregor Wagner [:gwagner]
fabrice: review+
Details | Diff | Review
WiP Part2: Events (18.75 KB, patch)
2012-03-14 14:04 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
AllInOne (47.85 KB, patch)
2012-03-23 15:46 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
Events (30.42 KB, patch)
2012-03-23 16:25 PDT, Gregor Wagner [:gwagner]
bugs: review-
Details | Diff | Review
Events (19.96 KB, patch)
2012-03-26 12:57 PDT, Gregor Wagner [:gwagner]
bugs: review+
Details | Diff | Review
JS nits (12.98 KB, patch)
2012-03-26 13:39 PDT, Gregor Wagner [:gwagner]
fabrice: review+
Details | Diff | Review
AllInOne (54.26 KB, patch)
2012-03-26 13:58 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Review

Description Andreas Gal :gal 2011-08-12 23:37:49 PDT
We need an API to configure device settings.
Comment 1 Gregor Wagner [:gwagner] 2012-02-08 15:50:24 PST
Created attachment 595563 [details] [diff] [review]
IDL

IDL files
Comment 2 Gregor Wagner [:gwagner] 2012-02-09 14:21:43 PST
Created attachment 595878 [details] [diff] [review]
WiP: Basic functionality

Can set and get values.
Comment 3 Gregor Wagner [:gwagner] 2012-02-09 16:11:06 PST
Created attachment 595899 [details] [diff] [review]
WiP

+ tests
Comment 4 Gregor Wagner [:gwagner] 2012-02-13 12:55:27 PST
Created attachment 596761 [details] [diff] [review]
JS part
Comment 5 Mounir Lamouri (:mounir) 2012-02-13 14:42:29 PST
It seems like we haven't find a consensus regarding the API and this API doesn't seem to look like what is currently discussed. If B2G needs something ASAP, I believe Vivien did implement something for b2g only.
Comment 6 Jonas Sicking (:sicking) PTO Until July 5th 2012-02-26 20:27:56 PST
Comment on attachment 595563 [details] [diff] [review]
IDL

We should do the new lock-based API instead...
Comment 7 Gregor Wagner [:gwagner] 2012-02-29 11:31:41 PST
Created attachment 601682 [details] [diff] [review]
IDL

IDL Part
Comment 8 Gregor Wagner [:gwagner] 2012-03-02 16:51:09 PST
Created attachment 602538 [details] [diff] [review]
WiP: Basic functionality
Comment 9 Jonas Sicking (:sicking) PTO Until July 5th 2012-03-03 15:07:50 PST
Comment on attachment 601682 [details] [diff] [review]
IDL

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

r=me with that changed.

::: dom/interfaces/settings/nsIDOMSettingsManager.idl
@@ +18,5 @@
> +  // result contains the value of the setting.
> +  nsIDOMDOMRequest get(in DOMString name);
> +
> +  // result contains JSON object with name/value pairs.
> +  nsIDOMDOMRequest getJSON(in jsval names); //DOMString[]

Why do we need separate get and getJSON function names? Can't we just have a single get function which either takes a string or an Array?

@@ +36,5 @@
> +{
> +  readonly attribute DOMString  settingName;
> +  readonly attribute nsIVariant settingValue;
> +
> +  void initSettingsEvent(in DOMString typeArg,

Remove this init function. The constructor is enough.
Comment 10 Gregor Wagner [:gwagner] 2012-03-07 17:06:24 PST
Created attachment 603919 [details] [diff] [review]
WiP

JS implementation
Comment 11 Gregor Wagner [:gwagner] 2012-03-13 14:41:39 PDT
Created attachment 605551 [details] [diff] [review]
Part1: JS + DB

JS Part
Comment 12 Gregor Wagner [:gwagner] 2012-03-14 14:04:03 PDT
Created attachment 605941 [details] [diff] [review]
WiP Part2: Events

Adding events.
Comment 13 [:fabrice] Fabrice Desré 2012-03-16 11:45:36 PDT
Comment on attachment 605551 [details] [diff] [review]
Part1: JS + DB

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

r=me with nits addressed.

According to an IRC discussion we rely on indexedDB to be e10s compliant. I'm not sure how we can test that be that would be nice to verify.

::: dom/interfaces/settings/Makefile.in
@@ +10,5 @@
> +include $(DEPTH)/config/autoconf.mk
> +
> +MODULE         = dom
> +XPIDL_MODULE   = dom_settings
> +GRE_MODULE     = 1

useless I think since we're not linking into libxul

::: dom/settings/SettingsManager.js
@@ +115,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +XPCOMUtils.defineLazyGetter(Services, "rs", function() {
> +  return Cc["@mozilla.org/dom/dom-request-service;1"].getService(Ci.nsIDOMRequestService);
> +});

Use Services.DOMRequest instead

@@ +143,5 @@
> +      debug("info:" + info.intent);
> +      let request = info.request;
> +      switch (info.intent) {
> +        case "clear":
> +          var req = store.clear();

let req =

@@ +153,5 @@
> +        case "set":
> +          for (var key in info.settings) {
> +            debug("key: " + key + ", val: " + JSON.stringify(info.settings[key]) + "type: " + typeof(info.settings[key]));
> +
> +            var req;

let req

@@ +158,5 @@
> +            if(typeof(info.settings[key]) != 'object') {
> +              req = store.put({settingName: key, settingValue: info.settings[key]});
> +            } else {
> +              //Workaround for cloning issues
> +              var obj = JSON.parse(JSON.stringify(info.settings[key]));

let obj

and a couple more below

@@ +322,5 @@
> +          Services.perms.add(Services.io.newURI(aHost, null, null), "websettings-readwrite",
> +                             Ci.nsIPermissionManager.ALLOW_ACTION);
> +      });
> +    } catch(e) { debug(e); }
> +

So, we constantly add whitelist entries in the permission manager. This means that if the pref changes, we never remove the old ones from the pref.

::: dom/settings/SettingsManager.manifest
@@ +2,5 @@
> +contract @mozilla.org/settingsManager;1 {5609d0a0-52a9-11e1-b86c-0800200c9a66}
> +category JavaScript-navigator-property mozSettings @mozilla.org/settingsManager;1
> +
> +component {ef95ddd0-6308-11e1-b86c-0800200c9a66} SettingsManager.js
> +contract @mozilla.org/settingsLock;1 {ef95ddd0-6308-11e1-b86c-0800200c9a66}
\ No newline at end of file

since the lock object is only instanciated by the settings manager, you don't have to expose it there, unless it makes sense for other code to create locks.

::: layout/build/Makefile.in
@@ +99,5 @@
>  	$(DEPTH)/dom/sms/src/$(LIB_PREFIX)dom_sms_s.$(LIB_SUFFIX) \
>  	$(DEPTH)/dom/src/events/$(LIB_PREFIX)jsdomevents_s.$(LIB_SUFFIX) \
>  	$(DEPTH)/dom/src/json/$(LIB_PREFIX)json_s.$(LIB_SUFFIX) \
>  	$(DEPTH)/dom/src/jsurl/$(LIB_PREFIX)jsurl_s.$(LIB_SUFFIX) \
> +	$(DEPTH)/dom/settings/$(LIB_PREFIX)dom_settings_s.$(LIB_SUFFIX) \

do we need this?
Comment 14 Gregor Wagner [:gwagner] 2012-03-23 15:46:38 PDT
Created attachment 608893 [details] [diff] [review]
AllInOne
Comment 15 Gregor Wagner [:gwagner] 2012-03-23 16:25:25 PDT
Created attachment 608908 [details] [diff] [review]
Events
Comment 16 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-03-26 09:49:07 PDT
Comment on attachment 608908 [details] [diff] [review]
Events

>+++ b/content/events/src/nsEventDispatcher.cpp	Fri Mar 23 16:23:59 2012 -0700
>@@ -909,16 +909,18 @@ nsEventDispatcher::CreateEvent(nsPresCon
>       nsDOMTouchEvent::PrefEnabled())
>     return NS_NewDOMTouchEvent(aDOMEvent, aPresContext, nsnull);
>   if (aEventType.LowerCaseEqualsLiteral("hashchangeevent"))
>     return NS_NewDOMHashChangeEvent(aDOMEvent, aPresContext, nsnull);
>   if (aEventType.LowerCaseEqualsLiteral("customevent"))
>     return NS_NewDOMCustomEvent(aDOMEvent, aPresContext, nsnull);
>   if (aEventType.LowerCaseEqualsLiteral("mozsmsevent"))
>     return NS_NewDOMSmsEvent(aDOMEvent, aPresContext, nsnull);
>+  if (aEventType.LowerCaseEqualsLiteral("settingsevent"))
>+    return NS_NewDOMSettingsEvent(aDOMEvent, aPresContext, nsnull);
Do we need this? Or is the support for ctor enough for JS?


> [scriptable, uuid(5609d0a0-52a9-11e1-b86c-0800200c9a66)]
> interface nsIDOMSettingsManager : nsISupports
> {
>   nsIDOMSettingsLock getLock();
> 
>-  //attribute nsIDOMEventListener onchange;
>+  attribute nsIDOMEventListener onchange;
Could we call this onsettingchange and the event settingchange.

>+// [Constructor(DOMString type, optional SettingsEventInit settingsEventInitDict)]
> [scriptable, uuid(5ce02690-52a9-11e1-b86c-0800200c9a66)]
> interface nsIDOMSettingsEvent : nsIDOMEvent
could you Moz prefix this.
nsIDOMMozSettingsEvent




> {
>   readonly attribute DOMString  settingName;
>   readonly attribute nsIVariant settingValue;
>+
>+  void initSettingsEvent(in DOMString aType,
>+                         in boolean aCanBubble,
>+                         in boolean aCancelable,
>+                         in DOMString aSettingName,
>+                         in nsIVariant aSettingValue);
Could you make this noscript. It would be great to not expose init*Event of new events to the
web. Support for ctors should be enough.


> };
> 
>-[scriptable, uuid(635c0070-52a9-11e1-b86c-0800200c9a66)]
>-interface nsIDOMSettingsEventInit : nsIEventInit {
>-  attribute DOMString  settingName;
>-  attribute nsIVariant settingValue;
>-};*/
>+dictionary SettingsEventInit : EventInit {
{ should be in the next line.
Comment 17 Andreas Gal :gal 2012-03-26 09:52:13 PDT
Gregor, can you fix this up right away? We are blocked on this patch. Thanks!
Comment 18 Gregor Wagner [:gwagner] 2012-03-26 12:57:11 PDT
Created attachment 609442 [details] [diff] [review]
Events
Comment 19 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-03-26 13:30:53 PDT
Comment on attachment 609442 [details] [diff] [review]
Events


>+NS_NewDOMMozSettingsEvent(nsIDOMEvent** aInstancePtrResult,
>+                          nsPresContext* aPresContext,
>+                          nsEvent* aEvent) 
>+{
>+  nsDOMMozSettingsEvent* e = new nsDOMMozSettingsEvent(aPresContext, aEvent);
>+  if (nsnull == e) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }
I know you copied this from elsewhere, but new is infallible so no need for null check.

>+
>+class nsDOMMozSettingsEvent : public nsDOMEvent,
>+                           public nsIDOMMozSettingsEvent
Align public nsIDOMMozSettingsEvent properly under public nsDOMEvent


r+ for the event stuff.
Comment 20 Gregor Wagner [:gwagner] 2012-03-26 13:39:01 PDT
Created attachment 609462 [details] [diff] [review]
JS nits
Comment 21 Gregor Wagner [:gwagner] 2012-03-26 13:58:34 PDT
Created attachment 609473 [details] [diff] [review]
AllInOne

combined patch
Comment 22 Andreas Gal :gal 2012-03-26 14:40:43 PDT
Missing space "//WebSettings"
Comment 23 Gregor Wagner [:gwagner] 2012-03-26 14:46:50 PDT
(In reply to Andreas Gal :gal from comment #22)
> Missing space "//WebSettings"

Thx! That got lost in a merge.
Comment 25 Ed Morley [:emorley] 2012-03-27 05:24:22 PDT
https://hg.mozilla.org/mozilla-central/rev/b0d644ecd8a7
Comment 27 Bobby Holley (busy) 2012-06-10 14:46:18 PDT
Gregor, were the tests here ever reviewed? They added a a call to enablePrivilege, which is forbidden going forward. Can you file a followup bug and take care of fixing the test?
Comment 28 Gregor Wagner [:gwagner] 2012-06-11 13:27:41 PDT
(In reply to Bobby Holley (:bholley) from comment #27)
> Gregor, were the tests here ever reviewed? They added a a call to
> enablePrivilege, which is forbidden going forward. Can you file a followup
> bug and take care of fixing the test?

Fabrice reviewed the tests. How does the new way look like?
Comment 29 Bobby Holley (busy) 2012-06-12 03:45:56 PDT
(In reply to Gregor Wagner [:gwagner] from comment #28)
> Fabrice reviewed the tests. How does the new way look like?

There are a few possibilities:
1 - Chrome test with content iframe, doing the privileged stuff in chrome.
2 - Add an API to SpecialPowers to do what you need to do.
3 - Use the transitive SpecialPowers.wrap() if the use-case is simple enough.

3 will probably work here, but if enabling webAPI permissions is a common thing we'll need to do in tests, it might be worth adding an explicit API to SpecialPowers (testing/mochitest/tests/SimpleTest/specialPowersAPI.js).
Comment 30 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-06-12 08:01:21 PDT
I already added permission manager stuff to SpecialPowers. If that is all you need then this should be trivial.

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/specialpowersAPI.js#1098
Comment 31 Jeremie Patonnier :Jeremie 2013-02-06 02:10:24 PST
Documentation done:

* https://developer.mozilla.org/en-US/docs/DOM/SettingsLock
* https://developer.mozilla.org/en-US/docs/DOM/SettingsManager

It required a technical review.

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