Bug 119583 – [Win] URL printing code in DumpRenderTree doesn't match WTR or Mac DRT

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

urlSuitableForTestResult() in DumpRenderTree/win/DumpRenderTree.cpp removes a prefix from url without checking whether the url actually has this prefix:

--------------------------
    RetainPtr<CFStringRef> basePath = adoptCF(CFURLCopyPath(baseURL.get()));
    RetainPtr<CFStringRef> path = adoptCF(CFURLCopyPath(url.get()));

    return cfStringRefToWString(substringFromIndex(path.get(), CFStringGetLength(basePath.get())).get());

--------------------------

When it doesn't match - or when base URL cannot be determined - we should print last path component.

[Alexey Proskuryakov]

Either WebKitTestRunner code or Mac DRT could probably serve as a good model to follow.

[Brent Fulgham]

Created attachment 208928 [details]
Patch

[Brent Fulgham]

Created attachment 208929 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 208929 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208929&action=review

There are more instances of "return urlString;" above the changed code, all except the very first one should be changed to return the last path component. We never want a full file:// path!

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:172
> +    if (path.get() && CFStringHasPrefix(path.get(), basePath.get()))

This null check should be for basePath, not for path.

[Brent Fulgham]

Comment on attachment 208929 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208929&action=review

>> Tools/DumpRenderTree/win/DumpRenderTree.cpp:172
>> +    if (path.get() && CFStringHasPrefix(path.get(), basePath.get()))
> 
> This null check should be for basePath, not for path.

Oops!

[Brent Fulgham]

Created attachment 208931 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 208931 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208931&action=review

Great, thank you for tackling this!

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:144
> +static wstring fallbackString(CFURLRef url)

I'd name this function after what it does - in this case, it seems more straightforward than naming it by what it is used for. What about lastPathComponent() as the name?

[Alexey Proskuryakov]

Or something like lastPathComponentAsWString().

[Brent Fulgham]

(In reply to comment #8)
> Or something like lastPathComponentAsWString().

That's a good idea.  I'll change it while landing.

[Brent Fulgham]

Committed r154194: <http://trac.webkit.org/changeset/154194>