Implement support for permissions API. "The Permissions API allows a web application to be aware of the status of a given permission, to know whether it is granted, denied or if the user will be asked whether the permission should be granted."

It's great to see some progress on the Permissions API in Gecko! :) FYI, PermissionStatus.status has been renamed to PermissionStatus.state, see https://github.com/w3c/permissions/issues/31

Comment on attachment 8622796 [details] [diff] [review] Part 1: Add stub PermissionStatus implementation baku, can you review these? I'll address comment 6 add support for the onchange event handler and Workers in future patches.

Comment on attachment 8622796 [details] [diff] [review] Part 1: Add stub PermissionStatus implementation Review of attachment 8622796 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/permission/PermissionStatus.h @@ +3,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef mozilla_dom_permissionstatus_h_ mozilla_dom_PermissionStatus_h @@ +21,5 @@ > + > + JSObject* WrapObject(JSContext* aCx, > + JS::Handle<JSObject*> aGivenProto) override; > + > + PermissionState Status() { return mState; } const ::: modules/libpref/init/all.js @@ +124,5 @@ > // Enable profiler marks for indexedDB events. > pref("dom.indexedDB.logging.profiler-marks", false); > > +// Whether or not the Permissions API is enabled. > +pref("dom.permissions.enabled", false); In the test_interfaces you wrote that it's enable by default in non-release build. I guess this should be: #ifdef RELEASE_BUILD pref("dom.permissions.enabled", false); else pref("dom.permissions.enabled", true); #endif

