Implement support for Permissions API

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
4 years ago
13 days ago

People

(Reporter: marcosc, Assigned: poiru)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla43
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox43 fixed)

Details

(URL)

Attachments

(12 attachments, 10 obsolete attachments)

7.18 KB, patch
baku
: review+
Details | Diff | Splinter Review
6.81 KB, patch
baku
: review+
Details | Diff | Splinter Review
6.14 KB, patch
baku
: review+
Details | Diff | Splinter Review
5.95 KB, patch
baku
: review+
Details | Diff | Splinter Review
4.27 KB, patch
baku
: review+
Details | Diff | Splinter Review
3.99 KB, patch
baku
: review+
Details | Diff | Splinter Review
4.97 KB, patch
baku
: review+
Details | Diff | Splinter Review
7.83 KB, patch
baku
: review+
Details | Diff | Splinter Review
3.82 KB, patch
baku
: review+
Details | Diff | Splinter Review
2.91 KB, patch
baku
: review+
Details | Diff | Splinter Review
8.97 KB, patch
baku
: review+
Details | Diff | Splinter Review
21.69 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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."
(Assignee)

Updated

4 years ago
Assignee: nobody → birunthan
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
Attachment #8622798 - Flags: review?(wchen)
(Assignee)

Comment 4

4 years ago
Attachment #8622799 - Flags: review?(wchen)
(Assignee)

Comment 5

4 years ago
Attachment #8622800 - Flags: review?(wchen)
Keywords: dev-doc-needed
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
(Assignee)

Comment 7

4 years ago
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.
Attachment #8622796 - Flags: review?(wchen) → review?(amarchesini)
(Assignee)

Updated

4 years ago
Attachment #8622797 - Flags: review?(wchen) → review?(amarchesini)
(Assignee)

Updated

4 years ago
Attachment #8622798 - Flags: review?(wchen) → review?(amarchesini)
(Assignee)

Updated

4 years ago
Attachment #8622799 - Flags: review?(wchen) → review?(amarchesini)
(Assignee)

Updated

4 years ago
Attachment #8622800 - Flags: review?(wchen) → review?(amarchesini)
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
Attachment #8622796 - Flags: review?(amarchesini) → review+
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?
Attachment #8622797 - Flags: review?(amarchesini) → review+
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.
Attachment #8622798 - Flags: review?(amarchesini) → review-
Attachment #8622799 - Flags: review?(amarchesini) → review+
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>
Attachment #8622800 - Flags: review?(amarchesini) → review+
(Assignee)

Updated

4 years ago
Attachment #8622798 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
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
Attachment #8632156 - Flags: review?(amarchesini)
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();
}

...
Attachment #8632156 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 15

4 years ago
This changes the Permissions.query parameter to an object rather than a PermissionDescriptor in order to construct a PushPermissionDescriptor in the next patch.
Attachment #8635553 - Flags: review?(amarchesini)
(Assignee)

Updated

4 years ago
Attachment #8632156 - Attachment is obsolete: true
(Assignee)

Comment 16

4 years ago
Attachment #8635555 - Flags: review?(amarchesini)
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?
Attachment #8635553 - Flags: review?(amarchesini) → review-
Attachment #8635555 - Flags: review?(amarchesini)
(Assignee)

Comment 18

4 years ago
(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?
Flags: needinfo?(amarchesini)
(Assignee)

Comment 19

4 years ago
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.
Attachment #8635553 - Attachment is obsolete: true
Attachment #8639394 - Flags: review?(amarchesini)
(Assignee)

Comment 20

4 years ago
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.
Attachment #8635555 - Flags: review?(amarchesini)
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);
Attachment #8635555 - Flags: review?(amarchesini) → review+
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.
Attachment #8639394 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 23

4 years ago
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.
Attachment #8635555 - Attachment is obsolete: true
Attachment #8639394 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Attachment #8639535 - Flags: review?(amarchesini)
(Assignee)

Comment 24

4 years ago
This adds tests for push.
Attachment #8639536 - Flags: review?(amarchesini)
(Assignee)

Updated

4 years ago
Attachment #8622800 - Attachment is obsolete: true
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
Attachment #8639535 - Flags: review?(amarchesini) → review+
Attachment #8639536 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 30

4 years ago
This is in prepartion of a subsequent change where we will update the state
within PermissionStatus.
Attachment #8641326 - Flags: review?(amarchesini)
(Assignee)

Updated

4 years ago
Attachment #8641341 - Attachment is obsolete: true
Attachment #8641341 - Flags: review?(amarchesini)
Attachment #8641320 - Flags: review?(amarchesini) → review+
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.
Attachment #8641321 - Flags: review?(amarchesini) → review+
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.
Attachment #8641326 - Flags: review?(amarchesini) → review-
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.
Attachment #8641353 - Flags: review?(amarchesini) → review-
(Assignee)

Comment 36

4 years ago
This is in prepartion of a subsequent change where we will update the state
within PermissionStatus.
Attachment #8641730 - Flags: review?(amarchesini)
(Assignee)

Updated

4 years ago
Attachment #8641326 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8641353 - Attachment is obsolete: true
(Assignee)

Comment 39

4 years ago
I forgot to do this in Part 1.
Attachment #8641906 - Flags: review?(amarchesini)
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.
Attachment #8641730 - Flags: review?(amarchesini) → review+
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?
Attachment #8641895 - Flags: review?(amarchesini) → review-
Attachment #8641900 - Flags: review?(amarchesini) → review+
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);
Attachment #8641906 - Flags: review?(amarchesini)
(Assignee)

Comment 43

4 years ago
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.
Attachment #8641906 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8641895 - Attachment is obsolete: true
Attachment #8645734 - Flags: review?(amarchesini) → review+
(Assignee)

Updated

4 years ago
Blocks: 1193373
(Assignee)

Updated

4 years ago
Blocks: 1193376
(Assignee)

Comment 48

4 years ago
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.
Attachment #8641906 - Attachment is obsolete: false
Attachment #8641906 - Flags: review?(amarchesini)
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'.
Attachment #8641906 - Flags: review?(amarchesini) → review+

Comment 50

4 years ago
Please send an intent to implement/ship for this.  Thanks!
(Assignee)

Comment 51

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

Here you go.
(Assignee)

Comment 53

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

Comment 54

4 years ago
(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
(Assignee)

Updated

4 years ago
Blocks: 1197461
(Assignee)

Comment 57

4 years ago
(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!
(Assignee)

Updated

3 years ago
Blocks: 1221104
(Assignee)

Updated

3 years ago
Blocks: 1221106
(Assignee)

Comment 59

3 years ago
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.
Depends on: 1261405
Depends on: 1278831
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.