Implement Permissions.revoke

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: poiru, Assigned: marco, Mentored)

Tracking

({dev-doc-complete})

unspecified
mozilla47
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [lang=c++], )

Attachments

(3 attachments, 3 obsolete attachments)

This is now part of the spec. We should implement it.
Reporter

Updated

3 years ago
Assignee: birunthan → nobody
Mentor: birunthan
Status: ASSIGNED → NEW
Whiteboard: [lang=c++]
Assignee

Comment 1

3 years ago
Posted patch permissions_revoke (obsolete) — Splinter Review
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
Attachment #8716134 - Flags: review?(birunthan)
Assignee

Comment 2

3 years ago
Attachment #8716135 - Flags: review?(birunthan)
Reporter

Comment 3

3 years ago
Comment on attachment 8716134 [details] [diff] [review]
permissions_revoke

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

::: dom/permission/Permissions.cpp
@@ +156,5 @@
> +    aRv.Throw(rv);
> +    return nullptr;
> +  }
> +
> +  RefPtr<Promise> promise = Promise::Create(global, aRv);

Could you do this as early as possible and change the code above to return rejected promise instead of `nullptr`?
Attachment #8716134 - Flags: review?(birunthan)
Reporter

Comment 4

3 years ago
Comment on attachment 8716135 [details] [diff] [review]
permissions_revoke_tests

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

Thanks for doing this!
Attachment #8716135 - Flags: review?(birunthan) → review+
Assignee

Comment 5

3 years ago
(In reply to Birunthan Mohanathas [:poiru] from comment #3)
> Comment on attachment 8716134 [details] [diff] [review]
> permissions_revoke
> 
> Review of attachment 8716134 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/permission/Permissions.cpp
> @@ +156,5 @@
> > +    aRv.Throw(rv);
> > +    return nullptr;
> > +  }
> > +
> > +  RefPtr<Promise> promise = Promise::Create(global, aRv);
> 
> Could you do this as early as possible and change the code above to return
> rejected promise instead of `nullptr`?

From the spec it was unclear to me when the function should throw and when it should
reject the promise.
I'll update the patch.
Assignee

Comment 6

3 years ago
Posted patch permissions_revoke (obsolete) — Splinter Review
Attachment #8716134 - Attachment is obsolete: true
Attachment #8716895 - Flags: review?(birunthan)
Reporter

Updated

3 years ago
Attachment #8716895 - Flags: review?(birunthan) → review+
Assignee

Comment 8

3 years ago
> Changes to WebIDL files in this repo require review from a DOM peer in the form of r=...

Who else should review the patch?
Flags: needinfo?(birunthan)
Reporter

Comment 9

3 years ago
Comment on attachment 8716895 [details] [diff] [review]
permissions_revoke

baku, can you review (at least) the WebIDL change?
Flags: needinfo?(birunthan)
Attachment #8716895 - Flags: review+ → review?(amarchesini)
Attachment #8716895 - Flags: review?(amarchesini) → review+
Backed out for various test failures in 

Push with failing jobs: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d776c61906c1
E.g. Windows 8 x64 opt failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=21721604&repo=mozilla-inbound

