Move Settings API to WebIDL

RESOLVED FIXED in mozilla25

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: reuben, Assigned: reuben)

Tracking

Trunk
mozilla25
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 770325 [details] [diff] [review]
Patch
Attachment #770325 - Flags: review?(dzbarsky)
(Assignee)

Comment 2

5 years ago
Created attachment 770336 [details] [diff] [review]
Gaia patch
Attachment #770336 - Flags: review?(anygregor)
Comment on attachment 770325 [details] [diff] [review]
Patch

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

::: dom/settings/SettingsManager.js
@@ -262,5 @@
>  };
>  
> -const SETTINGSMANAGER_CONTRACTID = "@mozilla.org/settingsManager;1";
> -const SETTINGSMANAGER_CID        = Components.ID("{c40b1c70-00fb-11e2-a21f-0800200c9a66}");
> -const nsIDOMSettingsManager      = Ci.nsIDOMSettingsManager;

It may be worth keeping these around.
Attachment #770325 - Flags: review?(dzbarsky)
Attachment #770325 - Flags: review?(bzbarsky)
Attachment #770325 - Flags: feedback+
Attachment #770336 - Flags: review?(anygregor) → review+
(Assignee)

Comment 4

5 years ago
Created attachment 772792 [details] [diff] [review]
Patch, v2
Attachment #770325 - Attachment is obsolete: true
Attachment #770325 - Flags: review?(bzbarsky)
Attachment #772792 - Flags: review?(bzbarsky)
Attachment #772792 - Flags: review?(anygregor)
Comment on attachment 772792 [details] [diff] [review]
Patch, v2

r=me
Attachment #772792 - Flags: review?(bzbarsky) → review+
Comment on attachment 772792 [details] [diff] [review]
Patch, v2

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

::: dom/settings/SettingsManager.js
@@ +361,5 @@
>      this.hasWritePrivileges = writePerm == Ci.nsIPermissionManager.ALLOW_ACTION;
>  
> +    if (this.hasReadPrivileges) {
> +      cpmm.sendAsyncMessage("Settings:RegisterForMessages");
> +    }

Huh? This defeats the purpose of the whole register message approach. We don't want to get messages if we don't have a listener. This means we have to wake up all apps when a setting has changed. Not good for mobile!
Attachment #772792 - Flags: review?(anygregor)
(Assignee)

Comment 7

5 years ago
Created attachment 776008 [details] [diff] [review]
Additional bits outside of dom/settings
Attachment #776008 - Flags: review?(anygregor)
Comment on attachment 772792 [details] [diff] [review]
Patch, v2

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

Lets hope the navigator.language issue is getting fixed soon.
Attachment #772792 - Flags: review+
Attachment #776008 - Flags: review?(anygregor) → review+
https://hg.mozilla.org/mozilla-central/rev/70f4d118d159
https://hg.mozilla.org/mozilla-central/rev/6c3396af4b17
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
https://hg.mozilla.org/integration/mozilla-inbound/rev/70f4d118d159 broke many gaia-unit tests, see bug 894964.  These tests need to be fixed (or this patch fixed, or backed out) so that we can unhide gaia-unit-tests in TBPL.
Blocks: 894964
(Assignee)

Updated

5 years ago
Blocks: 898512
You need to log in before you can comment on or make changes to this bug.