Implement Permissions.revoke

RESOLVED FIXED in Firefox 47

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: poiru, Assigned: marco, Mentored)

Tracking

({dev-doc-complete})

unspecified
mozilla47
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox47 fixed)

Details

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

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
This is now part of the spec. We should implement it.
(Reporter)

Updated

2 years ago
Assignee: birunthan → nobody
Mentor: birunthan@mohanathas.com
Status: ASSIGNED → NEW
Whiteboard: [lang=c++]
(Assignee)

Comment 1

2 years ago
Created attachment 8716134 [details] [diff] [review]
permissions_revoke
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
Attachment #8716134 - Flags: review?(birunthan)
(Assignee)

Comment 2

2 years ago
Created attachment 8716135 [details] [diff] [review]
permissions_revoke_tests
Attachment #8716135 - Flags: review?(birunthan)
(Reporter)

Comment 3

2 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

2 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

2 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

2 years ago
Created attachment 8716895 [details] [diff] [review]
permissions_revoke
Attachment #8716134 - Attachment is obsolete: true
Attachment #8716895 - Flags: review?(birunthan)
(Assignee)

Comment 7

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=765d3d89dd58
(Reporter)

Updated

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

Comment 8

2 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

2 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+
(Assignee)

Comment 10

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c046a81ab5a373e8ce2c2a8c16aca43eedf4914f
Bug 1197461 - Implement Permissions.revoke. r=poiru,baku

https://hg.mozilla.org/integration/mozilla-inbound/rev/d776c61906c17234f60c8df54ed4f913959eb5eb
Bug 1197461 - Tests for Permissions.revoke. r=poiru,baku
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)

Comment 12

2 years ago
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24876669d491
(Assignee)

Comment 13

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1aaff5d559f
(Assignee)

Comment 14

2 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

2 years ago
Created attachment 8719557 [details] [diff] [review]
permissions_revoke
Attachment #8716895 - Attachment is obsolete: true
Attachment #8719557 - Flags: review?(birunthan)
(Reporter)

Updated

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

Comment 16

2 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 17

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=75b24125b054
(Assignee)

Comment 18

2 years ago
Created attachment 8725679 [details] [diff] [review]
permissions_revoke_e10s
Attachment #8725679 - Flags: review?(birunthan)
(Assignee)

Comment 19

2 years ago
Created attachment 8725680 [details] [diff] [review]
permissions_revoke_e10s
Attachment #8725679 - Attachment is obsolete: true
Attachment #8725679 - Flags: review?(birunthan)
Attachment #8725680 - Flags: review?(birunthan)
(Reporter)

Comment 20

2 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+
(Assignee)

Comment 21

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e978b825517d6d06d1071d179d5f0d89a069733c
Bug 1197461 - Implement Permissions.revoke. r=poiru,baku

https://hg.mozilla.org/integration/mozilla-inbound/rev/59b8d765123ce48d13db01a00a3d3fc628ad67e8
Bug 1197461 - e10s support for Permissions.revoke. r=poiru

https://hg.mozilla.org/integration/mozilla-inbound/rev/990eeaa39c299b43c06b5b9b04951c8cf8b8b421
Bug 1197461 - Tests for Permissions.revoke. r=poiru,baku

Comment 22

2 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: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Updated

2 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!
Keywords: dev-doc-needed → dev-doc-complete
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

a year 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

a year 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?
You need to log in before you can comment on or make changes to this bug.