10:18:48     INFO -  127 INFO TEST-START | dom/permission/tests/test_permissions_api.html
10:18:49     INFO -  TEST-INFO | started process screenshot
10:18:49     INFO -  TEST-INFO | screenshot: exit 0
10:18:49     INFO -  128 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | query should have thrown TypeError for 'foobarbaz'
10:18:49     INFO -  129 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | query should have thrown TypeError for 'midi'
10:18:49     INFO -  130 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | query should have rejected for userVisible push
10:18:49     INFO -  131 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'geolocation'
10:18:49     INFO -  132 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'notifications'
10:18:49     INFO -  133 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'push'
10:18:49     INFO -  134 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'geolocation'
10:18:49     INFO -  135 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'notifications'
10:18:49     INFO -  136 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'push'
10:18:49     INFO -  137 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'geolocation'
10:18:49     INFO -  138 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'notifications'
10:18:49     INFO -  139 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'push'
10:18:49     INFO -  140 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'geolocation'
10:18:49     INFO -  141 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'notifications'
10:18:49     INFO -  142 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'push'
10:18:49     INFO -  143 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | state changed for 'geolocation'
10:18:49     INFO -  144 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | state changed for 'geolocation'
10:18:49     INFO -  145 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | invalid query should have rejected
10:18:49     INFO -  146 INFO TEST-UNEXPECTED-FAIL | dom/permission/tests/test_permissions_api.html | revoke should have thrown TypeError for 'foobarbaz' - got "NS_ERROR_UNEXPECTED", expected "TypeError"
10:18:49     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:267:5
10:18:49     INFO -      revokeUnsupportedPermissions/</<@dom/permission/tests/test_permissions_api.html:57:16
10:18:49     INFO -      Promise*revokeUnsupportedPermissions/<@dom/permission/tests/test_permissions_api.html:55:5
10:18:49     INFO -      revokeUnsupportedPermissions@dom/permission/tests/test_permissions_api.html:54:22
10:18:49     INFO -      promise callback*runTests@dom/permission/tests/test_permissions_api.html:134:3
10:18:49     INFO -      onload@dom/permission/tests/test_permissions_api.html:1:1
10:18:49     INFO -  Not taking screenshot here: see the one that was previously logged
10:18:49     INFO -  147 INFO TEST-UNEXPECTED-FAIL | dom/permission/tests/test_permissions_api.html | revoke should have thrown TypeError for 'midi' - got "NS_ERROR_UNEXPECTED", expected "TypeError"
10:18:49     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:267:5
10:18:49     INFO -      revokeUnsupportedPermissions/</<@dom/permission/tests/test_permissions_api.html:57:16
10:18:49     INFO -      Promise*revokeUnsupportedPermissions/<@dom/permission/tests/test_permissions_api.html:55:5
10:18:49     INFO -      revokeUnsupportedPermissions@dom/permission/tests/test_permissions_api.html:54:22
10:18:49     INFO -      promise callback*runTests@dom/permission/tests/test_permissions_api.html:134:3
10:18:49     INFO -      onload@dom/permission/tests/test_permissions_api.html:1:1
10:18:49     INFO -  Not taking screenshot here: see the one that was previously logged
10:18:49     INFO -  148 INFO TEST-UNEXPECTED-FAIL | dom/permission/tests/test_permissions_api.html | Unexpected error TypeError: 'name' member of PermissionDescriptor 'midi' is not a valid value for enumeration PermissionName.
10:18:49     INFO -      runTests/<@dom/permission/tests/test_permissions_api.html:153:7
10:18:49     INFO -      promise callback*runTests@dom/permission/tests/test_permissions_api.html:134:3
10:18:49     INFO -      onload@dom/permission/tests/test_permissions_api.html:1:1
Flags: needinfo?(mcastelluccio)
Assignee

Comment 14

3 years ago
There was a failure caused by the latest changes to the patch, I've had to move the call to PermissionDescriptor::init() before creating the promise (an assertion was failing because there was a pending exception).
Flags: needinfo?(mcastelluccio)
Assignee

Comment 15

3 years ago
Attachment #8716895 - Attachment is obsolete: true
Attachment #8719557 - Flags: review?(birunthan)
Reporter

Updated

3 years ago
Attachment #8719557 - Flags: review?(birunthan) → review+
Assignee

Comment 16

3 years ago
Unfortunately there's still a failure on e10s

> [Child 19092] ###!!! ASSERTION: Cannot perform action in content process!: 'Error', file extensions/cookie/nsPermissionManager.cpp, line 1813

http://hg.mozilla.org/mozilla-central/file/709f559b5406/extensions/cookie/nsPermissionManager.cpp#l1813
Assignee

Comment 18

3 years ago
Posted patch permissions_revoke_e10s (obsolete) — Splinter Review
Attachment #8725679 - Flags: review?(birunthan)
Assignee

Comment 19

3 years ago
Attachment #8725679 - Attachment is obsolete: true
Attachment #8725679 - Flags: review?(birunthan)
Attachment #8725680 - Flags: review?(birunthan)
Reporter

