Begin using C++ coroutines for WebPageProxy::decidePolicyForNavigationAction #23021
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
achristensen07
added
the
WebKit Process Model
|
EWS run on previous version of this PR (hash 56710cd) |
webkit-ews-buildbot
added
the
merging-blocked
achristensen07
removed
the
merging-blocked
achristensen07
force-pushed
the
eng/Begin-using-C-coroutines-for-WebPageProxydecidePolicyForNavigationAction
branch
from
56710cd
to
2e44d1a
Compare
|
EWS run on previous version of this PR (hash 2e44d1a) |
webkit-ews-buildbot
added
the
merging-blocked
achristensen07
removed
the
merging-blocked
achristensen07
force-pushed
the
eng/Begin-using-C-coroutines-for-WebPageProxydecidePolicyForNavigationAction
branch
from
2e44d1a
to
abec41a
Compare
|
EWS run on previous version of this PR (hash abec41a) |
webkit-ews-buildbot
added
the
merging-blocked
achristensen07
removed
the
merging-blocked
achristensen07
force-pushed
the
eng/Begin-using-C-coroutines-for-WebPageProxydecidePolicyForNavigationAction
branch
from
abec41a
to
09ea55c
Compare
|
EWS run on previous version of this PR (hash 09ea55c) |
aprotyas
reviewed
Jan 23, 2024
webkit-ews-buildbot
added
the
merging-blocked
achristensen07
removed
the
merging-blocked
achristensen07
force-pushed
the
eng/Begin-using-C-coroutines-for-WebPageProxydecidePolicyForNavigationAction
branch
from
09ea55c
to
ed3d5ee
Compare
|
EWS run on previous version of this PR (hash ed3d5ee)
|
🧪 ios-wk2-wpt
🧪 api-ios
webkit-ews-buildbot
added
the
merging-blocked
aprotyas
reviewed
May 17, 2024
achristensen07
removed
the
merging-blocked
achristensen07
force-pushed
the
eng/Begin-using-C-coroutines-for-WebPageProxydecidePolicyForNavigationAction
branch
from
ed3d5ee
to
4798912
Compare
|
EWS run on previous version of this PR (hash 4798912)
|
achristensen07
force-pushed
the
eng/Begin-using-C-coroutines-for-WebPageProxydecidePolicyForNavigationAction
branch
from
4798912
to
1a8f831
Compare
|
EWS run on previous version of this PR (hash 1a8f831) |
achristensen07
force-pushed
the
eng/Begin-using-C-coroutines-for-WebPageProxydecidePolicyForNavigationAction
branch
from
1a8f831
to
3c93b0e
Compare
|
EWS run on previous version of this PR (hash 3c93b0e) |
webkit-ews-buildbot
added
the
merging-blocked
aprotyas
approved these changes
May 18, 2024
| @@ -7074,6 +7088,15 @@ void WebPageProxy::decidePolicyForNavigationAction(Ref<WebProcessProxy>&& proces | |||
| sendCachedLinkDecorationFilteringData(); | |||
| #endif | |||
|
|
|||
| transaction = std::nullopt; | |||
There was a problem hiding this comment.
This is clearly not ratified in the style guide, but I think we're leaning towards { } instead of std::nullopt nowadays.
There was a problem hiding this comment.
That's definitely debatable. 832 vs 1362 instances of each.
Comment on lines
30
to
40
| #if __has_include(<coroutine>) | ||
| #include <coroutine> | ||
| #else | ||
| #include <experimental/coroutine> | ||
| namespace std { | ||
| using std::experimental::coroutine_handle; | ||
| using std::experimental::suspend_never; | ||
| using std::experimental::suspend_always; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Is <coroutine> not part of all toolchains we build against? Is this for macOS downlevels or for the ports using GCC?
There was a problem hiding this comment.
In either case, maybe add a FIXME to simplify this when it's unnecessary?
There was a problem hiding this comment.
This is needed for some old macOS still. Something similar is done in HTTPServer.h. We can remove it at some point, you're right.
achristensen07
removed
the
merging-blocked
achristensen07
force-pushed
the
eng/Begin-using-C-coroutines-for-WebPageProxydecidePolicyForNavigationAction
branch
from
3c93b0e
to
46de710
Compare
|
EWS run on current version of this PR (hash 46de710) |
webkit-ews-buildbot
added
the
merging-blocked
achristensen07
added
unsafe-merge-queue
…nAction https://bugs.webkit.org/show_bug.cgi?id=267827 Reviewed by Abrar Rahman Protyasha. WebKit has a long history with C++ and asynchrony. Deciding whether to proceed with a navigation has always had the ability to be done asynchronously. Before C++11, we made our own object that contained state and context for a call to indicate continuation. With https://commits.webkit.org/193766@main we finished the transition from this object, WebCore::PolicyCallback, to C++ lambdas. Since then, a large and growing amount of our code has developed the ability to do things asynchronously using WTF::CompletionHandler, a std::function-like object that contains state and context to use when continuing. Also since then, C++ has added coroutine support in C++20, including the co_await keyword. This has the potential to allow us to more elegantly write code that does many things asynchronously. WebPageProxy::decidePolicyForNavigationAction has a growing amount of complexity, with deeply nested lambdas and many functions calling another function to continue the logic, trying to break up the logic. With site isolation, I've needed to add many things to this already complex area of the code, and more are needed still. I've had difficulty passing parameters from one end of the flow to the other, through the many nested lambdas. This has led me to do introduce things like ProvisionalPageProxy's needsCookieAccessAddedInNetworkProcess, where I set a bool early in the flow and query the bool much later in the flow. This needs a better solution. In order to begin using C++ coroutines, we need a way to get into a coroutine from a function with a CompletionHandler parameter, and we also need a way to get from a coroutine to a function with a CompletionHandler parameter and await its result. For these two operations, I introduce CoroutineCaller and AwaitableTaskWithCompletionHandler. Task is the object that an asynchronous coroutine returns. Lazy<T> is what a coroutine returns for its resulting T to be awaitable, analogous to a CompletionHandler<void(T)> which is called with the resulting T. These primitives are used to make WebPageProxy::decidePolicyForNavigationAction an asynchronous coroutine. The introduction of this new technology in such a small scope makes it look like most of the code is just the CoroutineCaller/AwaitableTaskWithCompletionHandler borders to call to and from existing code, but as coroutine adoption increases we will see simpler and simpler code, where we can easily and elegantly add new steps in the logic flow by just awaiting another asynchronous step or calling a synchronous step directly. * Source/WebKit/Platform/CoroutineUtilities.h: Added. (WebKit::CoroutineHandle::CoroutineHandle): (WebKit::CoroutineHandle::operator=): (WebKit::CoroutineHandle::~CoroutineHandle): (WebKit::CoroutineHandle::handle const): (WebKit::Lazy::PromiseBase::initial_suspend): (WebKit::Lazy::PromiseBase::unhandled_exception): (WebKit::Lazy::PromiseBase::setHandle): (WebKit::Lazy::PromiseBase::handle): (WebKit::Lazy::Promise<void>::get_return_object): (WebKit::Lazy::Promise<void>::return_void): (WebKit::Lazy::Promise<void>::result): (WebKit::Lazy::Awaitable::Awaitable): (WebKit::Lazy::Awaitable::await_ready const): (WebKit::Lazy::Awaitable::await_suspend): (WebKit::Lazy::Awaitable::await_resume): (WebKit::Lazy::Lazy): (WebKit::Lazy::operator co_await const): (WebKit::Task::promise_type::get_return_object): (WebKit::Task::promise_type::initial_suspend): (WebKit::Task::promise_type::unhandled_exception): (WebKit::Task::promise_type::return_void): (WebKit::CoroutineCaller::setCoroutine): (WebKit::callCoroutine): (WebKit::AwaitableTaskWithCompletionHandler::AwaitableTaskWithCompletionHandler): (WebKit::AwaitableTaskWithCompletionHandler::await_ready): (WebKit::AwaitableTaskWithCompletionHandler::await_suspend): (WebKit::AwaitableTaskWithCompletionHandler::await_resume): * Source/WebKit/Platform/IPC/Connection.h: * Source/WebKit/UIProcess/WebPageProxy.cpp: (WebKit::WebPageProxy::decidePolicyForNavigationAction): (WebKit::WebPageProxy::awaitableDecidePolicyForNavigationAction): (WebKit::WebPageProxy::continueDecidePolicyForNavigationAction): * Source/WebKit/UIProcess/WebPageProxy.h: * Source/WebKit/WebKit.xcodeproj/project.pbxproj: Canonical link: https://commits.webkit.org/278995@main
webkit-commit-queue
force-pushed
the
eng/Begin-using-C-coroutines-for-WebPageProxydecidePolicyForNavigationAction
branch
from
46de710
to
e90f899
Compare
|
Committed 278995@main (e90f899): https://commits.webkit.org/278995@main Reviewed commits have been landed. Closing PR #23021 and removing active labels. |
webkit-commit-queue
removed
the
unsafe-merge-queue
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
e90f899
46de710