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.

Sign up for GitHub

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

Jump to bottom

WebPermissionControllerProxy::Query message reply should go to correct completion handler #5984

Merged
merged 1 commit into from
Nov 3, 2022
Merged

WebPermissionControllerProxy::Query message reply should go to correct completion handler #5984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 1 addition & 21 deletions Source/WebKit/WebProcess/WebCoreSupport/WebPermissionController.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,27 +65,7 @@ void WebPermissionController::query(WebCore::ClientOrigin&& origin, WebCore::Per
proxyIdentifier = WebPage::fromCorePage(*page).webPageProxyIdentifier();
}

m_requests.append(PermissionRequest { WTFMove(origin), descriptor, proxyIdentifier, source, WTFMove(completionHandler) });
tryProcessingRequests();
}

void WebPermissionController::tryProcessingRequests()
{
while (!m_requests.isEmpty()) {
auto& currentRequest = m_requests.first();
if (currentRequest.isWaitingForReply)
return;

currentRequest.isWaitingForReply = true;
WebProcess::singleton().sendWithAsyncReply(Messages::WebPermissionControllerProxy::Query(currentRequest.origin, currentRequest.descriptor, currentRequest.identifier, currentRequest.source), [this, weakThis = WeakPtr { *this }](auto state) {
RefPtr protectedThis { weakThis.get() };
if (!protectedThis)
return;

m_requests.takeFirst().completionHandler(state);
tryProcessingRequests();
Copy link
Contributor

Choose a reason for hiding this comment

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

Will tryProcessingRequests() become dead code now? What did it do previously?

Copy link
Contributor

Choose a reason for hiding this comment

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

In existing implementation, tryProcessingRequests just sends one async message for the first request in queue. If the first request has already sent message and no reply, it will return. That means one unhandled request will block the other requests in queue.
When the reply is received, the first request will be taken out of queue and tryProcessingRequests` is called to proceed next message.

We had this design because WebPermissionController used to cache permission result, which means tryProcessingRequests might handle multiple requests at a time (if the result is in the cache), and receiving one message reply (caching the result) may benefit the subsequent requests.

But we removed the cache and send message for each request now, so we can send them when request is received, and finish CompletionHandler in message reply (as in this patch). And there is not need to store the requests in m_requests.

});
}
WebProcess::singleton().sendWithAsyncReply(Messages::WebPermissionControllerProxy::Query(origin, descriptor, proxyIdentifier, source), WTFMove(completionHandler));
}

void WebPermissionController::addObserver(WebCore::PermissionObserver& observer)
Expand Down
14 changes: 0 additions & 14 deletions Source/WebKit/WebProcess/WebCoreSupport/WebPermissionController.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,21 +61,7 @@ class WebPermissionController final : public WebCore::PermissionController, publ
void removeObserver(WebCore::PermissionObserver&) final;
void permissionChanged(WebCore::PermissionName, const WebCore::SecurityOriginData&) final;

void tryProcessingRequests();

WeakHashSet<WebCore::PermissionObserver> m_observers;

using PermissionEntry = std::pair<WebCore::PermissionDescriptor, WebCore::PermissionState>;

struct PermissionRequest {
WebCore::ClientOrigin origin;
WebCore::PermissionDescriptor descriptor;
std::optional<WebPageProxyIdentifier> identifier;
WebCore::PermissionQuerySource source;
CompletionHandler<void(std::optional<WebCore::PermissionState>)> completionHandler;
bool isWaitingForReply { false };
};
Deque<PermissionRequest> m_requests;
};

} // namespace WebCore