Skip to content

Commit

Permalink
Change behavior of webNavigation.getFrame/getAllFrames to better matc…
Browse files Browse the repository at this point in the history 
…h spec

https://bugs.webkit.org/show_bug.cgi?id=266305
rdar://118340990

Reviewed by Timothy Hatcher.

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/webNavigation/getFrame states:
"If the specified tab or frame ID could not be found, or some other error occurs, the promise will be rejected with an error message."

Before this change, we weren't doing that - we were just vending an empty object.

This PR also adds a couple tests for this behavior.

* Source/WebKit/UIProcess/Extensions/Cocoa/API/WebExtensionContextAPIWebNavigationCocoa.mm:
(WebKit::WebExtensionContext::webNavigationGetFrame):
(WebKit::WebExtensionContext::webNavigationGetAllFrames):
* Source/WebKit/UIProcess/Extensions/WebExtensionContext.h:
* Source/WebKit/UIProcess/Extensions/WebExtensionContext.messages.in:
* Source/WebKit/WebProcess/Extensions/API/Cocoa/WebExtensionAPIWebNavigationCocoa.mm:
(WebKit::WebExtensionAPIWebNavigation::getAllFrames):
(WebKit::WebExtensionAPIWebNavigation::getFrame):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebExtensionAPIWebNavigation.mm:
(TestWebKitAPI::TEST):

Canonical link: https://commits.webkit.org/271961@main
  • Loading branch information
@b-weinstein @xeenon
b-weinstein authored and xeenon committed Dec 13, 2023
1 parent afc57f4 commit b14b70f
Show file tree
Hide file tree
Showing 5 changed files with 78 additions and 17 deletions.
20 changes: 12 additions & 8 deletions Source/WebKit/UIProcess/Extensions/Cocoa/API/WebExtensionContextAPIWebNavigationCocoa.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
#import "WebExtensionFrameIdentifier.h"
#import "WebExtensionFrameParameters.h"
#import "WebExtensionTab.h"
#import "WebExtensionUtilities.h"
#import "WebFrame.h"
#import "_WKFrameTreeNode.h"
#import <wtf/BlockPtr.h>
Expand Down Expand Up @@ -85,43 +86,46 @@ static WebExtensionFrameParameters frameParametersForFrame(_WKFrameTreeNode *fra
return std::nullopt;
}

void WebExtensionContext::webNavigationGetFrame(WebExtensionTabIdentifier tabIdentifier, WebExtensionFrameIdentifier frameIdentifier, CompletionHandler<void(std::optional<WebExtensionFrameParameters>)>&& completionHandler)
void WebExtensionContext::webNavigationGetFrame(WebExtensionTabIdentifier tabIdentifier, WebExtensionFrameIdentifier frameIdentifier, CompletionHandler<void(std::optional<WebExtensionFrameParameters>, std::optional<String> error)>&& completionHandler)
{
auto tab = getTab(tabIdentifier);
if (!tab) {
completionHandler(std::nullopt);
completionHandler(std::nullopt, toErrorString(@"webNavigation.getFrame()", nil, @"tab not found"));
return;
}

auto webView = tab->mainWebView();
if (!webView) {
completionHandler(std::nullopt);
completionHandler(std::nullopt, toErrorString(@"webNavigation.getFrame()", nil, @"tab not found"));
return;
}

[webView _frames:makeBlockPtr([this, protectedThis = Ref { *this }, completionHandler = WTFMove(completionHandler), tab, frameIdentifier] (_WKFrameTreeNode *mainFrame) mutable {
completionHandler(webNavigationFindFrameIdentifierInFrameTree(mainFrame, nil, tab.get(), frameIdentifier));
if (auto frameParameters = webNavigationFindFrameIdentifierInFrameTree(mainFrame, nil, tab.get(), frameIdentifier))
completionHandler(frameParameters, std::nullopt);
else
completionHandler(std::nullopt, toErrorString(@"webNavigation.getFrame()", nil, @"frame not found"));
}).get()];
}

void WebExtensionContext::webNavigationGetAllFrames(WebExtensionTabIdentifier tabIdentifier, CompletionHandler<void(std::optional<Vector<WebExtensionFrameParameters>>)>&& completionHandler)
void WebExtensionContext::webNavigationGetAllFrames(WebExtensionTabIdentifier tabIdentifier, CompletionHandler<void(std::optional<Vector<WebExtensionFrameParameters>>, std::optional<String> error)>&& completionHandler)
{
auto tab = getTab(tabIdentifier);
if (!tab) {
completionHandler(std::nullopt);
completionHandler(std::nullopt, toErrorString(@"webNavigation.getFrame()", nil, @"tab not found"));
return;
}

auto webView = tab->mainWebView();
if (!webView) {
completionHandler(std::nullopt);
completionHandler(std::nullopt, toErrorString(@"webNavigation.getFrame()", nil, @"tab not found"));
return;
}

[webView _frames:makeBlockPtr([this, protectedThis = Ref { *this }, completionHandler = WTFMove(completionHandler), tab] (_WKFrameTreeNode *mainFrame) mutable {
Vector<WebExtensionFrameParameters> frameParameters;
webNavigationTraverseFrameTreeForFrame(mainFrame, nil, tab.get(), frameParameters);
completionHandler(frameParameters);
completionHandler(frameParameters, std::nullopt);
}).get()];
}

