Closed Bug 776668 Opened 8 years ago Closed 8 years ago

Split permission for Settings/Contacts access into ReadOnly and ReadWrite variants

Categories

(Core :: DOM: Device Interfaces, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: cjones, Assigned: gwagner)

References

Details

(Whiteboard: [LOE:S])

Attachments

(1 file)

No description provided.
blocking-basecamp: --- → +
Chris - Can you give more context on what this bug is and help us find an assignee?
Assignee: nobody → anygregor
Whiteboard: [WebAPI:P2]
Keywords: feature
Whiteboard: [WebAPI:P2] → [WebAPI:P2][LOE:S]
This may seem artificial to remove it at this point but Gregor told me that he and Jonas decided this and bug 776671 aren't features so I'm removing the feature keyword.
Keywords: feature
Priority: -- → P2
Whiteboard: [WebAPI:P2][LOE:S] → [LOE:S]
Can we morph this to cover read vs. write privileges for the settings API?
(In reply to ben turner [:bent] from comment #3)
> Can we morph this to cover read vs. write privileges for the settings API?

Yes we can!
Summary: Check capabilities for Settings access → Split permission for Settings access into ReadOnly and ReadWrite variants
Depends on: 805646
Attached patch patchSplinter Review
For settings and contacts
Duplicate of this bug: 776671
Summary: Split permission for Settings access into ReadOnly and ReadWrite variants → Split permission for Settings/Contacts access into ReadOnly and ReadWrite variants
Attachment #675411 - Flags: review?(ddahl)
Comment on attachment 675411 [details] [diff] [review]
patch

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

Looks good,  only a few nits.

::: dom/contacts/ContactManager.js
@@ +446,5 @@
> +        break
> +      case "update":
> +      case "remove":
> +        access = "write";
> +        break

nit: break;

::: dom/contacts/fallback/ContactService.jsm
@@ +68,5 @@
>    },
>  
> +  assertPermission: function(aMessage, aPerm) {
> +    if (!aMessage.target.assertPermission(aPerm)) {
> +      dump("Contacts message " + msg.name +

debug() instead of dump()?

::: dom/settings/SettingsChangeNotifier.jsm
@@ +80,5 @@
>      let mm = aMessage.target;
>      switch (aMessage.name) {
>        case "Settings:Changed":
> +        if (!aMessage.target.assertPermission("settings-write")) {
> +          dump("Settings message " + msg.name +

debug() instead of dump() - or should this be Cu.reportError()? - same with above?

::: dom/settings/SettingsManager.js
@@ +339,5 @@
> +    this.hasReadPrivileges = readPerm == Ci.nsIPermissionManager.ALLOW_ACTION;
> +    this.hasWritePrivileges = writePerm == Ci.nsIPermissionManager.ALLOW_ACTION;
> +
> +    if (!this.hasReadPrivileges && !this.hasWritePrivileges) {
> +      dump("NO SETTINGS PERMISSION FOR: " + aWindow.document.nodePrincipal.origin + "\n");

I think if we want these messages to bubble up when developers are wokring on apps, etc, we want to use Cu.reportError instead of dump
Attachment #675411 - Flags: review?(ddahl) → review+
https://hg.mozilla.org/mozilla-central/rev/858e49a77dca
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.