[webkit-reviews] review denied: [Bug 238032] WKBundlePageUIClient console message support should include source URL, column number, and console messages with arguments : [Attachment 456304] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Mar 31 20:23:03 PDT 2022
Alex Christensen <achristensen at apple.com> has denied Jeff Miller
<jeffm at apple.com>'s request for review:
Bug 238032: WKBundlePageUIClient console message support should include source
URL, column number, and console messages with arguments
https://bugs.webkit.org/show_bug.cgi?id=238032
Attachment 456304: Patch
https://bugs.webkit.org/attachment.cgi?id=456304&action=review
--- Comment #15 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 456304
--> https://bugs.webkit.org/attachment.cgi?id=456304
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=456304&action=review
> Source/JavaScriptCore/inspector/ScriptArguments.cpp:88
> +bool ScriptArguments::getArgumentsAsStrings(Vector<String>& result) const
Vector<String> ScriptArguments::getArgumentsAsStrings() const
Can return an empty vector.
Or it could return a std::optional<Vector<String>>
> Source/JavaScriptCore/inspector/ScriptArguments.cpp:108
> + if (JSC::jsDynamicCast<JSC::ProxyObject*>(globalObject->vm(),
value)) {
> + result.append("[object Proxy]"_s);
> + continue;
> + }
> +
> + auto scope = DECLARE_CATCH_SCOPE(globalObject->vm());
> + result.append(value.toWTFString(globalObject));
> + scope.clearException();
This is all duplicate code.
> Source/WebCore/page/PageConsoleClient.cpp:130
> + messageArguments.remove(0);
This is the least efficient place to remove in a Vector. Let's not do this.
Could we instead make a createStringArray that takes a Span<String> and pass a
Span<String> through addMessageWithArgumentsToConsole containing indices 1
through the end? We'd have to do a bounds check here.
> Source/WebCore/page/PageConsoleClient.cpp:188
> + messageText = messageArguments[0];
ditto.
More information about the webkit-reviews
mailing list