We need an API to configure device settings.

IDL files

Can set and get values.

+ tests

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 on attachment 595563 [details] [diff] [review] IDL We should do the new lock-based API instead...

IDL Part

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.

JS implementation

JS Part

Adding events.

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

Gregor, can you fix this up right away? We are blocked on this patch. Thanks!

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.

combined patch

Missing space "//WebSettings"

(In reply to Andreas Gal :gal from comment #22) > Missing space "//WebSettings" Thx! That got lost in a merge.

Review Scheduled: https://mail.mozilla.com/home/ckoenig@mozilla.com/Security%20Review.html?view=month&action=view&invId=181869-181868&pstat=AC&instStartTime=1338404400000&instDuration=3600000

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?

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

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

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

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.