Bug 58665

Created attachment 89794 [details] fix patch

Comment on attachment 89794 [details] fix patch Do we have the same logic in DumpRenderTree somewhere?

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

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

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

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.

Created attachment 89828 [details] fix patch 3

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.

Created attachment 89838 [details] fix patch 4

Comment on attachment 89838 [details] fix patch 4 Clearing flags on attachment: 89838 Committed r84070: <http://trac.webkit.org/changeset/84070>

All reviewed patches have been landed. Closing bug.