-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Web Automation: implement Set Storage Access endpoint #49692
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
Web Automation: implement Set Storage Access endpoint #49692
Conversation
EWS run on previous version of this PR (hash e7a9ddc) |
continue; | ||
|
||
bool granted = state == Inspector::Protocol::BidiPermissions::PermissionState::Granted; | ||
page->protectedWebsiteDataStore()->setStorageAccessPermissionForTesting(granted, page->identifier(), topFrameOrigin.string(), (embeddedOriginIsWildcard ? topFrameOrigin : embeddedOrigin).string(), [aggregator] () { }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
page->protectedWebsiteDataStore()->setStorageAccessPermissionForTesting(granted, page->identifier(), topFrameOrigin.string(), (embeddedOriginIsWildcard ? topFrameOrigin : embeddedOrigin).string(), [aggregator] () { }); | |
page->protectedWebsiteDataStore()->setStorageAccessPermissionForTesting(granted, page->identifier(), topFrameOrigin.string(), (embeddedOriginIsWildcard ? topFrameOrigin : embeddedOrigin).string(), [aggregator]() { }); |
bool embeddedOriginIsWildcard = subFrameURLString == "*"_s; | ||
auto embeddedOrigin = RegistrableDomain { URL { subFrameURLString } }; | ||
|
||
auto aggregator = WTF::CallbackAggregator::create([callback = WTFMove(callback)]() mutable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should either this lambda or the one below also capture a protectedThis = Ref { *this }
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote that before, but I removed it because the code doesn't actually use this
in the callback. We have captured a ref to backendDispatcher
in the generated lambda glue code that calls into the agent method.
m_agent->setPermission(in_descriptor.releaseNonNull(), in_origin, *in_state, in_opt_userContext, [backendDispatcher = m_backendDispatcher.copyRef(),
...
So it shouldn't be necessary to protect this
, as BackendDispatcher and FrontendRounter will gracefully stop sending responses when there's no more frontends connected. In the WebDriver BiDi case, the BidiProcessor owns both frontend and backend dispatcher. So, if a callback outlasts the processor itself, sendResponse will dead end at the frontend router that doesn't have any connections.
I did a deep dive on the refcounting situation and i don't see the need to protect the agent as its owned by the processor. Plus, in the WebDriver BiDi case, the processor is used as a fake "connection" so that it can forward events and command responses through to WebAutomationSession::sendBidiMessage
.
e7a9ddc
to
10c0b6f
Compare
EWS run on previous version of this PR (hash 10c0b6f) |
"description": "Simulates user modification of a PermissionDescriptor's permission state.", | ||
"spec": "https://www.w3.org/TR/permissions/#webdriver-bidi-command-permissions-setPermission", | ||
"wpt": "https://github.com/web-platform-tests/wpt/tree/master/webdriver/tests/bidi/permissions/set", | ||
"async": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: async usually at the bottom
"name": "setPermission", | ||
"description": "Simulates user modification of a PermissionDescriptor's permission state.", | ||
"spec": "https://www.w3.org/TR/permissions/#webdriver-bidi-command-permissions-setPermission", | ||
"wpt": "https://github.com/web-platform-tests/wpt/tree/master/webdriver/tests/bidi/permissions/set", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link seems buggy ? I don't see a permissions bidi suite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, this is the correct link:
https://github.com/web-platform-tests/wpt/tree/master/webdriver/tests/bidi/external/permissions/set_permission
it's in a separate external
directory because it's defined externally from the main bidi spec.
Please also update the title to indicate this includes WebDriver Classic and the set storage access command. |
10c0b6f
to
4230afb
Compare
EWS run on previous version of this PR (hash 4230afb) |
This change contains multiple commits which are not squashed together, blocking PR #49692. Please squash the commits to land. |
4230afb
to
a0fdd11
Compare
EWS run on current version of this PR (hash a0fdd11) |
https://bugs.webkit.org/show_bug.cgi?id=297368 rdar://158263193 Reviewed by Charlie Wolfe. Add new endpoints for setting storage access permission state and granting storage access to embedded frames for specific origins. These methods will be called via Automation protocol to implement the WebDriver Classic endpoints in safaridriver. In a future patch, the WebAutomationSession methods will be used by the WebDriver BiDi permissions agent. * Source/WebKit/UIProcess/Automation/Automation.json: * Source/WebKit/UIProcess/Automation/WebAutomationSession.h: * Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp: (WebKit::WebAutomationSession::setStorageAccessPermissionState): (WebKit::WebAutomationSession::grantStorageAccess): [WebDriver BiDi] add support for permissions.setPermission command https://bugs.webkit.org/show_bug.cgi?id=265621 rdar://119346759 Reviewed by Charlie Wolfe. Implement the remote end steps for Set Permission. * Source/WebKit/CMakeLists.txt: * Source/WebKit/DerivedSources-input.xcfilelist: * Source/WebKit/DerivedSources.make: * Source/WebKit/Sources.txt: * Source/WebKit/UIProcess/Automation/BidiPermissionsAgent.cpp: Added. (WebKit::BidiPermissionsAgent::BidiPermissionsAgent): (WebKit::allPageProxiesFor): (WebKit::BidiPermissionsAgent::setPermission): Added. Defer to WebSiteDataStore and NetworkProcess for the actual implementation. * Source/WebKit/UIProcess/Automation/BidiPermissionsAgent.h: Added. * Source/WebKit/UIProcess/Automation/WebDriverBidiProcessor.cpp: (WebKit::WebDriverBidiProcessor::WebDriverBidiProcessor): Add BidiPermissionAgent. * Source/WebKit/UIProcess/Automation/WebDriverBidiProcessor.h: * Source/WebKit/UIProcess/Automation/protocol/BidiPermissions.json: Added. * Source/WebKit/WebKit.xcodeproj/project.pbxproj: Add new domain and associated derived sources and project entries. Canonical link: https://commits.webkit.org/299547@main
a0fdd11
to
19e1927
Compare
Committed 299547@main (19e1927): https://commits.webkit.org/299547@main Reviewed commits have been landed. Closing PR #49692 and removing active labels. |
19e1927
a0fdd11
🧪 win-tests