Status

()

defect
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: gal, Assigned: gwagner)

Tracking

({dev-doc-complete})

Trunk
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(5 attachments, 9 obsolete attachments)

Reporter

Description

8 years ago
We need an API to configure device settings.
Reporter

Updated

8 years ago
Blocks: webapi
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Assignee

Updated

7 years ago
Assignee: nobody → anygregor
Assignee

Comment 1

7 years ago
Posted patch IDL (obsolete) — Splinter Review
IDL files
Attachment #595563 - Flags: feedback?(jonas)
Assignee

Updated

7 years ago
Depends on: 722626
Assignee

Comment 2

7 years ago
Posted patch WiP: Basic functionality (obsolete) — Splinter Review
Can set and get values.
Assignee

Comment 3

7 years ago
Posted patch WiP (obsolete) — Splinter Review
+ tests
Attachment #595878 - Attachment is obsolete: true
Assignee

Comment 4

7 years ago
Posted 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)
Assignee

Comment 7

7 years ago
Posted patch IDLSplinter Review
IDL Part
Attachment #595563 - Attachment is obsolete: true
Attachment #601682 - Flags: review?(jonas)
Assignee

Comment 8

7 years ago
Posted 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+
Assignee

Comment 10

7 years ago
Posted patch WiP (obsolete) — Splinter Review
JS implementation
Attachment #602538 - Attachment is obsolete: true
Attachment #603919 - Flags: feedback?(jonas)
Assignee

Comment 11

7 years ago
JS Part
Attachment #603919 - Attachment is obsolete: true
Attachment #603919 - Flags: feedback?(jonas)
Attachment #605551 - Flags: review?(fabrice)
Attachment #605551 - Flags: feedback?(jonas)
Assignee

Comment 12

7 years ago
Posted 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?
Assignee

Comment 14

7 years ago
Posted patch AllInOne (obsolete) — Splinter Review
Attachment #608893 - Flags: feedback?(jonas)
Assignee

Comment 15

7 years ago
Posted patch Events (obsolete) — Splinter Review
Attachment #605941 - Attachment is obsolete: true
Attachment #608908 - Flags: review?(mrbkap)
Assignee

Updated

7 years ago
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-
Reporter

Comment 17

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

Comment 18

7 years ago
Posted 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+
Assignee

Comment 20

7 years ago
Posted patch JS nitsSplinter Review
Attachment #609462 - Flags: review?(fabrice)
Attachment #609462 - Flags: review?(fabrice) → review+
Attachment #605551 - Flags: review?(fabrice) → review+
Assignee

Comment 21

7 years ago
Posted patch AllInOneSplinter Review
combined patch
Attachment #608893 - Attachment is obsolete: true
Attachment #608893 - Flags: feedback?(jonas)
Reporter

Comment 22

7 years ago
Missing space "//WebSettings"
Assignee

Comment 23

7 years ago
(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
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Assignee

Updated

7 years ago
Depends on: 742065
Depends on: 750992
Assignee

Updated

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

Comment 28

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

Updated

7 years ago
Depends on: 764003
Assignee

Updated

7 years ago
Depends on: 772127
You need to log in before you can comment on or make changes to this bug.