Closed Bug 1295877 Opened 3 years ago Closed 3 years ago

Put Permissions API .revoke() behind a pref

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: marcosc, Assigned: marcosc)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(1 file, 1 obsolete file)

It seems we prematurely shipped the .revoke() method on the Permissions API before it was stable or deciding if we even wanted it in the platform. 

For those that don't know it: navigator.permission.revoke() allows a site to self-revoke a permission after a user has granted that permission. For example, a user may grant foo.com access to geolocation, but upon signing out of a site, a site might call .revoke({name:"geolocation"}) so that the next user to log into the site doesn't automatically get access to geolocation (because permissions are bound to origin).  

A few folks (who can chime in) working on the standard have raised concerns about the design, so we would like to suggest we put it behind a pref for now. Particularly, using in-browser user profiles can handle the above use case without a site taking away a user's decision. 

There seems to be consensus that .query() is useful, so that one can remain.
Assignee: nobody → mcaceres
Birunthan, because the .revoke() API is now behind a pref, I had to refactor the tests to use an iframe's window object.   

I also cleaned up the tests, as there were lots of unused variables, and things upsetting jshint.
Attachment #8781861 - Flags: review?(birunthan)
Comment on attachment 8781861 [details] [diff] [review]
Puts .revoke() behind pref. Refactors tests.

Bobby, can you kindly review the IDL change?
Attachment #8781861 - Flags: superreview?(bobbyholley)
Comment on attachment 8781861 [details] [diff] [review]
Puts .revoke() behind pref. Refactors tests.

Ah, my git blame archeology was off, :marco actually implemented .revoke().
Attachment #8781861 - Flags: review?(birunthan) → review?(mcastelluccio)
Comment on attachment 8781861 [details] [diff] [review]
Puts .revoke() behind pref. Refactors tests.

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

r=me on the webidl change assuming there continues to be no objection to it on the dev-platform thread.

Do we need an "Intent to unship" or did we never actually ship it?
Attachment #8781861 - Flags: superreview?(bobbyholley) → superreview+
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5)
> Do we need an "Intent to unship" or did we never actually ship it?

I was unsure, does pref'ing in off count as "unship"? We did ship it.
Flags: needinfo?(bobbyholley)
*it
I'd think so, but looking back I think the existing dev-platform thread is probably sufficient.
Flags: needinfo?(bobbyholley)
Attachment #8781861 - Flags: review?(mcastelluccio) → review?(amarchesini)
Attachment #8781861 - Flags: review?(amarchesini) → review+
Keywords: checkin-needed
Attachment #8781861 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19c4f264ce37
Put Permissions API .revoke() behind a pref. r=baku, sr=bholley
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/19c4f264ce37
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Marcos, why did you add |debugger;| in the test?
https://hg.mozilla.org/mozilla-central/rev/19c4f264ce37#l3.237
Flags: needinfo?(mcaceres)
> Marcos, why did you add |debugger;| in the test?

I forgot to remove that before we merged. Please feel free to remove.
Flags: needinfo?(mcaceres)
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/349dbf222bac
Remove accidentally added |debugger;|. r=marcosc
(In reply to Marcos Caceres [:marcosc] from comment #16)
> > Marcos, why did you add |debugger;| in the test?
> 
> I forgot to remove that before we merged. Please feel free to remove.

Thanks, pushed a remove commit.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.