Closed Bug 678695 Opened 9 years ago Closed 8 years ago

Settings API

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: gal, Assigned: gwagner)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 9 obsolete files)

We need an API to configure device settings.
Blocks: webapi
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Assignee: nobody → anygregor
Attached patch IDL (obsolete) — Splinter Review
IDL files
Attachment #595563 - Flags: feedback?(jonas)
Depends on: 722626
Attached patch WiP: Basic functionality (obsolete) — Splinter Review
Can set and get values.
Attached patch WiP (obsolete) — Splinter Review
+ tests
Attachment #595878 - Attachment is obsolete: true
Attached patch JS part (obsolete) — Splinter Review
Attachment #595899 - Attachment is obsolete: true
Attachment #596761 - Flags: review?(fabrice)
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...
Attachment #595563 - Flags: feedback?(jonas)
Attached patch IDLSplinter Review
IDL Part
Attachment #595563 - Attachment is obsolete: true
Attachment #601682 - Flags: review?(jonas)
Attached patch WiP: Basic functionality (obsolete) — Splinter Review
Attachment #596761 - Attachment is obsolete: true
Attachment #596761 - Flags: review?(fabrice)
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.
Attachment #601682 - Flags: review?(jonas) → review+
Attached patch WiP (obsolete) — Splinter Review
JS implementation
Attachment #602538 - Attachment is obsolete: true
Attachment #603919 - Flags: feedback?(jonas)
Attached patch Part1: JS + DBSplinter Review
JS Part
Attachment #603919 - Attachment is obsolete: true
Attachment #603919 - Flags: feedback?(jonas)
Attachment #605551 - Flags: review?(fabrice)
Attachment #605551 - Flags: feedback?(jonas)
Attached patch WiP Part2: Events (obsolete) — Splinter Review
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?
Attached patch AllInOne (obsolete) — Splinter Review
Attachment #608893 - Flags: feedback?(jonas)
Attached patch Events (obsolete) — Splinter Review
Attachment #605941 - Attachment is obsolete: true
Attachment #608908 - Flags: review?(mrbkap)
Attachment #608908 - Flags: review?(bugs)
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.
Attachment #608908 - Flags: review?(bugs) → review-
Gregor, can you fix this up right away? We are blocked on this patch. Thanks!
Attached patch EventsSplinter Review
Attachment #608908 - Attachment is obsolete: true
Attachment #608908 - Flags: review?(mrbkap)
Attachment #609442 - Flags: review?(bugs)
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.
Attachment #609442 - Flags: review?(bugs) → review+
Attached patch JS nitsSplinter Review
Attachment #609462 - Flags: review?(fabrice)
Attachment #609462 - Flags: review?(fabrice) → review+
Attachment #605551 - Flags: review?(fabrice) → review+
Attached patch AllInOneSplinter Review
combined patch
Attachment #608893 - Attachment is obsolete: true
Attachment #608893 - Flags: feedback?(jonas)
Missing space "//WebSettings"
(In reply to Andreas Gal :gal from comment #22)
> Missing space "//WebSettings"

Thx! That got lost in a merge.
https://hg.mozilla.org/mozilla-central/rev/b0d644ecd8a7
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Depends on: 742065
Depends on: 754142
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
Depends on: 764003
Depends on: 772127
You need to log in before you can comment on or make changes to this bug.