Closed Bug 1206245 Opened 4 years ago Closed 4 years ago

Implement API for whether there are "dot" state permissions

Categories

(Firefox :: General, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 49
Iteration:
49.3 - Jun 6
Tracking Status
firefox49 --- fixed

People

(Reporter: MarcoM, Assigned: johannh)

References

Details

(Whiteboard: [fxprivacy])

Attachments

(1 file)

No description provided.
Flags: qe-verify?
Priority: P3 → P2
Priority: P2 → P3
Priority: P3 → P1
Flags: qe-verify? → qe-verify-
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.
Assignee: nobody → jhofmann
Iteration: --- → 49.3 - Jun 6
Status: NEW → ASSIGNED
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 }.
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.
> And customizedPermissions would be an ordered list.

Ordered by what? :)
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?
Attachment #8758670 - Flags: feedback?(paolo.mozmail)
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.
Attachment #8758670 - Flags: feedback?(paolo.mozmail)
(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?
(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 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/
Attachment #8758670 - Attachment description: MozReview Request: Bug 1206245 - Add a function to SitePermissions.jsm to check permissions by uri → MozReview Request: Bug 1206245 - Add a function to SitePermissions.jsm to check granted permissions by uri. r?paolo
Attachment #8758670 - Flags: review?(paolo.mozmail)
Attachment #8758670 - Flags: review?(paolo.mozmail)
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 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/
Attachment #8758670 - Flags: review?(paolo.mozmail)
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 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!
Attachment #8758670 - Flags: review?(paolo.mozmail) → review+
See Also: → 1277289
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/359674b9970a
Add a function to SitePermissions.jsm to check granted permissions by uri. r=paolo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/359674b9970a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
No longer blocks: 1206246
Blocks: 1206246
You need to log in before you can comment on or make changes to this bug.