1105827 - Implement support for Permissions API

Status: RESOLVED FIXED

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

Description

[Marcos Caceres [:marcosc]]

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."

Comment 1

[Biru [:poiru]]

Attachment: Part 1: Add stub PermissionStatus implementation

Comment 2

[Biru [:poiru]]

Attachment: Part 2: Add stub Permissions implementation

Comment 3

[Biru [:poiru]]

Attachment: Part 3: Implement Permissions.query

Comment 4

[Biru [:poiru]]

Attachment: Part 4: Add Navigator.permissions

Comment 5

[Biru [:poiru]]

Attachment: Part 5: Add test for Permissions.query

Comment 6

[Mounir Lamouri (:mounir)]

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 7

[Biru [:poiru]]

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 8

[Andrea Marchesini [:baku]]

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 9

[Andrea Marchesini [:baku]]

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 10

[Andrea Marchesini [:baku]]

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 11

[Andrea Marchesini [:baku]]

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 12

[Biru [:poiru]]

Attachment: Part 3: Implement Permissions.query

Comment 13

[Biru [:poiru]]

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 14

[Andrea Marchesini [:baku]]

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();
}

...

Comment 15

[Biru [:poiru]]

Attachment: Part 3: Implement Permissions.query

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

Comment 16

[Biru [:poiru]]

Attachment: Part 3.1: Add support for 'push' to Permissions.query

Comment 17

[Andrea Marchesini [:baku]]

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?

Comment 18

[Biru [:poiru]]

(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?

Comment 19

[Biru [:poiru]]

Attachment: Part 3: Implement Permissions.query

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 20

[Biru [:poiru]]

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 21

[Andrea Marchesini [:baku]]

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 22

[Andrea Marchesini [:baku]]

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.

Comment 23

[Biru [:poiru]]

Attachment: Part 3: Implement Permissions.query

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.

Comment 24

[Biru [:poiru]]

Attachment: Part 5: Add test for Permissions.query

This adds tests for push.

Comment 25

[Andrea Marchesini [:baku]]

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

Comment 26

[Biru [:poiru]]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Comment 28

[Biru [:poiru]]

Attachment: Part 6: Add PermissionUtils.h for helper functions

Comment 29

[Biru [:poiru]]

Attachment: Part 7: Add helpers to convert between PermissionName and permission type

Comment 30

[Biru [:poiru]]

Attachment: Part 8: Move permission checking into PermissionStatus

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

Comment 31

[Biru [:poiru]]

Attachment: Part 9: Add PermissionObserver to watch for perm-changed notifications

Comment 32

[Biru [:poiru]]

Attachment: Part 9: Add PermissionObserver to watch for perm-changed notifications

Comment 33

[Andrea Marchesini [:baku]]

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 34

[Andrea Marchesini [:baku]]

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 35

[Andrea Marchesini [:baku]]

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.

Comment 36

[Biru [:poiru]]

Attachment: Part 8: Move permission checking into PermissionStatus

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

Comment 37

[Biru [:poiru]]

Attachment: Part 9: Add PermissionObserver to watch for perm-changed notifications

Comment 38

[Biru [:poiru]]

Attachment: Part 10: Fire change event for PermissionStatus objects

Comment 39

[Biru [:poiru]]

Attachment: Part 11: Use NS_DECL_ISUPPORTS_INHERITED with PermissionStatus

I forgot to do this in Part 1.

Comment 40

[Andrea Marchesini [:baku]]

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 41

[Andrea Marchesini [:baku]]

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 42

[Andrea Marchesini [:baku]]

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 43

[Biru [:poiru]]

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.

Comment 44

[Biru [:poiru]]

Attachment: Part 9: Add PermissionObserver to watch for perm-changed notifications

Comment 45

[Pulsebot]

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

Comment 46

[Pulsebot]

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

Comment 47

[Ryan VanderMeulen [:RyanVM]]

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

Comment 48

[Biru [:poiru]]

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 49

[Andrea Marchesini [:baku]]

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'.

Comment 50

[(no longer active)]

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

Comment 51

[Biru [:poiru]]

Attachment: All patches (Part 1 - Part 11)

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

Here you go.

Comment 52

[Pulsebot]

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

Comment 53

[Biru [:poiru]]

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

Comment 54

[Biru [:poiru]]

(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

Comment 55

[Ryan VanderMeulen [:RyanVM]]

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

Comment 56

[Chris Mills [:cmills]]

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.

Comment 57

[Biru [:poiru]]

(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).

Comment 58

[Chris Mills [:cmills]]

(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!

Comment 59

[Biru [:poiru]]

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.

Comment 60

[Chris Mills [:cmills]]

(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.