WKBundlePageUIClient console message support should include source URL, column number, and console messages with arguments
rdar://90439983
Created attachment 455016 [details] Patch
It appears this change inserts an extra space in console messages, which breaks test expectations. For example: -CONSOLE MESSAGE: This test passes if it does not crash. +CONSOLE MESSAGE: This test passes if it does not crash. I'll fix this before landing.
Created attachment 455056 [details] Patch
Created attachment 455063 [details] Patch
Not surprisingly, now that we include console message arguments, some existing test expectations are now incorrect. For example: -CONSOLE MESSAGE: [loadCrossOriginImage] +CONSOLE MESSAGE: [loadCrossOriginImage] trying http://localhost:8000/webgl/resources/webgl_test_files/resources/opengl_logo.jpg Also, I think the fact that we always call addMessageToConsole() now, even if the message is empty, is causing issues. For example, an expected result is: Test for the Console.messagesCleared event. and the actual result is: CONSOLE MESSAGE: CONSOLE MESSAGE: Test for the Console.messagesCleared event. The latter issue is a bug in this patch, but the former issue is trickier. Rather than changing the existing behavior of the willAddMessageToConsole callback, a more conservative change is to only include the message arguments when using the new willAddMessageWithDetailsToConsole callback.
Created attachment 455115 [details] Patch
The win bot passes now, but the console-log-proxy test on the gtk-wk2 bot is failing: https://ews-build.s3-us-west-2.amazonaws.com/GTK-WK2-Tests-EWS/455115-11928/inspector/console/console-log-proxy-diff.txt --- /home/ews/worker/GTK-WK2-Tests-EWS/build/layout-test-results/inspector/console/console-log-proxy-expected.txt +++ /home/ews/worker/GTK-WK2-Tests-EWS/build/layout-test-results/inspector/console/console-log-proxy-actual.txt @@ -1,14 +1,21 @@ CONSOLE MESSAGE: [object Proxy] CONSOLE MESSAGE: 0 CONSOLE MESSAGE: [object Proxy] +CONSOLE MESSAGE: 1 +CONSOLE MESSAGE: 1 +CONSOLE MESSAGE: 1 CONSOLE MESSAGE: 1 Tests for the console.log with Proxy objects. I'll leave the patch up, I want to see what happens with the mac-wk2 bot, but I definitely seem to have regressed the console-log-proxy, although I have no idea how.
Tim Hatcher pointed out that the console-log-proxy is likely failing because I'm walking through all the arguments, fetching them, and converting them to strings. This causes the Proxy object to be fetched. I need to be implementing this more like ScriptArguments::getFirstArgumentAsString() and do special Proxy object handling.
Created attachment 456068 [details] Patch
Comment on attachment 456068 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456068&action=review No test? BundlePageConsoleMessage uses willAddMessageToConsole, so a similar technique could be used to test this. > Source/WebCore/ChangeLog:9 > + Add a new ChromeClient addMessageWithArgumentsToConsole() member function for adding a console > + message that includes formatted arguments. I left the existing behavior of addMessageToConsole() I can't say I think this is a great API. I would expect a WKArrayRef rather than a space separated list inside the string with the message, for which you have to assume that none of the arguments have spaces in them if you want to do anything with them. > Source/WebCore/page/PageConsoleClient.cpp:222 > + for (size_t i = 1; i < arguments->argumentCount(); ++i) { This is basically duplicate code.
Comment on attachment 456068 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456068&action=review >> Source/WebCore/ChangeLog:9 >> + message that includes formatted arguments. I left the existing behavior of addMessageToConsole() > > I can't say I think this is a great API. I would expect a WKArrayRef rather than a space separated list inside the string with the message, for which you have to assume that none of the arguments have spaces in them if you want to do anything with them. Since you mention WKArrayRef, I assume you're talking about the API exposed to the injected bundle: typedef void (*WKBundlePageWillAddMessageWithDetailsToConsoleCallback)(WKBundlePageRef page, WKStringRef message, uint32_t lineNumber, uint32_t columnNumber, WKStringRef sourceID, const void *clientInfo); The intent of this patch was to better support writing client-side tests that typically look for complete messages from JavaScript, but we can still do this while providing more information about the individual parameters as you suggested. I'll change this. >> Source/WebCore/page/PageConsoleClient.cpp:222 >> + for (size_t i = 1; i < arguments->argumentCount(); ++i) { > > This is basically duplicate code. Good point, I'll refactor this logic into a separate function.
I'll add a test as well.
Created attachment 456304 [details] Patch
Comment on attachment 456304 [details] 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.
Created attachment 456419 [details] Patch
Comment on attachment 456419 [details] 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
Comment on attachment 456419 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456419&action=review >> Source/JavaScriptCore/inspector/ScriptArguments.cpp:111 >> + } > > This should be removed. It's redundant and unused. Yup, just missed this during the refactoring.
Created attachment 456428 [details] Patch
ChangeLog entry in Tools/ChangeLog contains OOPS!.
r292254
http://trac.webkit.org/r292259