Bug 58665 – [WK2] Should strip file:// urls before adding to console message

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chang Shu]

When a message with url embedded is added to console, the "file:" scheme and path should be stripped.

[Chang Shu]

Created attachment 89794 [details]
fix patch

[Alexey Proskuryakov]

Comment on attachment 89794 [details]
fix patch

Do we have the same logic in DumpRenderTree somewhere?

[Chang Shu]

(In reply to comment #2)
> (From update of attachment 89794 [details])
> Do we have the same logic in DumpRenderTree somewhere?

Yes. The DumpRenderTree implementation is platform-dependent. I found the code for Chromium (chromium/WebViewHost.cpp) is actually a platform-independent version and just copied over.

[Alexey Proskuryakov]

Comment on attachment 89794 [details]
fix patch

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).

[Chang Shu]

Created attachment 89823 [details]
fix patch 2: follow review

[Alexey Proskuryakov]

Comment on attachment 89823 [details]
fix patch 2: follow review

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

This looks generally right, but I had enough comments to justify another short review round.

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

It's not clear from the name if we are dealing with URL paths or OS ones. This confusion spills into actual code, so I suggest improving the name (maybe lastURLPathComponent()?).

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:758
> +#if OS(WINDOWS)
> +    char delim = '\\';

Is this actually true? I strongly doubt it, we use forward slashes for file URLs in KURL.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:763
> +    if (tmpPath[tmpPath.length() - 1] == delim)

This will be an out of bounds read if the passed string is "file://".

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:764
> +        tmpPath = path.substr(0, tmpPath.length() - 1);

This should be tmpPath.substr(), not path.substr(). Also, you can just use remove().

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:778
> +    size_t fileProtocolStart = messageString.find(string("file://"));

I don't think that explicitly constructing a string is necessary here.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:780
> +        // FIXME: The code below does not handle additional texts after url nor multiple urls.

s/texts/text/. It might make sense to mention that this matches DumpRenderTree.

[Chang Shu]

Created attachment 89828 [details]
fix patch 3

[Alexey Proskuryakov]

Comment on attachment 89828 [details]
fix patch 3

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

r=me unless you decide to switch to KURL, which would require a new review.

> Tools/ChangeLog:10
> +        (WTR::lastPathComponent):

This function has a different name now.

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

I just realized that since we're asserting that it starts with file://, the name should be lastFileURLPathComponent(). Sorry for a bad suggestion previously.

Also, can we use KURL here? That would obviously be much cleaner than writing custom code.

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

We prefer early returns to indenting code in if blocks.

[Chang Shu]

Created attachment 89838 [details]
fix patch 4

[WebKit Commit Bot]

Comment on attachment 89838 [details]
fix patch 4

Clearing flags on attachment: 89838

Committed r84070: <http://trac.webkit.org/changeset/84070>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.