905397 - Handle installed permissions

Status: RESOLVED FIXED

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

Description

[Marco Castelluccio [:marco] (PTO until August 30)]

Attachment: prompt_unknown_permissions

Needed after bug 892837 to correctly handle installed permissions.

Comment 1

[Marco Castelluccio [:marco] (PTO until August 30)]

Attachment: Patch

Comment 2

[Myk Melez [:myk] [@mykmelez]]

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?

Comment 3

[Marco Castelluccio [:marco] (PTO until August 30)]

(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" :)

Comment 4

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 5

[Marco Castelluccio [:marco] (PTO until August 30)]

Attachment: Patch

Carrying r+.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a1ba99bda320