Expand Down
4 changes: 2 additions & 2 deletions Source/WebKit/UIProcess/Extensions/WebExtensionContext.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -623,8 +623,8 @@ class WebExtensionContext : public API::ObjectImpl<API::Object::Type::WebExtensi
void testFinished(bool result, String message, String sourceURL, unsigned lineNumber);

// WebNavigation APIs
void webNavigationGetFrame(WebExtensionTabIdentifier, WebExtensionFrameIdentifier, CompletionHandler<void(std::optional<WebExtensionFrameParameters>)>&&);
void webNavigationGetAllFrames(WebExtensionTabIdentifier, CompletionHandler<void(std::optional<Vector<WebExtensionFrameParameters>>)>&&);
void webNavigationGetFrame(WebExtensionTabIdentifier, WebExtensionFrameIdentifier, CompletionHandler<void(std::optional<WebExtensionFrameParameters>, std::optional<String> error)>&&);
void webNavigationGetAllFrames(WebExtensionTabIdentifier, CompletionHandler<void(std::optional<Vector<WebExtensionFrameParameters>>, std::optional<String> error)>&&);
void webNavigationTraverseFrameTreeForFrame(_WKFrameTreeNode *, _WKFrameTreeNode *parentFrame, WebExtensionTab*, Vector<WebExtensionFrameParameters> &);
std::optional<WebExtensionFrameParameters> webNavigationFindFrameIdentifierInFrameTree(_WKFrameTreeNode *, _WKFrameTreeNode *parentFrame, WebExtensionTab*, WebExtensionFrameIdentifier);

