The default bug view has changed. See this FAQ.

Status

()

Core
General
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: gal, Assigned: gwagner)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

Trunk
mozilla14
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments, 9 obsolete attachments)

(Reporter)

Description

6 years ago
We need an API to configure device settings.
(Reporter)

Updated

6 years ago
Blocks: 673923
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Blocks: 712778
Blocks: 714357
Blocks: 715782
(Assignee)

Updated

5 years ago
Assignee: nobody → anygregor
(Assignee)

Comment 1

5 years ago
Created attachment 595563 [details] [diff] [review]
IDL

IDL files
Attachment #595563 - Flags: feedback?(jonas)
(Assignee)

Updated

5 years ago
Depends on: 722626
(Assignee)

Comment 2

5 years ago
Created attachment 595878 [details] [diff] [review]
WiP: Basic functionality

Can set and get values.
(Assignee)

Comment 3

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

+ tests
Attachment #595878 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
Created attachment 596761 [details] [diff] [review]
JS part
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

5 years ago
Created attachment 601682 [details] [diff] [review]
IDL

IDL Part
Attachment #595563 - Attachment is obsolete: true
Attachment #601682 - Flags: review?(jonas)
Keywords: dev-doc-needed
(Assignee)

Comment 8

5 years ago
Created attachment 602538 [details] [diff] [review]
WiP: Basic functionality
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)

Updated

5 years ago
(Assignee)

Comment 10

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

JS implementation
Attachment #602538 - Attachment is obsolete: true
Attachment #603919 - Flags: feedback?(jonas)
(Assignee)

Comment 11

5 years ago
Created attachment 605551 [details] [diff] [review]
Part1: JS + DB

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

5 years ago
Created attachment 605941 [details] [diff] [review]
WiP Part2: Events

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

5 years ago
Created attachment 608893 [details] [diff] [review]
AllInOne
Attachment #608893 - Flags: feedback?(jonas)
(Assignee)

Comment 15

5 years ago
Created attachment 608908 [details] [diff] [review]
Events
Attachment #605941 - Attachment is obsolete: true
Attachment #608908 - Flags: review?(mrbkap)
(Assignee)

Updated

5 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

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

Comment 18

5 years ago
Created attachment 609442 [details] [diff] [review]
Events
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

5 years ago
Created attachment 609462 [details] [diff] [review]
JS nits
Attachment #609462 - Flags: review?(fabrice)
Attachment #609462 - Flags: review?(fabrice) → review+
Attachment #605551 - Flags: review?(fabrice) → review+
(Assignee)

Comment 21

5 years ago
Created attachment 609473 [details] [diff] [review]
AllInOne

combined patch
Attachment #608893 - Attachment is obsolete: true
Attachment #608893 - Flags: feedback?(jonas)
(Reporter)

Comment 22

5 years ago
Missing space "//WebSettings"
(Assignee)

Comment 23

5 years ago
(In reply to Andreas Gal :gal from comment #22)
> Missing space "//WebSettings"

Thx! That got lost in a merge.
(Assignee)

Comment 24

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0d644ecd8a7
https://hg.mozilla.org/mozilla-central/rev/b0d644ecd8a7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Depends on: 739680
Blocks: 740337
(Assignee)

Updated

5 years ago
Depends on: 742065
Depends on: 743018
Depends on: 750992
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
Depends on: 758486
(Assignee)

Updated

5 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

5 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

5 years ago
Depends on: 764003
(Assignee)

Updated

5 years ago
Depends on: 772127
Attachment #605551 - Flags: feedback?(jonas)
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.