Comment 20

3 years ago
Comment on attachment 8725680 [details] [diff] [review]
permissions_revoke_e10s

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

::: dom/permission/Permissions.cpp
@@ +179,5 @@
> +    // Permissions can't be removed from the content process. Send a message
> +    // to the parent; `ContentParent::RecvRemovePermission` will call
> +    // `RemovePermission`.
> +    ContentChild::GetSingleton()->SendRemovePermission(
> +      IPC::Principal(document->NodePrincipal()), nsDependentCString(permissionType), &rv);

Ideally I think this should be async (note that we would need to resolve the promise after the async message has been handled). I'm fine this being synchronous for now, but we may have to revisit this when we add worker support.
Attachment #8725680 - Flags: review?(birunthan) → review+

Comment 22

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e978b825517d
https://hg.mozilla.org/mozilla-central/rev/59b8d765123c
https://hg.mozilla.org/mozilla-central/rev/990eeaa39c29
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Assignee

Updated

3 years ago
Flags: in-testsuite+
I've added this method to the MDN docs, including entries and browser compat listings on all related pages:

https://developer.mozilla.org/en-US/docs/Web/API/Permissions_API
https://developer.mozilla.org/en-US/docs/Web/API/Permissions
https://developer.mozilla.org/en-US/docs/Web/API/Permissions/revoke

I also added a new feature to my simple demo to show revoke() in use:

https://chrisdavidmills.github.io/location-finder-permissions-api/
https://developer.mozilla.org/en-US/docs/Web/API/Permissions_API/Using_the_Permissions_API

I've also updated the relevant release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/47#Others

Two questions:

1. Am I using the method wrong in my demo? The state always seems to return "prompt", even after I've revoked the permission.
2. Have I written a reasonable description of when the TypeError exception occurs (see the revoke page, above)? I wasn't sure I'd got this right.

A tech review of the new content would be great, when you get a moment. Thanks!
Regarding your first question, Chris, this is correct. permissions.revoke() will revert the permission to its default state if it was previously granted. Usually, the default state is "prompt" :)
(In reply to Mounir Lamouri (:mounir) from comment #24)
> Regarding your first question, Chris, this is correct. permissions.revoke()
> will revert the permission to its default state if it was previously
> granted. Usually, the default state is "prompt" :)

Great, thanks for the clarification Mounir! I've updated my descriptions a bit in light of this.
Assignee

Comment 26

3 years ago
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #23)
> 2. Have I written a reasonable description of when the TypeError exception
> occurs (see the revoke page, above)? I wasn't sure I'd got this right.

Sounds good to me.
The exception will be thrown also if the permission doesn't exist or is unsupported (e.g. 'midi', or 'push' with userVisibleOnly).
The same is true for Permissions.query().

On https://developer.mozilla.org/en-US/docs/Web/API/Permissions/revoke, the URL to the specification is wrong (https://w3c.github.io/permissions/https://w3c.github.io/permissions/#dom-permissions-revoke), I don't know if it's wrong in other pages as well.
(In reply to Marco Castelluccio [:marco] from comment #26)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #23)
> > 2. Have I written a reasonable description of when the TypeError exception
> > occurs (see the revoke page, above)? I wasn't sure I'd got this right.
> 
> Sounds good to me.
> The exception will be thrown also if the permission doesn't exist or is
> unsupported (e.g. 'midi', or 'push' with userVisibleOnly).
> The same is true for Permissions.query().

Ah, cool - thanks. I've updated the exceptions parts of those pages.

> On https://developer.mozilla.org/en-US/docs/Web/API/Permissions/revoke, the
> URL to the specification is wrong
> (https://w3c.github.io/permissions/https://w3c.github.io/permissions/#dom-
> permissions-revoke), I don't know if it's wrong in other pages as well.

Hrm, don't know how I managed to get that wrong. Thanks for the fix ;-)

Comment 28

3 years ago
The specification isn't clear about what happens when you have, for example, {name:"midi", sysex:false} permission and you try to revoke {name:"midi", sysex:true}. What does Firefox's implementation do? How about the opposite set of sysex values?
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.