The default bug view has changed. See this FAQ.

Implement support for Permissions API

RESOLVED FIXED in Firefox 43

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: marcosc, Assigned: poiru)

Tracking

(Blocks: 2 bugs, {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

2 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."
(Reporter)

Updated

2 years ago
(Assignee)

Updated

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

Comment 1

2 years ago
Created attachment 8622796 [details] [diff] [review]
Part 1: Add stub PermissionStatus implementation
Attachment #8622796 - Flags: review?(wchen)
(Assignee)

Comment 2

2 years ago
Created attachment 8622797 [details] [diff] [review]
Part 2: Add stub Permissions implementation
Attachment #8622797 - Flags: review?(wchen)
(Assignee)

Comment 3

2 years ago
Created attachment 8622798 [details] [diff] [review]
Part 3: Implement Permissions.query
Attachment #8622798 - Flags: review?(wchen)
(Assignee)

Comment 4

2 years ago
Created attachment 8622799 [details] [diff] [review]
Part 4: Add Navigator.permissions
Attachment #8622799 - Flags: review?(wchen)
(Assignee)

Comment 5

2 years ago
Created attachment 8622800 [details] [diff] [review]
Part 5: Add test for Permissions.query
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

2 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

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

Updated

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

Updated

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

Updated

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

Comment 12

2 years ago
Created attachment 8632156 [details] [diff] [review]
Part 3: Implement Permissions.query
(Assignee)

Updated

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

Comment 13

2 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

2 years ago
Created attachment 8635553 [details] [diff] [review]
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.
Attachment #8635553 - Flags: review?(amarchesini)
(Assignee)

Updated

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

Comment 16

2 years ago
Created attachment 8635555 [details] [diff] [review]
Part 3.1: Add support for 'push' to Permissions.query
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

2 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

2 years ago
Created attachment 8639394 [details] [diff] [review]
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.
Attachment #8635553 - Attachment is obsolete: true
Attachment #8639394 - Flags: review?(amarchesini)
(Assignee)

Comment 20

2 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

2 years ago
Created attachment 8639535 [details] [diff] [review]
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.
Attachment #8635555 - Attachment is obsolete: true
Attachment #8639394 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Attachment #8639535 - Flags: review?(amarchesini)
(Assignee)

Comment 24

2 years ago
Created attachment 8639536 [details] [diff] [review]
Part 5: Add test for Permissions.query

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

Updated

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

2 years ago
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
Keywords: leave-open
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
Flags: in-testsuite+
(Assignee)

Comment 28

2 years ago
Created attachment 8641320 [details] [diff] [review]
Part 6: Add PermissionUtils.h for helper functions
Attachment #8641320 - Flags: review?(amarchesini)
(Assignee)

Comment 29

2 years ago
Created attachment 8641321 [details] [diff] [review]
Part 7: Add helpers to convert between PermissionName and permission type
Attachment #8641321 - Flags: review?(amarchesini)
(Assignee)

Comment 30

2 years ago
Created attachment 8641326 [details] [diff] [review]
Part 8: Move permission checking into PermissionStatus

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

Comment 31

2 years ago
Created attachment 8641341 [details] [diff] [review]
Part 9: Add PermissionObserver to watch for perm-changed notifications
Attachment #8641341 - Flags: review?(amarchesini)
(Assignee)

Comment 32

2 years ago
Created attachment 8641353 [details] [diff] [review]
Part 9: Add PermissionObserver to watch for perm-changed notifications
Attachment #8641353 - Flags: review?(amarchesini)
(Assignee)

Updated

2 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

2 years ago
Created attachment 8641730 [details] [diff] [review]
Part 8: Move permission checking into PermissionStatus

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

Updated

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

Comment 37

2 years ago
Created attachment 8641895 [details] [diff] [review]
Part 9: Add PermissionObserver to watch for perm-changed notifications
Attachment #8641895 - Flags: review?(amarchesini)
(Assignee)

Updated

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

Comment 38

2 years ago
Created attachment 8641900 [details] [diff] [review]
Part 10: Fire change event for PermissionStatus objects
Attachment #8641900 - Flags: review?(amarchesini)
(Assignee)

Comment 39

2 years ago
Created attachment 8641906 [details] [diff] [review]
Part 11: Use NS_DECL_ISUPPORTS_INHERITED with PermissionStatus

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

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

Comment 44

2 years ago
Created attachment 8645734 [details] [diff] [review]
Part 9: Add PermissionObserver to watch for perm-changed notifications
Attachment #8645734 - Flags: review?(amarchesini)
(Assignee)

Updated

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

Comment 45

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

Updated

2 years ago
Blocks: 1193373
(Assignee)

Updated

2 years ago
Blocks: 1193376

Comment 46

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

Comment 48

2 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+
Please send an intent to implement/ship for this.  Thanks!
(Assignee)

Comment 51

2 years ago
Created attachment 8648265 [details] [diff] [review]
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

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

Comment 53

2 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

2 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

2 years ago
Blocks: 1197461
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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.
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 57

2 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

a year ago
Blocks: 1221104
(Assignee)

Updated

a year ago
Blocks: 1221106
(Assignee)

Comment 59

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