Comment on attachment 8622797 [details] [diff] [review] Part 2: Add stub Permissions implementation Review of attachment 8622797 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/permission/Permissions.cpp @@ +48,5 @@ > + } > + > + ErrorResult aRv; > + nsRefPtr<Promise> promise = Promise::Create(global, aRv); > + if (aRv.Failed()) { NS_WARN_IF ::: dom/permission/Permissions.h @@ +12,5 @@ > +#include "nsWrapperCache.h" > + > +namespace mozilla { > + > +class ErrorResult; you actually don't use this. remove it. ::: dom/webidl/Permissions.webidl @@ +2,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. > + */ > + some URL with the documentation or where this idl is from? same for PermissionStatus. @@ +11,5 @@ > + "midi" > +}; > + > +dictionary PermissionDescriptor { > + required PermissionName name; a default value?

Comment on attachment 8622798 [details] [diff] [review] Part 3: Implement Permissions.query Review of attachment 8622798 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/permission/Permissions.cpp @@ +40,5 @@ > return PermissionsBinding::Wrap(aCx, this, aGivenProto); > } > > +static const char* > +PermissionNameToString(PermissionName aName) use a anonymous namespace instead static functions. @@ +50,5 @@ > + case PermissionName::Notifications: > + return "desktop-notification"; > + > + case PermissionName::Push: > + return nullptr; I need a bit more context to understand why this is 'nullptr'. @@ +80,4 @@ > already_AddRefed<Promise> > Permissions::Query(const PermissionDescriptor& aPermission) > { > + nsCOMPtr<nsIPermissionManager> permMgr = services::GetPermissionManager(); move it close to where you actually use it. @@ +81,5 @@ > Permissions::Query(const PermissionDescriptor& aPermission) > { > + nsCOMPtr<nsIPermissionManager> permMgr = services::GetPermissionManager(); > + if (!permMgr) { > + return nullptr; plus, if you return null, you must return an error. So, maybe you want to mark this method with [Throw] in the webidl file. @@ +88,3 @@ > nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(mWindow); > if (!global) { > return nullptr; here too.

Comment on attachment 8622800 [details] [diff] [review] Part 5: Add test for Permissions.query Review of attachment 8622800 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/permission/tests/test_permissions_api.html @@ +11,5 @@ > + <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/> > +</head> > +<body onload='runTests()'> > +<p id="display"></p> > +<div id="content" style="display: none"></div> remove this <p> and this <div>

Comment on attachment 8632156 [details] [diff] [review] Part 3: Implement Permissions.query (In reply to Andrea Marchesini (:baku) from comment #9) > @@ +11,5 @@ > > + "midi" > > +}; > > + > > +dictionary PermissionDescriptor { > > + required PermissionName name; > > a default value? Not sure I understand, could you clarify? (In reply to Andrea Marchesini (:baku) from comment #10) > Comment on attachment 8622798 [details] [diff] [review] > Part 3: Implement Permissions.query > > Review of attachment 8622798 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/permission/Permissions.cpp > @@ +40,5 @@ > > return PermissionsBinding::Wrap(aCx, this, aGivenProto); > > } > > > > +static const char* > > +PermissionNameToString(PermissionName aName) > > use a anonymous namespace instead static functions. > > @@ +50,5 @@ > > + case PermissionName::Notifications: > > + return "desktop-notification"; > > + > > + case PermissionName::Push: > > + return nullptr; > > I need a bit more context to understand why this is 'nullptr'. I figured it would be best to support "push" together with PushPermissionDescriptor[0]. [0]: https://w3c.github.io/permissions/#push

Comment on attachment 8632156 [details] [diff] [review] Part 3: Implement Permissions.query Review of attachment 8632156 [details] [diff] [review]: ----------------------------------------------------------------- Ok. but I don't like that we return NS_ERROR_NOT_IMPLEMENTED. Land this code when all is implemented... follow up? or new patches? ::: dom/permission/Permissions.cpp @@ +99,5 @@ > + const char* name = PermissionNameToString(aPermission.mName); > + if (name) { > + nsCOMPtr<nsIPermissionManager> permMgr = services::GetPermissionManager(); > + if (!permMgr) { > + aRv.Throw(NS_ERROR_UNEXPECTED); promise->MaybeReject(NS_ERROR_UNEXPECTED); return promise.forget(); @@ +108,5 @@ > + permMgr->TestPermissionFromWindow(mWindow, name, &action); > + promise->MaybeResolve( > + new PermissionStatus(mWindow, ActionToPermissionState(action))); > + } else { > + promise->MaybeReject(NS_ERROR_NOT_IMPLEMENTED); maybe it's better if you do: if (!name) { promise->MaybeReject(NS_ERROR_NOT_IMPLEMENTED); return promise.forget(); } ...

This changes the Permissions.query parameter to an object rather than a PermissionDescriptor in order to construct a PushPermissionDescriptor in the next patch.

Comment on attachment 8635553 [details] [diff] [review] Part 3: Implement Permissions.query Review of attachment 8635553 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/permission/Permissions.cpp @@ +94,5 @@ > return nullptr; > } > > + PermissionDescriptor permission; > + JS::Rooted<JS::Value> value(aCx, JS::ObjectOrNullValue(aPermission)); Just use PermissionDescriptor in the webIDL file. @@ +103,5 @@ > + > + PermissionState state = PermissionState::Denied; > + switch (permission.mName) { > + case PermissionName::Geolocation: > + aRv = CheckPermission("geo", mWindow, state); Don't use aRv here. You have a promise obj. Use it setting the correct value. ::: dom/webidl/Permissions.webidl @@ +21,5 @@ > [Exposed=(Window), > Pref="dom.permissions.enabled"] > interface Permissions { > + [Throws] > + Promise<PermissionStatus> query(object permission); why this 'object'? Why not PermissionDescriptor?

(In reply to Andrea Marchesini (:baku) from comment #17) > why this 'object'? Why not PermissionDescriptor? It used to be PermissionDescriptor in the previous version of the patch. As mentioned in commenet 15, I changed it to an object because we need to be able to construct *both* a PermissionDescriptor and a PushPermissionDescriptor (see part 3.1). Can you think of a better way to do this?

baku, ping. I'm rerequesting review for this, please see comment 18. (In reply to Andrea Marchesini (:baku) from comment #17) > @@ +103,5 @@ > > + > > + PermissionState state = PermissionState::Denied; > > + switch (permission.mName) { > > + case PermissionName::Geolocation: > > + aRv = CheckPermission("geo", mWindow, state); > > Don't use aRv here. You have a promise obj. Use it setting the correct value. Done.

Comment on attachment 8635555 [details] [diff] [review] Part 3.1: Add support for 'push' to Permissions.query I'm rerequesting review for this, please see comment 18.

Comment on attachment 8635555 [details] [diff] [review] Part 3.1: Add support for 'push' to Permissions.query Review of attachment 8635555 [details] [diff] [review]: ----------------------------------------------------------------- A test about this? ::: dom/permission/Permissions.cpp @@ +79,5 @@ > +CheckPushPermission(JSContext* aCx, > + JS::Handle<JSObject*> aPermission, > + nsPIDOMWindow* aWindow, > + PermissionState& aResult) > +{ MOZ_ASSERT(aCx); MOZ_ASSERT(aPermission);

Comment on attachment 8639394 [details] [diff] [review] Part 3: Implement Permissions.query Review of attachment 8639394 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/permission/Permissions.cpp @@ +66,5 @@ > + MOZ_ASSERT(aName); > + MOZ_ASSERT(aWindow); > + > + nsCOMPtr<nsIPermissionManager> permMgr = services::GetPermissionManager(); > + NS_ENSURE_TRUE(permMgr, NS_ERROR_UNEXPECTED); if (NS_WARN_IF(!permMgr)) { return NS_ERROR_FAILURE; } @@ +69,5 @@ > + nsCOMPtr<nsIPermissionManager> permMgr = services::GetPermissionManager(); > + NS_ENSURE_TRUE(permMgr, NS_ERROR_UNEXPECTED); > + > + uint32_t action = nsIPermissionManager::DENY_ACTION; > + permMgr->TestPermissionFromWindow(aWindow, aName, &action); this can fail. nsresult rv = permMgr->... if (NS_WARN_IF(... @@ +91,3 @@ > nsRefPtr<Promise> promise = Promise::Create(global, aRv); > if (NS_WARN_IF(aRv.Failed())) { > + aRv.Throw(NS_ERROR_UNEXPECTED); Remove this line. aRv already contains a error message. @@ +96,5 @@ > > + PermissionDescriptor permission; > + JS::Rooted<JS::Value> value(aCx, JS::ObjectOrNullValue(aPermission)); > + if (!permission.Init(aCx, value)) { > + aRv.Throw(NS_ERROR_UNEXPECTED); Remove this line and do: promise->MaybeReject(NS_ERROR_UNEXPECTED); return promise.forget(); @@ +103,5 @@ > > + PermissionState state = PermissionState::Denied; > + switch (permission.mName) { > + case PermissionName::Geolocation: > + if (NS_FAILED(CheckPermission("geo", mWindow, state)) { NS_WARN_IF @@ +110,5 @@ > + } > + break; > + > + case PermissionName::Notifications: > + if (NS_FAILED(CheckPermission("desktop-notification", mWindow, state)) { same here.

Requesting review again because I decided to refactor Part 3 a bit (reducing the promise boilerplate etc.). I went with a single NS_WARN_IF that is eventually hit all code paths.

This adds tests for push.

Comment on attachment 8639535 [details] [diff] [review] Part 3: Implement Permissions.query Review of attachment 8639535 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/permission/Permissions.cpp @@ +72,5 @@ > + } > + > + uint32_t action = nsIPermissionManager::DENY_ACTION; > + nsresult rv = permMgr->TestPermissionFromWindow(aWindow, aName, &action); > + if (NS_FAILED(rv)) { if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } @@ +88,5 @@ > + PermissionState& aResult) > +{ > + PushPermissionDescriptor permission; > + JS::Rooted<JS::Value> value(aCx, JS::ObjectOrNullValue(aPermission)); > + if (!permission.Init(aCx, value)) { NS_WARN_IF @@ +96,5 @@ > + if (permission.mUserVisible) { > + return NS_ERROR_NOT_IMPLEMENTED; > + } > + > + return CheckPermission("push", aWindow, aResult); I prefer to read: nsresult rv = CheckPermission(...) if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } return NS_OK; @@ +109,5 @@ > + PermissionDescriptor permission; > + JS::Rooted<JS::Value> value(aCx, JS::ObjectOrNullValue(aPermission)); > + if (!permission.Init(aCx, value)) { > + return NS_ERROR_UNEXPECTED; > + } extra space

https://hg.mozilla.org/integration/mozilla-inbound/rev/d03882fd396c https://hg.mozilla.org/integration/mozilla-inbound/rev/adf77961b360 https://hg.mozilla.org/integration/mozilla-inbound/rev/f276dc2b667b https://hg.mozilla.org/integration/mozilla-inbound/rev/b2d9e0cdfc4d https://hg.mozilla.org/integration/mozilla-inbound/rev/b308fdf94e9e

https://hg.mozilla.org/mozilla-central/rev/d03882fd396c https://hg.mozilla.org/mozilla-central/rev/adf77961b360 https://hg.mozilla.org/mozilla-central/rev/f276dc2b667b https://hg.mozilla.org/mozilla-central/rev/b2d9e0cdfc4d https://hg.mozilla.org/mozilla-central/rev/b308fdf94e9e

This is in prepartion of a subsequent change where we will update the state within PermissionStatus.

Comment on attachment 8641321 [details] [diff] [review] Part 7: Add helpers to convert between PermissionName and permission type Review of attachment 8641321 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/permission/Permissions.cpp @@ +97,5 @@ > return NS_ERROR_UNEXPECTED; > } > > switch (permission.mName) { > case PermissionName::Geolocation: // fall through.

Comment on attachment 8641326 [details] [diff] [review] Part 8: Move permission checking into PermissionStatus Review of attachment 8641326 [details] [diff] [review]: ----------------------------------------------------------------- I prefer to see this patch again. ::: dom/permission/PermissionStatus.cpp @@ +21,2 @@ > { > + NS_WARN_IF(NS_FAILED(UpdateState())); instead doing this, do this: 1. make PermissionStatus constructor private. 2. do a: static method like this: /* static */ nsresult PermissionStatus::Create(nsPIDOMWindow* aWindow, PermissionName aName, PermissionStatus** aStatus) { MOZ_ASSERT(aStatus); *aStatus = nullptr; nsAutoPtr<PermissionStatus> status = new PermissionStatus(aWindow, aName); nsresult rv = status->UpdateState(); if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } status.forget(aStatus); return NS_OK; } @@ +44,5 @@ > + nsresult rv = permMgr->TestPermissionFromWindow(GetOwner(), > + PermissionNameToType(mName), > + &action); > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return NS_ERROR_FAILURE; return rv; ::: dom/permission/PermissionStatus.h @@ +17,5 @@ > class PermissionStatus final > : public DOMEventTargetHelper > { > public: > + PermissionStatus(nsPIDOMWindow* aWindow, PermissionName aName); move it to private. @@ +24,5 @@ > JS::Handle<JSObject*> aGivenProto) override; > > PermissionState State() const { return mState; } > > + nsresult UpdateState(); this can be private. ::: dom/permission/Permissions.cpp @@ +57,5 @@ > if (permission.mUserVisible) { > return NS_ERROR_NOT_IMPLEMENTED; > } > > + *aResult = new PermissionStatus(aWindow, permission.mName); This will be: return PermissionStatus::Create(aWindow, permission.mName, aResult); @@ +76,5 @@ > > switch (permission.mName) { > case PermissionName::Geolocation: > case PermissionName::Notifications: > + *aResult = new PermissionStatus(aWindow, permission.mName); ditto.

Comment on attachment 8641353 [details] [diff] [review] Part 9: Add PermissionObserver to watch for perm-changed notifications Review of attachment 8641353 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/permission/PermissionObserver.cpp @@ +27,5 @@ > + if (sSelf) { > + return NS_OK; > + } > + > + nsCOMPtr<nsIObserverService> obs = services::GetObserverService(); move this to line 38. @@ +35,5 @@ > + > + sSelf = new PermissionObserver(); > + NS_ADDREF(sSelf); > + > + obs->AddObserver(sSelf, "perm-changed", true); This can fail. @@ +74,5 @@ > +} > + > +NS_IMETHODIMP > +PermissionObserver::Observe(nsISupports* aSubject, > + const char* aTopic, indentation ::: layout/build/nsLayoutStatics.cpp @@ +238,5 @@ > NS_ERROR("Could not initialize DOMStorageObserver"); > return rv; > } > > + rv = PermissionObserver::Init(); Instead doing this, what about if PermissionStatus keeps a reference of PermissionObserver? I would do this: 1. Create a static method like: GetOrCreate() 2. if sSelf is null, you create it 3. PermissionObserver is kept alive by PermissionStatus. If you do this you don't need Init and Shutdown.

This is in prepartion of a subsequent change where we will update the state within PermissionStatus.

I forgot to do this in Part 1.

Comment on attachment 8641730 [details] [diff] [review] Part 8: Move permission checking into PermissionStatus Review of attachment 8641730 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/permission/Permissions.cpp @@ +57,5 @@ > if (permission.mUserVisible) { > return NS_ERROR_NOT_IMPLEMENTED; > } > > + just 1 empty line, not 2.

Comment on attachment 8641895 [details] [diff] [review] Part 9: Add PermissionObserver to watch for perm-changed notifications Review of attachment 8641895 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but I would like to see it again for memory management of PermissionObserver. ::: dom/permission/PermissionObserver.cpp @@ +22,5 @@ > + > +/* static */ already_AddRefed<PermissionObserver> > +PermissionObserver::GetInstance() > +{ > + static nsRefPtr<PermissionObserver> sInstance; Instead doing this, do namespace { PermissionObserver* sInstance = nullptr; } // anonymous namespace /* static */ already_AddRefed<PermissionObserver> PermissionObserver::GetInstance() { nsRefPtr<PermissionObserver> instance = sInstance; if (!instance) { instance = new PermissionObserver(); ... sInstance = instance; } return instance.forget(); } Plus, add: PermissionObserver::PermissionObserver() { MOZ_ASSERT(!sInstance); } @@ +45,5 @@ > +} > + > +void > +PermissionObserver::AddSink(PermissionStatus* aSink) > +{ MOZ_ASSERT(aSink); MOZ_ASSERT(!mSinks.Contains(aSink)); @@ +51,5 @@ > +} > + > +void > +PermissionObserver::RemoveSink(PermissionStatus* aSink) > +{ MOZ_ASSERT(aSink); MOZ_ASSERT(mSinks.Contains(aSink)); ::: dom/permission/PermissionObserver.h @@ +20,5 @@ > +class PermissionStatus; > + > +// Singleton that watches for perm-changed notifications in order to notify > +// PermissionStatus objects. > +class PermissionObserver final? @@ +34,5 @@ > + void AddSink(PermissionStatus* aObs); > + void RemoveSink(PermissionStatus* aObs); > + > +private: > + virtual ~PermissionObserver() {} move it into the cpp file and do: 1. MOZ_ASSERT(mSinks.IsEmpty()); 2. MOZ_ASSERT(sInstance == this); 3. sInstance = nullptr; ::: dom/permission/PermissionStatus.cpp @@ +10,5 @@ > #include "mozilla/Services.h" > #include "mozilla/UniquePtr.h" > #include "nsIPermissionManager.h" > #include "PermissionUtils.h" > +#include "PermissionObserver.h" alphabetic order. @@ +46,5 @@ > +PermissionStatus::Init() > +{ > + mObserver = PermissionObserver::GetInstance(); > + if (NS_WARN_IF(!mObserver)) { > + return NS_ERROR_NOT_AVAILABLE; NS_ERROR_FAILURE ? ::: dom/permission/PermissionStatus.h @@ +43,3 @@ > nsresult UpdateState(); > > + nsIPrincipal* GetPrincipal(); can this be const? @@ +43,5 @@ > nsresult UpdateState(); > > + nsIPrincipal* GetPrincipal(); > + > + void PermissionChanged() {} why this?

Comment on attachment 8641906 [details] [diff] [review] Part 11: Use NS_DECL_ISUPPORTS_INHERITED with PermissionStatus Review of attachment 8641906 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/permission/PermissionStatus.cpp @@ +34,5 @@ > nsresult rv = status->Init(); > if (NS_WARN_IF(NS_FAILED(rv))) { > return rv; > } > Why this change? Tell me more. Then I would change this ::Create to: already_AddRefed<PermissionStatus> PermissionStatus::Create(nsPIDOMWindow* aWindow, PermissionName aName, ErrorResult& aRv);

Comment on attachment 8641906 [details] [diff] [review] Part 11: Use NS_DECL_ISUPPORTS_INHERITED with PermissionStatus wchen told me this is not needed, but I forgot to obsolete this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e13c9c09273 https://hg.mozilla.org/integration/mozilla-inbound/rev/92d2843b5938 https://hg.mozilla.org/integration/mozilla-inbound/rev/fc34b9b8e874 https://hg.mozilla.org/integration/mozilla-inbound/rev/7ee1772ffc10 https://hg.mozilla.org/integration/mozilla-inbound/rev/b78a97800675

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/44204bf72cbf https://hg.mozilla.org/integration/mozilla-inbound/rev/7b6f60b570c2 https://hg.mozilla.org/integration/mozilla-inbound/rev/2ec72d95fe87 https://hg.mozilla.org/integration/mozilla-inbound/rev/019492cdccf9 https://hg.mozilla.org/integration/mozilla-inbound/rev/11467fe19249

This has B2G mochitest failures as well. https://treeherder.mozilla.org/logviewer.html#?job_id=12749800&repo=mozilla-inbound

Comment on attachment 8641906 [details] [diff] [review] Part 11: Use NS_DECL_ISUPPORTS_INHERITED with PermissionStatus Unobsoleting this because this seems to fix the hazard caused by using a UniquePtr in PermissionStatus::Create: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux64-br-haz/20150811083337/hazards.txt.gz I'm not quite sure why, though.

Comment on attachment 8641906 [details] [diff] [review] Part 11: Use NS_DECL_ISUPPORTS_INHERITED with PermissionStatus Review of attachment 8641906 [details] [diff] [review]: ----------------------------------------------------------------- can you send me all these patches merged in one? ::: dom/permission/PermissionStatus.cpp @@ +18,5 @@ > +NS_IMPL_ADDREF_INHERITED(PermissionStatus, DOMEventTargetHelper) > +NS_IMPL_RELEASE_INHERITED(PermissionStatus, DOMEventTargetHelper) > + > +NS_INTERFACE_MAP_BEGIN(PermissionStatus) > +NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper) remove all of this. PermissionStatus is a DOMEventTargetHelper. It doesn't require all of this. @@ +34,5 @@ > nsresult rv = status->Init(); > if (NS_WARN_IF(NS_FAILED(rv))) { > return rv; > } > I think it's better if you do: already_AddRefed<PermissionStatus> PermissionStatus::Create(nsPIDOMWindow* aWindow, PermissionName aName, ErrorResult& aRv) { MOZ_ASSERT(aWindow); nsRefPtr<PermissionStatus> status = new PermissionStatus(aWindow, aName); aRv = status->Init(); if (NS_WARN_IF(aRv.Failed())) { return nullptr; } return status.forget(); } } ::: dom/permission/PermissionStatus.h @@ +21,5 @@ > { > friend class PermissionObserver; > > public: > + NS_DECL_ISUPPORTS_INHERITED remove this. @@ +35,5 @@ > > IMPL_EVENT_HANDLER(change) > > private: > + virtual ~PermissionStatus(); remove virtual. This class is 'final'.

Please send an intent to implement/ship for this. Thanks!

(In reply to Andrea Marchesini (:baku) from comment #49) > can you send me all these patches merged in one? Here you go.

https://hg.mozilla.org/integration/mozilla-inbound/rev/161351ddf3c5 https://hg.mozilla.org/integration/mozilla-inbound/rev/f7b73d985f8a https://hg.mozilla.org/integration/mozilla-inbound/rev/b67133d730e1 https://hg.mozilla.org/integration/mozilla-inbound/rev/657af0e9e13a https://hg.mozilla.org/integration/mozilla-inbound/rev/1bb4c708f660 https://hg.mozilla.org/integration/mozilla-inbound/rev/413c77eaef08 https://hg.mozilla.org/integration/mozilla-inbound/rev/6e3e2cc1988e

The initial scope of this bug (navigator.permissions.query) has been implemented so removing leave-open. See dependant bugs for the other bits.

(In reply to Ehsan Akhgari (don't ask for review please) from comment #50) > Please send an intent to implement/ship for this. Thanks! Done: https://lists.mozilla.org/pipermail/dev-platform/2015-August/011466.html

https://hg.mozilla.org/mozilla-central/rev/161351ddf3c5 https://hg.mozilla.org/mozilla-central/rev/f7b73d985f8a https://hg.mozilla.org/mozilla-central/rev/b67133d730e1 https://hg.mozilla.org/mozilla-central/rev/657af0e9e13a https://hg.mozilla.org/mozilla-central/rev/1bb4c708f660 https://hg.mozilla.org/mozilla-central/rev/413c77eaef08 https://hg.mozilla.org/mozilla-central/rev/6e3e2cc1988e

The Permissions API has been documented (with help from Joe at Google - thanks Joe!) See https://developer.mozilla.org/en-US/docs/Web/API/Permissions_API (main landing page) https://developer.mozilla.org/en-US/docs/Web/API/Navigator/permissions https://developer.mozilla.org/en-US/docs/Web/API/WorkerNavigator/permissions https://developer.mozilla.org/en-US/docs/Web/API/Permissions https://developer.mozilla.org/en-US/docs/Web/API/PermissionStatus https://developer.mozilla.org/en-US/docs/Web/API/Permissions_API/Using_the_Permissions_API (beginner's guide) I've also added a note in the relevant release notes: https://developer.mozilla.org/en-US/Firefox/Releases/43#New_APIs_2 A tech review would be lovely - cheers! I will update the content when the missing methods are implemented.

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #56) > The Permissions API has been documented (with help from Joe at Google - > thanks Joe!) Looks good, thank you. I made a few corrections (mostly about WorkerNavigator.permissions not being implemented yet).

(In reply to Birunthan Mohanathas [:poiru] from comment #57) > (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #56) > > The Permissions API has been documented (with help from Joe at Google - > > thanks Joe!) > > Looks good, thank you. I made a few corrections (mostly about > WorkerNavigator.permissions not being implemented yet). Cool, thanks!

Chris, this was will not ship in 43 due to bug 1221104 and bug 1221106. I'll go ahead and update the MDN pages accordingly. I'm not sure if some other pages (e.g. release notes?) needs to be fixed as well.

(In reply to Birunthan Mohanathas [:poiru] from comment #59) > Chris, this was will not ship in 43 due to bug 1221104 and bug 1221106. I'll > go ahead and update the MDN pages accordingly. I'm not sure if some other > pages (e.g. release notes?) needs to be fixed as well. I've checked, and it looks like you have this all nicely done - thanks.