Skip to content

Commit

Permalink
Simplify the CodeGeneratorExtensions.pm generated code for the most c…
Browse files Browse the repository at this point in the history 
…ommon optional argument case.

https://webkit.org/b/261163
rdar://problem/114986995

Reviewed by Brian Weinstein.

Refine the argument handling for the Web Extension code generator.

* Streamlined argument handling for the common 2 and 3 argument cases, specifically when the last argument is optional.
* Replaced the previous loop-based method for these cases with more direct conditional checks.
* Complex handling remains in place for scenarios with multiple optional arguments beyond 3 total arguments.
* Cuts down the functions using the complex method from roughly 50 to just 7.
* Helps both improved readability and potential generated code performance.

* Source/WebKit/WebProcess/Extensions/Bindings/Cocoa/JSWebExtensionWrapperCocoa.mm:
(WebKit::toNSString): Only try to convert strings.
(WebKit::toNSDictionary): Don't try to convert functions or promises.
* Source/WebKit/WebProcess/Extensions/Bindings/Scripts/CodeGeneratorExtensions.pm:
(_generateImplementationFile):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebExtensionAPIExtension.mm:
(TestWebKitAPI::TEST):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebExtensionAPIRuntime.mm:
(TestWebKitAPI::TEST):

Canonical link: https://commits.webkit.org/267695@main
  • Loading branch information
@xeenon
xeenon committed Sep 6, 2023
1 parent 2a314fd commit 77ea37f
Show file tree
Hide file tree
Showing 4 changed files with 97 additions and 56 deletions.
19 changes: 16 additions & 3 deletions Source/WebKit/WebProcess/Extensions/Bindings/Cocoa/JSWebExtensionWrapperCocoa.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,22 +193,35 @@ id toNSObject(JSContextRef context, JSValueRef valueRef, Class requiredClass)
FALLTHROUGH;

case NullStringPolicy::NoNullString:
// Don't try to convert other objects into strings.
if (!JSValueIsString(context, value))
return nil;

JSRetainPtr<JSStringRef> string(Adopt, JSValueToStringCopy(context, value, 0));
return CFBridgingRelease(JSStringCopyCFString(nullptr, string.get()));
}
}