Expand Down
4 changes: 2 additions & 2 deletions Source/WebKit/UIProcess/Extensions/WebExtensionContext.messages.in
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,8 @@ messages -> WebExtensionContext {
TestFinished(bool result, String message, String sourceURL, unsigned lineNumber);

// WebNavigation APIs
WebNavigationGetAllFrames(WebKit::WebExtensionTabIdentifier tabIdentifier) -> (std::optional<Vector<WebKit::WebExtensionFrameParameters>> allFrameParameters)
WebNavigationGetFrame(WebKit::WebExtensionTabIdentifier tabIdentifier, WebKit::WebExtensionFrameIdentifier frameIdentifier) -> (std::optional<WebKit::WebExtensionFrameParameters> frameParameters)
WebNavigationGetAllFrames(WebKit::WebExtensionTabIdentifier tabIdentifier) -> (std::optional<Vector<WebKit::WebExtensionFrameParameters>> allFrameParameters, std::optional<String> error)
WebNavigationGetFrame(WebKit::WebExtensionTabIdentifier tabIdentifier, WebKit::WebExtensionFrameIdentifier frameIdentifier) -> (std::optional<WebKit::WebExtensionFrameParameters> frameParameters, std::optional<String> error)

// Windows APIs
WindowsCreate(WebKit::WebExtensionWindowParameters creationParameters) -> (std::optional<WebKit::WebExtensionWindowParameters> windowParameters, WebKit::WebExtensionWindow::Error error);
Expand Down
14 changes: 12 additions & 2 deletions Source/WebKit/WebProcess/Extensions/API/Cocoa/WebExtensionAPIWebNavigationCocoa.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,12 @@
if (!isValid(tabIdentifier, outExceptionString))
return;

WebProcess::singleton().sendWithAsyncReply(Messages::WebExtensionContext::WebNavigationGetAllFrames(tabIdentifier.value()), [protectedThis = Ref { *this }, callback = WTFMove(callback)](std::optional<Vector<WebExtensionFrameParameters>> results) {
WebProcess::singleton().sendWithAsyncReply(Messages::WebExtensionContext::WebNavigationGetAllFrames(tabIdentifier.value()), [protectedThis = Ref { *this }, callback = WTFMove(callback)](std::optional<Vector<WebExtensionFrameParameters>> results, std::optional<String> error) {
if (error) {
callback->reportError(error.value());
return;
}

callback->call(results ? toWebAPI(results.value()) : nil);
}, extensionContext().identifier());
}
Expand Down Expand Up @@ -119,7 +124,12 @@
return;
}

WebProcess::singleton().sendWithAsyncReply(Messages::WebExtensionContext::WebNavigationGetFrame(tabIdentifier.value(), frameIdentifier.value()), [protectedThis = Ref { *this }, callback = WTFMove(callback)](std::optional<WebExtensionFrameParameters> frameInfo) {
WebProcess::singleton().sendWithAsyncReply(Messages::WebExtensionContext::WebNavigationGetFrame(tabIdentifier.value(), frameIdentifier.value()), [protectedThis = Ref { *this }, callback = WTFMove(callback)](std::optional<WebExtensionFrameParameters> frameInfo, std::optional<String> error) {
if (error) {
callback->reportError(error.value());
return;
}

callback->call(frameInfo ? toWebAPI(frameInfo.value()) : nil);
}, extensionContext().identifier());
}
Expand Down
53 changes: 50 additions & 3 deletions Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebExtensionAPIWebNavigation.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@
[manager run];
}

TEST(WKWebExtensionAPIWebNavigation, GetMainFrameTest)
TEST(WKWebExtensionAPIWebNavigation, GetMainFrame)
{
TestWebKitAPI::HTTPServer server({
{ "/"_s, { { { "Content-Type"_s, "text/html"_s } }, "<iframe src='/frame.html'></iframe>"_s } },
Expand Down Expand Up @@ -267,7 +267,7 @@
[manager run];
}

TEST(WKWebExtensionAPIWebNavigation, GetSubframeTest)
TEST(WKWebExtensionAPIWebNavigation, GetSubframe)
{
TestWebKitAPI::HTTPServer server({
{ "/"_s, { { { "Content-Type"_s, "text/html"_s } }, "<iframe src='/frame.html'></iframe>"_s } },
Expand Down Expand Up @@ -316,7 +316,7 @@
[manager run];
}

TEST(WKWebExtensionAPIWebNavigation, GetAllFramesTest)
TEST(WKWebExtensionAPIWebNavigation, GetAllFrames)
{
TestWebKitAPI::HTTPServer server({
{ "/"_s, { { { "Content-Type"_s, "text/html"_s } }, "<iframe src='/frame.html'></iframe>"_s } },
Expand Down Expand Up @@ -375,6 +375,53 @@
[manager run];
}

TEST(WKWebExtensionAPIWebNavigation, Errors)
{
TestWebKitAPI::HTTPServer server({
{ "/"_s, { { { "Content-Type"_s, "text/html"_s } }, "<iframe src='/frame.html'></iframe>"_s } },
{ "/frame.html"_s, { { { "Content-Type"_s, "text/html"_s } }, "<body style='background-color: blue'></body>"_s } },
}, TestWebKitAPI::HTTPServer::Protocol::Http);

auto *backgroundScript = Util::constructScript(@[
// Setup
@"async function completedListener(details) {",
// Only listen for when the main frame loads so we don't call this method more than once.
@" if (details.frameId !== 0)",
@" return",
@" const activeTab = await browser.tabs.query({ active: true })",
// Make sure invalid tab/frame IDs vend an error message - use arbitrary frame and tabIds.
@" await browser.test.assertRejects(browser.webNavigation.getFrame({tabId: (details.tabId + 1), frameId: 0}), /tab not found/i)",
@" await browser.test.assertRejects(browser.webNavigation.getFrame({tabId: details.tabId, frameId: 42}), /frame not found/i)",
@" browser.test.notifyPass()",
@"}",

// The passListener firing will consider the test passed.
@"browser.webNavigation.onCompleted.addListener(completedListener)",

// Yield after creating the listener so we can load a tab.
@"browser.test.yield('Load Tab')"
]);

auto extension = adoptNS([[_WKWebExtension alloc] _initWithManifestDictionary:webNavigationManifest resources:@{ @"background.js": backgroundScript }]);
auto manager = adoptNS([[TestWebExtensionManager alloc] initForExtension:extension.get()]);

// Grant the webNavigation permission.
[manager.get().context setPermissionStatus:_WKWebExtensionContextPermissionStatusGrantedExplicitly forPermission:_WKWebExtensionPermissionWebNavigation];

auto *urlRequest = server.requestWithLocalhost();
NSURL *requestURL = urlRequest.URL;
[manager.get().context setPermissionStatus:_WKWebExtensionContextPermissionStatusGrantedExplicitly forURL:requestURL];
[manager.get().context setPermissionStatus:_WKWebExtensionContextPermissionStatusGrantedExplicitly forURL:[requestURL URLByAppendingPathComponent:@"frame.html"]];

[manager loadAndRun];

EXPECT_NS_EQUAL(manager.get().yieldMessage, @"Load Tab");

[manager.get().defaultTab.mainWebView loadRequest:urlRequest];

[manager run];
}

TEST(WKWebExtensionAPIWebNavigation, URLFilterTestMatchAllPredicates)
{
NSString *errorString = nil;
Expand Down

0 comments on commit b14b70f

Please sign in to comment.