Bug 47486 – WTR should accept relative paths

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Balazs Kelemen]

Since http://trac.webkit.org/changeset/69297 WTR does not accepts relative file paths (my fault).
For manual testing this is uncomfortable and should be fixed.

[Balazs Kelemen]

Created attachment 70505 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 70505 [details]
Patch

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

> WebKitTools/ChangeLog:11
> +        * WebKitTestRunner/TestInvocation.cpp:
> +        (WTR::createWKURL): Moved from StringFunctions.h since it is
> +        used only here. Extend relative paths to absolute.

I don't think we need this move. WTR::createWKURL is a general function 
and not TestInvocation specific. Now only the TestInvocation.cpp uses it,
but it can be changed in the future.

> WebKitTools/WebKitTestRunner/TestInvocation.cpp:64
> +#if OS(WINDOWS)
> +    const char separator = '\\';
> +    bool isAbsolutePath = length >= 3 && pathOrURL[1] == ':' && pathOrURL[2] == separator;
> +#else
> +    const char separator = '/';
> +    bool isAbsolutePath = pathOrURL[0] == separator;
> +#endif
> +

We don't need Windows case, because we run WTR inside CygWin
where the paths are similar to paths on Linux (/cygdrive/c/....)

[Balazs Kelemen]

Created attachment 70525 [details]
Patch

[Balazs Kelemen]

(In reply to comment #2)
> (From update of attachment 70505 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70505&action=review
> 
> > WebKitTools/ChangeLog:11
> > +        * WebKitTestRunner/TestInvocation.cpp:
> > +        (WTR::createWKURL): Moved from StringFunctions.h since it is
> > +        used only here. Extend relative paths to absolute.
> 
> I don't think we need this move. WTR::createWKURL is a general function 
> and not TestInvocation specific. Now only the TestInvocation.cpp uses it,
> but it can be changed in the future.
> 

Then we can move back, but I am pretty sure that it would not be needed anywhere
else. I believe that the mistake was to put in in the header. Actually it is not
something we should inlineing.

[Balazs Kelemen]

Let's start the whole thing from the beginning.
We are sending the url string to the web process.
It does not matter that WTR is running in cygwin or not,
the WebPage needs a native file system path in the url.
I think the first patch is the right one, but the problem is
that WTR does not working on our windows machine currently.
I am going to try to making it work before setting r+ on the first one
or uploading a new patch. Will be continued...

[Balazs Kelemen]

Finally I have found out that old-run-webkit-test gives absolute windows path to
WTR and WTR never supported relative cygwin paths. That means that the first
approach was the right one.

[Balazs Kelemen]

Created attachment 70607 [details]
same as the first with a win build fix

With this change relative paths works on *nix and on windows from cmd.exe.
On windows from cygwin relative paths also work as long as the patch is just
the filename (so the file is in the current directory). In summary the behavior
is the same that it was before http://trac.webkit.org/changeset/69297.

[Csaba Osztrogonác]

Comment on attachment 70607 [details]
same as the first with a win build fix

LGTM, r=me.

[Balazs Kelemen]

Comment on attachment 70607 [details]
same as the first with a win build fix

Clearing flags on attachment: 70607

Committed r69657: <http://trac.webkit.org/changeset/69657>

[Balazs Kelemen]

All reviewed patches have been landed.  Closing bug.