[webkit-reviews] review granted: [Bug 238032] WKBundlePageUIClient console message support should include source URL, column number, and console messages with arguments : [Attachment 456419] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted 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 456419: Patch

https://bugs.webkit.org/attachment.cgi?id=456419&action=review




--- Comment #17 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 456419
  --> https://bugs.webkit.org/attachment.cgi?id=456419
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456419&action=review

> Source/JavaScriptCore/inspector/ScriptArguments.cpp:111
> +    auto* globalObject = this->globalObject();
> +    if (!globalObject) {
> +	   ASSERT_NOT_REACHED();
> +	   return std::nullopt;
> +    }

This should be removed.  It's redundant and unused.

> Source/WebCore/page/PageConsoleClient.cpp:126
> +    ASSERT(!argumentsVector.isEmpty());
> +    if (size_t numberOfAdditionalArguments = argumentsVector.size() - 1)
> +	   return argumentsVector.span().subspan(1,
numberOfAdditionalArguments);
> +
> +    return { };

I think it would be safer if you checked instead of asserted that the vector is
empty.
You also don't need to check if numberOfAdditionalArguments is nonzero because
if it is zero it will just return an empty span.
I also don't think subspan needs a second argument in this case.

> Source/WebKit/Shared/API/APIArray.cpp:53
> +    return createStringArray(Vector<WTF::String>(strings));

This is an unneeded allocation