Closed Bug 889503 Opened 6 years ago Closed 6 years ago

Move Settings API to WebIDL

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: reuben, Assigned: reuben)

References

Details

Attachments

(3 files, 1 obsolete file)

No description provided.
Attached patch Patch (obsolete) — Splinter Review
Attachment #770325 - Flags: review?(dzbarsky)
Attached patch Gaia patchSplinter Review
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+
Attached patch Patch, v2Splinter Review
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)
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
Closed: 6 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
Blocks: 898512
You need to log in before you can comment on or make changes to this bug.