[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
bugzilla-daemon at webkit.org
Fri Apr 15 12:09:54 PDT 2011
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.
More information about the webkit-reviews
mailing list