1206245 - Implement API for whether there are "dot" state permissions

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Comment 1

[:Paolo Amadini]

This might also be developed as part of bug 1206246. We thought we'd expose a function on SitePermissions.jsm or a new TabPermissions.jsm module to determine if any permission was customized for the site, but if the "dot" state of the icon matches the case where the permissions section of the Control Center is not empty, we might as well implement the logic in the front-end.

Comment 2

[:Paolo Amadini]

According to bug 1206246 comment 4, we need to show the dot only when we have one or more explicitly granted permissions for the top level URL. So we need some extra logic here.

If I remember correctly the discussions that led to this and other similar API bugs to be filed, we'd like to see a more structured approach to SitePermissions.jsm where the data model is more similar to the front-end model needed by the control center. One step in that direction could be to create a stateFromUri(uri) function that returns an object like this:

{
  hasAnyGrantedPermission: true,
  customizedPermissions: [
    {
      id: "...",
      label: "...",
      stateLabel: "...",
    },
    {
      id: "...",
      label: "...",
      stateLabel: "...",
      blocked: true,
    },
  ],
}

And customizedPermissions would be an ordered list. This way, we could have simple non-ui regression tests to check that the model works, and we can keep the front-end code simpler, just dealing with rendering the controls and handling commands.

In this bug we may start with the implementation and test of a stateFromUri(uri) function that only returns { hasAnyGrantedPermission: true/false }.

Comment 3

[Johann Hofmann [:johannh]]

Yes it's probably a good idea to do this in SitePermissions so that the UI doesn't have to worry about that logic.

I was thinking of splitting your suggestion into two different functions "hasCustomPermissions" and "getCustomPermissionsByURL" or something.

Comment 4

[Johann Hofmann [:johannh]]

> And customizedPermissions would be an ordered list.

Ordered by what? :)

Comment 5

[Johann Hofmann [:johannh]]

Attachment: MozReview Request: Bug 1206245 - Add a function to SitePermissions.jsm to check granted permissions by uri. r?paolo

Review commit: https://reviewboard.mozilla.org/r/56892/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56892/

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8758670 [details]
MozReview Request: Bug 1206245 - Add a function to SitePermissions.jsm to check granted permissions by uri. r?paolo

This is a rough version of the simpler functionality, that checks whether the site has any permissions at all. Paolo, what do you think of it?

Comment 7

[:Paolo Amadini]

Comment on attachment 8758670 [details]
MozReview Request: Bug 1206245 - Add a function to SitePermissions.jsm to check granted permissions by uri. r?paolo

(In reply to Johann Hofmann [:johannh] from comment #3)
> I was thinking of splitting your suggestion into two different functions
> "hasCustomPermissions" and "getCustomPermissionsByURL" or something.

Sounds good to me, we can always refactor later.

The function we use in the front-end for the "dot" state should return false if there are only blocked permissions, so hasCustomPermissions is not exactly what we need here.

Comment 8

[Johann Hofmann [:johannh]]

(In reply to :Paolo Amadini from comment #7)
> Sounds good to me, we can always refactor later.
> 
> The function we use in the front-end for the "dot" state should return false
> if there are only blocked permissions, so hasCustomPermissions is not
> exactly what we need here.

Ah, I see! I'll add a function "hasGrantedPermissions". Should we still keep "hasCustomPermissions" even though it's not used?

Comment 9

[:Paolo Amadini]

(In reply to Johann Hofmann [:johannh] from comment #8)
> Should we still keep "hasCustomPermissions" even though it's not used?

No, it's easy to add back when needed.

Comment 10

[Johann Hofmann [:johannh]]

Comment on attachment 8758670 [details]
MozReview Request: Bug 1206245 - Add a function to SitePermissions.jsm to check granted permissions by uri. r?paolo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56892/diff/1-2/

Comment 11

[:Paolo Amadini]

Comment on attachment 8758670 [details]
MozReview Request: Bug 1206245 - Add a function to SitePermissions.jsm to check granted permissions by uri. r?paolo

https://reviewboard.mozilla.org/r/56892/#review53574

::: browser/modules/SitePermissions.jsm:19
(Diff revision 2)
> +  hasGrantedPermissions: function (aURI) {
> +    if (!this.isSupportedURI(aURI)) {

Needs jsdoc!

::: browser/modules/test/xpcshell/test_SitePermissions.js:33
(Diff revision 2)
> +  Assert.equal(SitePermissions.hasGrantedPermissions(uri), true);
> +
> +  SitePermissions.remove(uri, "geo");
> +  Assert.equal(SitePermissions.hasGrantedPermissions(uri), false);
> +
> +  SitePermissions.set(uri, "pointerLock", SitePermissions.BLOCK);

This permission should be already set to the same value at this point.

It can be more interesting to use "set" to change an existing permission from allow to block and the other way around, however this would start to be about testing the "set" function rather than the combinations. It seems to me we can just remove every test from this onwards.

We need tests for combinations including the SESSION case, since you're handling it in the code.

One-line comments explaining which case you're testing and why may help readability.

Comment 12

[Johann Hofmann [:johannh]]

Comment on attachment 8758670 [details]
MozReview Request: Bug 1206245 - Add a function to SitePermissions.jsm to check granted permissions by uri. r?paolo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56892/diff/2-3/

Comment 13

[Johann Hofmann [:johannh]]

Good points, thanks :)

> this would start to be about testing the "set" function rather than the combinations. It seems to me we can just remove every test from this onwards.

I agree!

Comment 14

[:Paolo Amadini]

Comment on attachment 8758670 [details]
MozReview Request: Bug 1206245 - Add a function to SitePermissions.jsm to check granted permissions by uri. r?paolo

https://reviewboard.mozilla.org/r/56892/#review53600

Thanks!

Comment 15

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01edc1295562

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/359674b9970a
Add a function to SitePermissions.jsm to check granted permissions by uri. r=paolo

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/359674b9970a