Bug 58665

Summary: [WK2] Should strip file:// urls before adding to console message
Product: WebKit Reporter: Chang Shu <cshu>
Component: Tools / TestsAssignee: Chang Shu <cshu>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on:    
Bug Blocks: 42541    
Attachments:
Description Flags
fix patch
ap: review-
fix patch 2: follow review
ap: review-
fix patch 3
ap: review+
fix patch 4 none

Description Chang Shu 2011-04-15 08:18:04 PDT
When a message with url embedded is added to console, the "file:" scheme and path should be stripped.
Comment 1 Chang Shu 2011-04-15 08:42:22 PDT
Created attachment 89794 [details]
fix patch
Comment 2 Alexey Proskuryakov 2011-04-15 09:08:29 PDT
Comment on attachment 89794 [details]
fix patch

Do we have the same logic in DumpRenderTree somewhere?
Comment 3 Chang Shu 2011-04-15 09:12:26 PDT
(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 4 Alexey Proskuryakov 2011-04-15 10:12:47 PDT
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).
Comment 5 Chang Shu 2011-04-15 11:53:55 PDT
Created attachment 89823 [details]
fix patch 2: follow review
Comment 6 Alexey Proskuryakov 2011-04-15 12:09:55 PDT
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.
Comment 7 Chang Shu 2011-04-15 12:33:18 PDT
Created attachment 89828 [details]
fix patch 3
Comment 8 Alexey Proskuryakov 2011-04-15 12:40:25 PDT
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.
Comment 9 Chang Shu 2011-04-15 13:01:06 PDT
Created attachment 89838 [details]
fix patch 4
Comment 10 WebKit Commit Bot 2011-04-15 18:53:18 PDT
Comment on attachment 89838 [details]
fix patch 4

Clearing flags on attachment: 89838

Committed r84070: <http://trac.webkit.org/changeset/84070>
Comment 11 WebKit Commit Bot 2011-04-15 18:53:22 PDT
All reviewed patches have been landed.  Closing bug.