Bug 67254 – [Qt][DRT] Normalize file:///tmp/LayoutTests in LayoutTestController::pathToLocalResource()

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jarred Nicholls]

Qt DRT should translate file:///tmp/LayoutTests paths into a local URL that's derived from the path of the DRT application exec.

[Jarred Nicholls]

Created attachment 105837 [details]
Proposed DRT Patch

The following tests are now passing in Qt's DumpRenderTree:

fast/dom/frame-loading-via-document-write.html
fast/loader/local-CSS-from-local.html
fast/loader/local-JavaScript-from-local.html
fast/loader/local-image-from-local.html

Some others in http/tests/misc and http/tests/security are also now passing.

[Benjamin Poulain]

Comment on attachment 105837 [details]
Proposed DRT Patch

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

Suggestion:
Create a temp variable for "file:///tmp/LayoutTests/", and use it afterwards. Pseudo-ish code:
const Qstring localTmpUrl(QLatin1String("file:///tmp/LayoutTests/"));
if (url.startsWith(localTmpUrl) {
    QFileInfo layoutTestsRoot(QCoreApplication::applicationDirPath() + "/../../../LayoutTests/");
    if (layoutTestsRoot.exists())
        return url.right(localTmpUrl.length()) + layoutTestsRoot.absolutePath();
}

> Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:248
> +        QString u(url);

not a fan of "u" for the variable name.

> Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:252
> +        // https://bugs.webkit.org/show_bug.cgi?id=67254

I would not add the bug number in this comment, it implies this bug report is about fixing the test on Windows due to the symlink issue.

> Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:708
> +    QByteArray urlData = pathToLocalResource(url).toAscii();

.toAscii() is evil, you should use toUtf8() or toLatin1()

[Jarred Nicholls]

(In reply to comment #2)
> (From update of attachment 105837 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105837&action=review
> 
> Suggestion:
> Create a temp variable for "file:///tmp/LayoutTests/", and use it afterwards. Pseudo-ish code:
> const Qstring localTmpUrl(QLatin1String("file:///tmp/LayoutTests/"));
> if (url.startsWith(localTmpUrl) {
>     QFileInfo layoutTestsRoot(QCoreApplication::applicationDirPath() + "/../../../LayoutTests/");
>     if (layoutTestsRoot.exists())
>         return url.right(localTmpUrl.length()) + layoutTestsRoot.absolutePath();
> }

I'll do this and fix any tests that have four slashes e.g. file:////tmp/LayoutTests (that's the reason for indexOf)

> 
> > Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:248
> > +        QString u(url);
> 
> not a fan of "u" for the variable name.

Me neither, but I'll avoid a non-const string.

> 
> > Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:252
> > +        // https://bugs.webkit.org/show_bug.cgi?id=67254
> 
> I would not add the bug number in this comment, it implies this bug report is about fixing the test on Windows due to the symlink issue.

Good call

> 
> > Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:708
> > +    QByteArray urlData = pathToLocalResource(url).toAscii();
> 
> .toAscii() is evil, you should use toUtf8() or toLatin1()

Agreed, wanted to keep consistent with what's there but definitely agree.  Shouldn't bother data: encoded URLs.

[Jarred Nicholls]

Created attachment 105896 [details]
Proposed DRT Patch

Reviewer's comments

[Benjamin Poulain]

Comment on attachment 105896 [details]
Proposed DRT Patch

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

> Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:246
>      // Function introduced in r28690.
> -    return QDir::toNativeSeparators(url);
> +

You can remove the comment and the empty line, the are not adding any value.

> Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:254
> +        QFileInfo layoutTestsRoot(QCoreApplication::applicationDirPath() + "/../../../LayoutTests/");

Should probably be + QLatin1String("/../../../LayoutTests/");

> Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:256
> +            return url.left(7) + layoutTestsRoot.absolutePath() + url.mid(localTmpUrl.length());

url.left(7) -> magic numbers are not nice.
You can just use QLatin1String("file://").

[Benjamin Poulain]

The code looks good otherwise but I have no verified if the patch is the right solution :)

The win port implementation is quite different, and not specific to /tmp: http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/win/LayoutTestControllerWin.cpp#L674

[Jarred Nicholls]

Created attachment 105939 [details]
Proposed DRT Patch

QLatin1Strings

[Jarred Nicholls]

(In reply to comment #6)
> The code looks good otherwise but I have no verified if the patch is the right solution :)
> 
> The win port implementation is quite different, and not specific to /tmp: http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/win/LayoutTestControllerWin.cpp#L674

I've read the win port's implementation, and obviously it's based on a Cygwin environment.  That's not good enough.  And that would still mean the NRWT would have to create a symlink into that environment, which it doesn't right now.

Dirk Pranke from Google and I are on the same page, that DRT/WTR should be smart and take care of remapping these URLs to a local URL that's derived by the running process' location.  NRWT shouldn't have to know to create a symlink or be tied to a *nix environment in order to function properly.

Thus, this is what Chromium does.  See some of the relevant code parts here: http://pastie.org/pastes/2464106  The LayoutTests folder is derived by the running exec's location.

Changing the tests is also not good, because it will lower the integrity of the test; and have to be absolute URLs for the security tests where we're attempting to load local resources from a remote domain.

Let me know if you have a better idea :)  A better generic solution would be a configurable hash of remappable URL paths that DRT/WTR can work with, instead of this static solution - but this is a good stop gap I think.

[Andras Becsi]

Comment on attachment 105939 [details]
Proposed DRT Patch

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

I've CC'd Ossy who cares about this.

The change looks good to me except that you change the fallback behavior compared to the original approach.

> Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:257
> +    return url;

I think this should be "return QDir::toNativeSeparators(url);" so that the fallback case matches the previous behavior.

[Csaba Osztrogonác]

I'll check it and verify today evening.

[Jarred Nicholls]

(In reply to comment #9)
> (From update of attachment 105939 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105939&action=review
> 
> I've CC'd Ossy who cares about this.
> 
> The change looks good to me except that you change the fallback behavior compared to the original approach.
> 
> > Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:257
> > +    return url;
> 
> I think this should be "return QDir::toNativeSeparators(url);" so that the fallback case matches the previous behavior.

This change was in intentional.  What's provided to pathToLocalResource is always a URL for remapping if needed.  Turning all forward slashes into back slashes is unnecessary.  The function ought to be named "urlToLocalResource" or something.

[Andras Becsi]

Ok, in this case the call is superfluous indeed. I think the change can go. But let's wait for Ossy's verification first.

(In reply to comment #11)
> > > Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:257
> > > +    return url;
> > 
> > I think this should be "return QDir::toNativeSeparators(url);" so that the fallback case matches the previous behavior.
> 
> This change was in intentional.  What's provided to pathToLocalResource is always a URL for remapping if needed.  Turning all forward slashes into back slashes is unnecessary.  The function ought to be named "urlToLocalResource" or something.

[Jarred Nicholls]

(In reply to comment #12)
> Ok, in this case the call is superfluous indeed. I think the change can go. But let's wait for Ossy's verification first.

K, sounds good.

[Jarred Nicholls]

Ossy did you get a few moments to verify? Thanks.

[Csaba Osztrogonác]

Oooops, I forgot it. I'll check it immediately.

[Jarred Nicholls]

(In reply to comment #15)
> Oooops, I forgot it. I'll check it immediately.

No worries!  Just checking in :)

[Csaba Osztrogonác]

Comment on attachment 105939 [details]
Proposed DRT Patch

LGTM, r=me

[Csaba Osztrogonác]

Comment on attachment 105939 [details]
Proposed DRT Patch

Clearing flags on attachment: 105939

Committed r94676: <http://trac.webkit.org/changeset/94676>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because fast/dom/frame-loading-via-document-write.html still fails. Could you check it, please? (Unfortunately I didn't realized it before, because I had /tmp/LayoutTests symlink)

[Jarred Nicholls]

(In reply to comment #20)
> Reopen, because fast/dom/frame-loading-via-document-write.html still fails. Could you check it, please? (Unfortunately I didn't realized it before, because I had /tmp/LayoutTests symlink)

Indeed, see bug #67784.  Thanks!

[Jarred Nicholls]

Ossy, is this good to close this again?

[Csaba Osztrogonác]

(In reply to comment #22)
> Ossy, is this good to close this again?

Yes, I checked it and it works. What do you think if we disable creating /tmp/LayoutTests symlink in ORWT for Qt port.

[Ademar (unprivileged account)]

Revision r94676 cherry-picked into qtwebkit-2.2 with commit 4f9120c <http://gitorious.org/webkit/qtwebkit/commit/4f9120c>

[Jarred Nicholls]

(In reply to comment #23)
> (In reply to comment #22)
> > Ossy, is this good to close this again?
> 
> Yes, I checked it and it works. What do you think if we disable creating /tmp/LayoutTests symlink in ORWT for Qt port.

Eventually yes; in fact all ports will be able to do it together.  But I don't think we should do that until WebKitTestRunner also has a similar enhancement in its own pathToLocalResource.  IIRC ORWT will use WKTR for testing WK2 (how many acronyms can you fit into a sentence?).