[webkit-reviews] review denied: [Bug 58665] [WK2] Should strip file:// urls before adding to console message : [Attachment 89794] fix patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Chang Shu <cshu at webkit.org>'s
request for review:
Bug 58665: [WK2] Should strip file:// urls before adding to console message
https://bugs.webkit.org/show_bug.cgi?id=58665

Attachment 89794: fix patch
https://bugs.webkit.org/attachment.cgi?id=89794&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=89794&action=review

Thanks for the clarification!

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:751
> +static string urlSuitableForTestResult(const string& url)

This function returns a name (lastPathComponent), not a URL, and should be
named accordingly.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:754
> +    if (url.empty() || string::npos == url.find("file://"))
> +	   return url;

Shouldn't this be an ASSERT?

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:766
> +    string filename = url.substr(pos + 1);

This doesn't seem right when pos is zero, as set in some cases above.

This also doesn't seem right for URLs ending with a slash. I'm not 100% sure
what lastPathComponent does, but it probably doesn't return an empty string for
those.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:768
> +    if (filename.empty())
> +	   return "file:"; // A WebKit test has this in its expected output.

I don't see Mac DumpRenderTree do that, and it's inconsistent with the rest of
the function, which returns a name, not a URL.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:777
> +    string newMessage = toSTD(message);

I'd call it messageString. There is nothing new about this message - it's the
same one that was passed in, possibly slightly massaged to remove paths.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:778
> +    if (!newMessage.empty()) {

I don't see any need for this check.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:779
> +	   size_t fileProtocol = newMessage.find(string("file://"));

This variable doesn't contain the protocol, so it would be better to call this
fileProtocolStart.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:781
> +	       newMessage = newMessage.substr(0, fileProtocol) +
urlSuitableForTestResult(newMessage.substr(fileProtocol));

I see that DumpRenderTree implementations do the same, but this is obviously
incorrect. There can be additional text after the URL, or there could be
multiple URLs in the message. This is worth a comment (you could explain how
this is broken, and that it matches DumpRenderTree).