NSDictionary *toNSDictionary(JSContextRef context, JSValueRef value)
NSDictionary *toNSDictionary(JSContextRef context, JSValueRef valueRef)
{
ASSERT(context);

if (!JSValueIsObject(context, value))
if (!JSValueIsObject(context, valueRef))
return nil;

JSObjectRef object = JSValueToObject(context, value, nullptr);
JSObjectRef object = JSValueToObject(context, valueRef, nullptr);
if (!object)
return nil;

// Don't try to convert functions.
if (JSObjectIsFunction(context, object))
return nil;

// Don't try to convert promises or regular expressions.
JSValue *value = [JSValue valueWithJSValueRef:valueRef inContext:[JSContext contextWithJSGlobalContextRef:JSContextGetGlobalContext(context)]];
if (value._isThenable || value._isRegularExpression)
return nil;

JSPropertyNameArrayRef propertyNames = JSObjectCopyPropertyNames(context, object);
size_t propertyNameCount = JSPropertyNameArrayGetCount(propertyNames);

Expand Down
126 changes: 77 additions & 49 deletions Source/WebKit/WebProcess/Extensions/Bindings/Scripts/CodeGeneratorExtensions.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,7 @@ EOF

$self->_includeHeaders(\%contentsIncludes, $function->type, $function);

my $argumentCount = scalar @specifiedParameters;
my $optionalArgumentCount = 0;

foreach my $parameter (@specifiedParameters) {
Expand All @@ -510,11 +511,20 @@ EOF
$callbackHandlerArgument = $parameter->name if $parameter->extendedAttributes->{"CallbackHandler"} && $parameter->extendedAttributes->{"Optional"};
}

my $requiredArgumentCount = scalar @specifiedParameters - $optionalArgumentCount;
my $processArgumentsLeftToRight = $function->extendedAttributes->{"ProcessArgumentsLeftToRight"};
my $requiredArgumentCount = $argumentCount - $optionalArgumentCount;
my $hasOptionalAsLastArgument = 1 if $lastParameter && $lastParameter->extendedAttributes->{"Optional"};
my $hasSimpleOptionalArgumentHandling = !$optionalArgumentCount || ($hasOptionalAsLastArgument && ($argumentCount <= 2 || $requiredArgumentCount >= $argumentCount - 1));
my $argumentIndexConditon = undef;

if (!$hasSimpleOptionalArgumentHandling) {
push(@contents, " ssize_t argumentIndex = -1;\n") unless $processArgumentsLeftToRight;
push(@contents, " size_t argumentIndex = argumentCount;\n") if $processArgumentsLeftToRight;
}

if ($requiredArgumentCount) {
push(@contents, <<EOF);
const size_t requiredArgumentCount = ${requiredArgumentCount};
constexpr size_t requiredArgumentCount = ${requiredArgumentCount};
if (UNLIKELY(argumentCount < requiredArgumentCount)) {
*exception = toJSError(context, @"${call}", nil, @"a required argument is missing");
return ${defaultEarlyReturnValue};
Expand All @@ -532,73 +542,83 @@ EOF
push(@parameters, $parameter->name) unless $parameter->extendedAttributes->{"CallbackHandler"} && $parameter->extendedAttributes->{"Optional"};
}

push(@contents, "\n");
push(@contents, "\n") unless $argumentCount eq 1;

push(@contents, " if (argumentCount == " . scalar @specifiedParameters . ") {\n");
my $hasOptionalCallbackHandlerAsLastArgument = 1 if $lastParameter->extendedAttributes->{"CallbackHandler"} && $hasOptionalAsLastArgument;

my $hasOptionalCallbackHandlerAsLastArgument = 1 if $lastParameter->extendedAttributes->{"CallbackHandler"} && $lastParameter->extendedAttributes->{"Optional"};
push(@contents, " if (argumentCount == ${argumentCount}) {\n") unless $argumentCount eq 1;
push(@contents, " if (argumentCount == ${argumentCount})\n") if $argumentCount eq 1;

foreach my $i (0..$#specifiedParameters) {
my $parameter = $specifiedParameters[$i];
push(@contents, " " . $self->_platformTypeVariableDeclaration($parameter, $parameter->name, "arguments[${i}]", undef, 1) . "\n");
}

push(@contents, " } else {\n");
if ($hasSimpleOptionalArgumentHandling && $argumentCount > 1) {
push(@contents, " } else if (argumentCount == " . ($argumentCount - 1) . ") {\n") if $requiredArgumentCount eq ($argumentCount - 1) || !$hasOptionalCallbackHandlerAsLastArgument;
push(@contents, " } else if (argumentCount == " . ($argumentCount - 1) . " && !(" . $self->_javaScriptTypeCondition($lastParameter, "arguments[0]") . ")) {\n") if $hasOptionalCallbackHandlerAsLastArgument && $requiredArgumentCount ne ($argumentCount - 1);

my $processArgumentsLeftToRight = $function->extendedAttributes->{"ProcessArgumentsLeftToRight"};
foreach my $i (0..$#specifiedParameters - 1) {
my $parameter = $specifiedParameters[$i];
push(@contents, " " . $self->_platformTypeVariableDeclaration($parameter, $parameter->name, "arguments[${i}]", undef, 1) . "\n");
}

push(@contents, " ssize_t argumentIndex = argumentCount - 1;\n") unless $processArgumentsLeftToRight;
push(@contents, " size_t argumentIndex = 0;\n") if $processArgumentsLeftToRight;
push(@contents, " JSValueRef currentArgument = nullptr;\n\n");
if ($hasOptionalCallbackHandlerAsLastArgument && $requiredArgumentCount ne ($argumentCount - 1)) {
push(@contents, " } else if (argumentCount == 1) {\n");
push(@contents, " " . $self->_platformTypeVariableDeclaration($lastParameter, $lastParameter->name, "arguments[0]", undef, 1) . "\n");
}
} elsif ($argumentCount > 1) {
push(@contents, " } else {\n");

push(@contents, " size_t allowedOptionalArgumentCount = argumentCount - requiredArgumentCount;\n") if $requiredArgumentCount;
push(@contents, " size_t processedOptionalArgumentCount = 0;\n\n") if $requiredArgumentCount;
push(@contents, " JSValueRef currentArgument = nullptr;\n");

if ($hasOptionalCallbackHandlerAsLastArgument && $processArgumentsLeftToRight) {
my $parameter = $specifiedParameters[$#specifiedParameters];
my $optionalCondition = $requiredArgumentCount ? "allowedOptionalArgumentCount && " : "";
push(@contents, " if ($optionalCondition(currentArgument = arguments[argumentCount - 1]) && (" . $self->_javaScriptTypeCondition($parameter, "currentArgument") . ")) {\n");
push(@contents, " " . $self->_platformTypeVariableDeclaration($parameter, $parameter->name, "currentArgument", undef, 1) . "\n");
push(@contents, " --allowedOptionalArgumentCount;\n") if $requiredArgumentCount;
push(@contents, " if (argumentCount)\n");
push(@contents, " --argumentCount;\n");
push(@contents, " }\n\n");
}
push(@contents, " const size_t allowedOptionalArgumentCount = argumentCount - requiredArgumentCount;\n") if $requiredArgumentCount;
push(@contents, " size_t processedOptionalArgumentCount = 0;\n") if $requiredArgumentCount;
push(@contents, " argumentIndex = argumentCount - 1;\n") unless $processArgumentsLeftToRight;
push(@contents, " argumentIndex = 0;\n") if $processArgumentsLeftToRight;

my $argumentIndexConditon = $processArgumentsLeftToRight ? "argumentIndex < argumentCount" : "argumentIndex >= 0";
my $nextArgumentIndex = $processArgumentsLeftToRight ? "++argumentIndex" : "--argumentIndex";
if ($hasOptionalCallbackHandlerAsLastArgument && $processArgumentsLeftToRight) {
my $parameter = $specifiedParameters[$#specifiedParameters];
my $optionalCondition = $requiredArgumentCount ? "allowedOptionalArgumentCount && " : "";
push(@contents, "\n");
push(@contents, " if ($optionalCondition(currentArgument = arguments[argumentCount - 1]) && (" . $self->_javaScriptTypeCondition($parameter, "currentArgument") . ")) {\n");
push(@contents, " " . $self->_platformTypeVariableDeclaration($parameter, $parameter->name, "currentArgument", undef, 1) . "\n");
push(@contents, " --allowedOptionalArgumentCount;\n") if $requiredArgumentCount;
push(@contents, " if (argumentCount)\n");
push(@contents, " --argumentCount;\n");
push(@contents, " }\n");
}

for (my $j = $processArgumentsLeftToRight ? 0 : $#specifiedParameters; $processArgumentsLeftToRight ? $j <= $#specifiedParameters : $j >= 0; $processArgumentsLeftToRight ? $j++ : $j--) {
my $parameter = $specifiedParameters[$j];
my $lastArgument = $j eq ($processArgumentsLeftToRight ? $#specifiedParameters : 0);
$argumentIndexConditon = $processArgumentsLeftToRight ? "argumentIndex < argumentCount" : "argumentIndex >= 0";
my $nextArgumentIndex = $processArgumentsLeftToRight ? "++argumentIndex" : "--argumentIndex";

next if $lastArgument && $hasOptionalCallbackHandlerAsLastArgument && $processArgumentsLeftToRight;
for (my $j = $processArgumentsLeftToRight ? 0 : $#specifiedParameters; $processArgumentsLeftToRight ? $j <= $#specifiedParameters : $j >= 0; $processArgumentsLeftToRight ? $j++ : $j--) {
my $parameter = $specifiedParameters[$j];
my $lastArgument = $j eq ($processArgumentsLeftToRight ? $#specifiedParameters : 0);

my $optionalCondition = $requiredArgumentCount && $parameter->extendedAttributes->{"Optional"} ? " && processedOptionalArgumentCount < allowedOptionalArgumentCount" : "";
push(@contents, " if ($argumentIndexConditon$optionalCondition && (currentArgument = arguments[argumentIndex])) {\n");
next if $lastArgument && $hasOptionalCallbackHandlerAsLastArgument && $processArgumentsLeftToRight;

if ($parameter->extendedAttributes->{"Optional"}) {
push(@contents, " if (" . $self->_javaScriptTypeCondition($parameter, "currentArgument") . ") {\n");
push(@contents, " " . $self->_platformTypeVariableDeclaration($parameter, $parameter->name, "currentArgument", undef, 1) . "\n");
push(@contents, " ++processedOptionalArgumentCount;\n") if $requiredArgumentCount;
push(@contents, " $nextArgumentIndex;\n");
push(@contents, " }\n");
} else {
push(@contents, " " . $self->_platformTypeVariableDeclaration($parameter, $parameter->name, "currentArgument", undef, 1) . "\n");
push(@contents, " $nextArgumentIndex;\n");
}
push(@contents, "\n");

push(@contents, " }\n");
my $optionalCondition = $requiredArgumentCount && $parameter->extendedAttributes->{"Optional"} ? " && processedOptionalArgumentCount < allowedOptionalArgumentCount" : "";
push(@contents, " if ($argumentIndexConditon$optionalCondition && (currentArgument = arguments[argumentIndex])) {\n");

push(@contents, "\n") unless ($j eq $#specifiedParameters - 1 && $hasOptionalCallbackHandlerAsLastArgument && $processArgumentsLeftToRight);
}
if ($parameter->extendedAttributes->{"Optional"}) {
push(@contents, " if (" . $self->_javaScriptTypeCondition($parameter, "currentArgument") . ") {\n");
push(@contents, " " . $self->_platformTypeVariableDeclaration($parameter, $parameter->name, "currentArgument", undef, 1) . "\n");
push(@contents, " ++processedOptionalArgumentCount;\n") if $requiredArgumentCount;
push(@contents, " $nextArgumentIndex;\n");
push(@contents, " }\n");
} else {
push(@contents, " " . $self->_platformTypeVariableDeclaration($parameter, $parameter->name, "currentArgument", undef, 1) . "\n");
push(@contents, " $nextArgumentIndex;\n");
}

push(@contents, " if (UNLIKELY($argumentIndexConditon)) {\n");
push(@contents, " *exception = toJSError(context, @\"${call}\", nil, @\"an unknown argument was provided\");\n");
push(@contents, " return ${defaultReturnValue};\n");
push(@contents, " }\n");
push(@contents, " }\n");
}
}

push(@contents, " }\n");
push(@contents, " }\n") unless $argumentCount eq 1;
} else {
foreach my $i (0..$#specifiedParameters) {
my $parameter = $specifiedParameters[$i];
Expand Down Expand Up @@ -629,6 +649,14 @@ EOF
$self->_installAutomaticExceptions(\@contents, $parameter, $idlType, $parameter->name, $parameter->name, $defaultReturnValue, \%contentsIncludes, $function, 1);
}

if (!$hasSimpleOptionalArgumentHandling) {
push(@contents, "\n");
push(@contents, " if (UNLIKELY($argumentIndexConditon)) {\n");
push(@contents, " *exception = toJSError(context, @\"${call}\", nil, @\"an unknown argument was provided\");\n");
push(@contents, " return ${defaultReturnValue};\n");
push(@contents, " }\n");
}

unshift(@methodSignatureNames, "context") if $needsScriptContext;
unshift(@parameters, "context") if $needsScriptContext;

Expand Down Expand Up @@ -663,7 +691,7 @@ EOF
EOF
}

if ($callbackHandlerArgument) {
if ($callbackHandlerArgument && !$returnsPromiseIfNoCallback) {
push(@contents, <<EOF);
if (!${callbackHandlerArgument})
${callbackHandlerArgument} = toJSErrorCallbackHandler(context, impl->runtime());
Expand Down
4 changes: 2 additions & 2 deletions Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebExtensionAPIExtension.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@
@"browser.test.assertThrows(() => browser.extension.getURL(), /required argument is missing/i)",
@"browser.test.assertThrows(() => browser.extension.getURL(null), /'resourcePath' value is invalid, because a string is expected/i)",
@"browser.test.assertThrows(() => browser.extension.getURL(undefined), /'resourcePath' value is invalid, because a string is expected/i)",
@"browser.test.assertThrows(() => browser.extension.getURL(42), /'resourcePath' value is invalid, because a string is expected/i)",
@"browser.test.assertThrows(() => browser.extension.getURL(/test/), /'resourcePath' value is invalid, because a string is expected/i)",

// Normal Cases
@"browser.test.assertEq(browser.extension.getURL(''), `${baseURL}/`)",
Expand All @@ -59,8 +61,6 @@

// Unexpected Cases
// FIXME: <https://webkit.org/b/248154> browser.extension.getURL() has some edge cases that should be failures or return different results
@"browser.test.assertEq(browser.extension.getURL(42), `${baseURL}/42`)",
@"browser.test.assertEq(browser.extension.getURL(/test/), `${baseURL}/test/`)",
@"browser.test.assertEq(browser.extension.getURL('//'), 'test-extension://')",
@"browser.test.assertEq(browser.extension.getURL('//example'), `test-extension://example`)",
@"browser.test.assertEq(browser.extension.getURL('///'), 'test-extension:///')",
Expand Down
4 changes: 2 additions & 2 deletions Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebExtensionAPIRuntime.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@
@"browser.test.assertThrows(() => browser.runtime.getURL(), /required argument is missing/i)",
@"browser.test.assertThrows(() => browser.runtime.getURL(null), /'resourcePath' value is invalid, because a string is expected/i)",
@"browser.test.assertThrows(() => browser.runtime.getURL(undefined), /'resourcePath' value is invalid, because a string is expected/i)",
@"browser.test.assertThrows(() => browser.runtime.getURL(42), /'resourcePath' value is invalid, because a string is expected/i)",
@"browser.test.assertThrows(() => browser.runtime.getURL(/test/), /'resourcePath' value is invalid, because a string is expected/i)",

// Normal Cases
@"browser.test.assertEq(browser.runtime.getURL(''), `${baseURL}/`)",
Expand All @@ -69,8 +71,6 @@

// Unexpected Cases
// FIXME: <https://webkit.org/b/248154> browser.runtime.getURL() has some edge cases that should be failures or return different results
@"browser.test.assertEq(browser.runtime.getURL(42), `${baseURL}/42`)",
@"browser.test.assertEq(browser.runtime.getURL(/test/), `${baseURL}/test/`)",
@"browser.test.assertEq(browser.runtime.getURL('//'), 'test-extension://')",
@"browser.test.assertEq(browser.runtime.getURL('//example'), `test-extension://example`)",
@"browser.test.assertEq(browser.runtime.getURL('///'), 'test-extension:///')",
Expand Down

0 comments on commit 77ea37f

Please sign in to comment.