Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add name attribute to PermissionStatus #248

Merged
merged 1 commit into from Aug 11, 2021
Merged

Conversation

marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented Jun 24, 2021

closes #237

Avoids having to rely on array order to know what a particular permission status belongs to.

The following tasks have been completed:

Implementation commitment:


Preview | Diff

@marcoscaceres
Copy link
Member Author

@annevk or @martinthomson, if you agree, I'm happy to add to Gecko.

@jyasskin or other Chrome folks, wdyt?

Any objection from anyone else?

@annevk
Copy link
Member

annevk commented Jun 24, 2021

I'm not sure I understand the motivation. A singular PermissionStatus instance is returned by query() when invoked for a particular name. It seems that in any code you might write the name is already in scope. Can you give an example where this would help?

@marcoscaceres
Copy link
Member Author

Yes, sorry, see the example in #237

@annevk
Copy link
Member

annevk commented Jun 24, 2021

Ah, thanks! Yeah, that seems reasonable to me.

cc @johannhof

@tomayac
Copy link
Contributor

tomayac commented Jun 24, 2021

It looks like the aim could be to future-proof the API for the case where one day query() could allow for multiple permissions to be queried. Right now, the recommendation is to use Promise.all() for this, which seems like a not too far off idea.

// Now
Promise.all([
  navigator.permissions.query({ name: "geolocation" }),
  navigator.permissions.query({ name: "notifications" })
])
.then(([{ state: geoState }, { state: notifState }]) => {
  console.log("Geolocation permission state is:", geoState);
  console.log("Notifications permission state is:", notifState);
});
// Potential future
const states = await navigator.permissions.query({
  name: "geolocation",
  name: "notifications",
});

@johannhof
Copy link
Member

Yeah, I don't see a huge benefit to that alternative API but also no obvious downsides to the name attribute, so I think we'd take a patch for adding it to Gecko.

@marcoscaceres
Copy link
Member Author

filed Gecko bug https://bugzilla.mozilla.org/show_bug.cgi?id=1718021 ... sent patch.

@marcoscaceres
Copy link
Member Author

@jyasskin
Copy link
Member

jyasskin commented Aug 5, 2021

I think this is a good idea.

@marcoscaceres
Copy link
Member Author

Ok, I'm going to go ahead and merge this as it's not particularly controversial. If no one picks it up in a few days on the Chrome side, I'll send a patch for it as it's a trivial thing to add.

@marcoscaceres marcoscaceres merged commit 9c6fdc2 into main Aug 11, 2021
@marcoscaceres marcoscaceres deleted the PermissionStatus-name branch August 11, 2021 05:10
github-actions bot added a commit that referenced this pull request Aug 11, 2021
SHA: 9c6fdc2
Reason: push, by @marcoscaceres

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 11, 2021
Spec change w3c/permissions#248

Differential Revision: https://phabricator.services.mozilla.com/D118692

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1718021
gecko-commit: d47214bb80b452b84b9685a7f8c21f2aee417cf6
gecko-reviewers: johannh, peterv
@marcoscaceres
Copy link
Member Author

Chrome patch: https://chromium-review.googlesource.com/c/chromium/src/+/3089276

Added a WPT via the Gecko patch (if it sticks 🤞) web-platform-tests/wpt#29999

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Aug 12, 2021
marcoscaceres added a commit to web-platform-tests/wpt that referenced this pull request Aug 12, 2021
Spec change w3c/permissions#248

Differential Revision: https://phabricator.services.mozilla.com/D118692

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1718021
gecko-commit: d47214bb80b452b84b9685a7f8c21f2aee417cf6
gecko-reviewers: johannh, peterv

Co-authored-by: Marcos Cáceres <marcos@marcosc.com>
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Aug 20, 2021
pull bot pushed a commit to luojiguicai/chromium that referenced this pull request Oct 13, 2021
Adds a readonly "name" attribute to PermissionStatus as per spec change:
w3c/permissions#248

To match Gecko's implementation:
https://bugzilla.mozilla.org/show_bug.cgi?id=1718021

Also adds PermissionNameToString() to permissions_utils, that
converts from from an enum value name to a string.

Bug: 1236856
Change-Id: I844a160dcc426fa5b72fb24f2859d15a0f2ac294

Intent to Ship:

https: //groups.google.com/a/chromium.org/g/blink-dev/c/LPvBoisFnAA
Change-Id: I844a160dcc426fa5b72fb24f2859d15a0f2ac294
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3089276
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Auto-Submit: Marcos Caceres <marcos@marcosc.com>
Reviewed-by: Yoav Weiss <yoavweiss@chromium.org>
Reviewed-by: Kamila Hasanbega <hkamila@chromium.org>
Reviewed-by: Kamila Hasanbega <hkamila@google.com>
Cr-Commit-Position: refs/heads/main@{#930753}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
Adds a readonly "name" attribute to PermissionStatus as per spec change:
w3c/permissions#248

To match Gecko's implementation:
https://bugzilla.mozilla.org/show_bug.cgi?id=1718021

Also adds PermissionNameToString() to permissions_utils, that
converts from from an enum value name to a string.

Bug: 1236856
Change-Id: I844a160dcc426fa5b72fb24f2859d15a0f2ac294

Intent to Ship:

https: //groups.google.com/a/chromium.org/g/blink-dev/c/LPvBoisFnAA
Change-Id: I844a160dcc426fa5b72fb24f2859d15a0f2ac294
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3089276
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Auto-Submit: Marcos Caceres <marcos@marcosc.com>
Reviewed-by: Yoav Weiss <yoavweiss@chromium.org>
Reviewed-by: Kamila Hasanbega <hkamila@chromium.org>
Reviewed-by: Kamila Hasanbega <hkamila@google.com>
Cr-Commit-Position: refs/heads/main@{#930753}
NOKEYCHECK=True
GitOrigin-RevId: 21467eb74fcec04a52322c4f605b1c63c5ced7be
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PermissionStatus should have a name
5 participants