Skip to content

Remove oneRealm arg from set_permission#37152

Merged
gsnedders merged 1 commit intomasterfrom
oneRealm
Dec 2, 2022
Merged

Remove oneRealm arg from set_permission#37152
gsnedders merged 1 commit intomasterfrom
oneRealm

Conversation

@marcoscaceres
Copy link
Copy Markdown
Contributor

@marcoscaceres marcoscaceres commented Nov 25, 2022

The use of realm and "oneRealm" are being removed from the Permission spec.
w3c/permissions#397

And related discussion:
w3c/permissions#387

marcoscaceres
marcoscaceres commented Nov 25, 2022
View reviewed changes
rakuco
rakuco reviewed Nov 28, 2022
View reviewed changes
reillyeon
reillyeon approved these changes Nov 28, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@reillyeon reillyeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test changes that I would probably be considered an owner of look good to me. Didn't look at any of the automation infrastructure changes.

rakuco
rakuco approved these changes Nov 29, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@rakuco rakuco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

The Chromium failure in the checks looks unrelated; I'd try pushing again to see if it goes away, otherwise someone with enough power will have to force-merge this PR.

@marcoscaceres
Copy link
Copy Markdown
Contributor Author

marcoscaceres commented Dec 1, 2022

@rakuco indeed.

@jgraham maybe you can override it?

@reillyeon, as spec editor, any chance you could check if idle-detection/idle-detection-allowed-by-permissions-policy.https.sub.html is indeed flaky? It does to a lot of cross origin stuff, so it certainly has the potential to be flaky.

@wpt-pr-bot wpt-pr-bot removed the request for review from alvestrand December 1, 2022 22:30
annevk
annevk reviewed Dec 2, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed this was already being reviewed by @rakuco. Anyway, couple small drive-by nits.

infrastructure/testdriver/set_permission.https.html
@@ -1,17 +1,23 @@
<!DOCTYPE html>
<meta charset="utf-8">
<meta charset="utf-8" />
Copy link
Copy Markdown
Member

@annevk annevk Dec 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unrelated?

Copy link
Copy Markdown
Contributor Author

@marcoscaceres marcoscaceres Dec 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, ran prettier over it… it adds the “ />” garbage.

infrastructure/testdriver/set_permission.https.html
await test_driver.set_permission(descriptor, "denied");
permission = await navigator.permissions.query(descriptor);
assert_equals(permission.state, "denied");
}, "Deny Permission");
Copy link
Copy Markdown
Member

@annevk annevk Dec 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we really be changing this entire test as part of this?

Copy link
Copy Markdown
Contributor Author

@marcoscaceres marcoscaceres Dec 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, yes. It doesn’t do any harm to test that setting the permission worked.

@whimboo whimboo mentioned this pull request Dec 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@whimboo whimboo whimboo left review comments

@annevk annevk annevk left review comments

@rakuco rakuco rakuco approved these changes

@reillyeon reillyeon reillyeon approved these changes

@birtles birtles Awaiting requested review from birtles

@dbaron dbaron Awaiting requested review from dbaron

@dirkschulze dirkschulze Awaiting requested review from dirkschulze

@domenic domenic Awaiting requested review from domenic

@fantasai fantasai Awaiting requested review from fantasai

@FremyCompany FremyCompany Awaiting requested review from FremyCompany

@frivoal frivoal Awaiting requested review from frivoal

@GPHemsley GPHemsley Awaiting requested review from GPHemsley

@graouts graouts Awaiting requested review from graouts

@guidou guidou Awaiting requested review from guidou

@henbos henbos Awaiting requested review from henbos

@hoch hoch Awaiting requested review from hoch

@jdm jdm Awaiting requested review from jdm

@mikewest mikewest Awaiting requested review from mikewest

Assignees

@kenchris kenchris

Labels

compute-pressure css-anchor-position css-contain css-images css-masking css-multicol css-pseudo css-values docs feature-policy generic-sensor geolocation-API html idle-detection infra interfaces mediacapture-image mediacapture-streams permissions-policy pointerevents screen-details scroll-animations speculation-rules testdriver.js url web-nfc webaudio webdriver webrtc wg-css wg-das wg-geolocation wg-s_audio wg-s_pointer-events wg-s_webappsec wg-s_webrtc wg-testing_browser wptrunner The automated test runner, commonly called through ./wpt run

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@marcoscaceres @rakuco @whimboo @reillyeon @annevk @gsnedders @kenchris @wpt-pr-bot @moz-wptsync-bot