Bug 58665 - [WK2] Should strip file:// urls before adding to console message
Summary: [WK2] Should strip file:// urls before adding to console message
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Chang Shu
URL:
Keywords:
Depends on:
Blocks: 42541
  Show dependency treegraph
 
Reported: 2011-04-15 08:18 PDT by Chang Shu
Modified: 2011-04-15 18:53 PDT (History)
2 users (show)

See Also:


Attachments
fix patch (4.43 KB, patch)
2011-04-15 08:42 PDT, Chang Shu
ap: review-
Details | Formatted Diff | Diff
fix patch 2: follow review (4.53 KB, patch)
2011-04-15 11:53 PDT, Chang Shu
ap: review-
Details | Formatted Diff | Diff
fix patch 3 (4.54 KB, patch)
2011-04-15 12:33 PDT, Chang Shu
ap: review+
Details | Formatted Diff | Diff
fix patch 4 (4.56 KB, patch)
2011-04-15 13:01 PDT, Chang Shu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.