Closed Bug 905397 Opened 6 years ago Closed 6 years ago

Handle installed permissions

Categories

(Firefox Graveyard :: Webapp Runtime, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: marco, Assigned: marco)

References

Details

(Whiteboard: DesktopWebRT2)

Attachments

(1 file, 2 obsolete files)

Attached patch prompt_unknown_permissions (obsolete) — Splinter Review
Needed after bug 892837 to correctly handle installed permissions.
Attachment #790457 - Flags: review?(myk)
Attachment #790457 - Attachment is obsolete: true
Attachment #790457 - Flags: review?(myk)
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
Attachment #790497 - Flags: review?(myk)
Marco: if this blocks bug 892837, then we can't fix that bug until we fix this one.  But that bug is currently checkin-needed, while this one isn't ready for commission.  Are they really dependent?  Or perhaps the dependency relationship goes the other way?
(In reply to Myk Melez [:myk] [@mykmelez] from comment #2)
> Marco: if this blocks bug 892837, then we can't fix that bug until we fix
> this one.  But that bug is currently checkin-needed, while this one isn't
> ready for commission.  Are they really dependent?  Or perhaps the dependency
> relationship goes the other way?

Yes we can land them separately. This patch completes the permissions support, this is why it's blocking that bug, but actually the patches are "parallel" :)
Priority: -- → P1
Whiteboard: DesktopWebRT2
Comment on attachment 790497 [details] [diff] [review]
Patch

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

::: webapprt/ContentPermission.js
@@ +26,5 @@
> +      .rootTreeItem
> +      .QueryInterface(Ci.nsIInterfaceRequestor)
> +      .getInterface(Ci.nsIDOMWindow)
> +      .QueryInterface(Ci.nsIDOMChromeWindow);
> +    return chromeWin;

Nit: var -> let, but since all you do is return it, you might as well simply return the value instead of assigning it to a variable first.

@@ +33,4 @@
>    prompt: function(request) {
> +    // Reuse any remembered permission preferences
> +    let result = Services.perms.
> +                   testExactPermissionFromPrincipal(request.principal, request.type);

Nit: this line exceeds 80 columns and should be wrapped differently, f.e.:

    let result =
      Services.perms.testExactPermissionFromPrincipal(request.principal,
                                                      request.type);

@@ +91,5 @@
> +      // Persist the choice if the user wants to remember
> +      Services.perms.addFromPrincipal(request.principal, request.type, action);
> +    } else {
> +      // Otherwise allow the permission for the current session
> +      Services.perms.addFromPrincipal(request.principal, request.type, action, Ci.nsIPermissionManager.EXPIRE_SESSION);

Nit: this line exceeds 80 columns and should be wrapped, f.e.:

      Services.perms.addFromPrincipal(request.principal, request.type, action,
                                      Ci.nsIPermissionManager.EXPIRE_SESSION);

::: webapprt/locales/en-US/webapprt/webapp.properties
@@ +25,4 @@
>  geolocation.remember=Remember my choice
>  
> +# LOCALIZATION NOTE (desktop-notification.title): %S will be replaced with the name of
> +# the webapp.

Nit: the first of these two lines exceeds 80 columns, so these lines should be rewrapped.
Attachment #790497 - Flags: review?(myk) → review+
Attached patch PatchSplinter Review
Carrying r+.
Attachment #790497 - Attachment is obsolete: true
Attachment #792481 - Flags: review+
This landed as 892837. Please double-check your commit messages in the future. Backed out and re-landed.
https://hg.mozilla.org/integration/fx-team/rev/a1ba99bda320
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a1ba99bda320
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.