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

[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 89823: fix patch 2: follow review
https://bugs.webkit.org/attachment.cgi?id=89823&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.