Bug 67254 - [Qt][DRT] Normalize file:///tmp/LayoutTests in LayoutTestController::pathToLocalResource()
Summary: [Qt][DRT] Normalize file:///tmp/LayoutTests in LayoutTestController::pathToLo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: LayoutTestFailure, Qt
Depends on: 67784
Blocks: 67239
  Show dependency treegraph
 
Reported: 2011-08-30 20:14 PDT by Jarred Nicholls
Modified: 2011-09-13 07:23 PDT (History)
4 users (show)

See Also:


Attachments
Proposed DRT Patch (2.98 KB, patch)
2011-08-31 15:03 PDT, Jarred Nicholls
benjamin: review-
Details | Formatted Diff | Diff
Proposed DRT Patch (2.88 KB, patch)
2011-08-31 22:30 PDT, Jarred Nicholls
benjamin: review-
Details | Formatted Diff | Diff
Proposed DRT Patch (2.91 KB, patch)
2011-09-01 04:21 PDT, Jarred Nicholls
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jarred Nicholls 2011-08-30 20:14:09 PDT
Qt DRT should translate file:///tmp/LayoutTests paths into a local URL that's derived from the path of the DRT application exec.
Comment 1 Jarred Nicholls 2011-08-31 15:03:33 PDT
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.
Comment 2 Benjamin Poulain 2011-08-31 15:17:40 PDT
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()
Comment 3 Jarred Nicholls 2011-08-31 17:10:18 PDT
(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.
Comment 4 Jarred Nicholls 2011-08-31 22:30:55 PDT
Created attachment 105896 [details]
Proposed DRT Patch

Reviewer's comments
Comment 5 Benjamin Poulain 2011-09-01 03:36:58 PDT
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://").
Comment 6 Benjamin Poulain 2011-09-01 03:39:09 PDT
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
Comment 7 Jarred Nicholls 2011-09-01 04:21:40 PDT
Created attachment 105939 [details]
Proposed DRT Patch

QLatin1Strings
Comment 8 Jarred Nicholls 2011-09-01 04:34:04 PDT
(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.
Comment 9 Andras Becsi 2011-09-02 04:47:43 PDT
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.
Comment 10 Csaba Osztrogonác 2011-09-02 04:53:54 PDT
I'll check it and verify today evening.
Comment 11 Jarred Nicholls 2011-09-02 06:42:13 PDT
(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.
Comment 12 Andras Becsi 2011-09-02 06:54:03 PDT
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.
Comment 13 Jarred Nicholls 2011-09-02 19:18:36 PDT
(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.
Comment 14 Jarred Nicholls 2011-09-07 07:36:56 PDT
Ossy did you get a few moments to verify? Thanks.
Comment 15 Csaba Osztrogonác 2011-09-07 07:44:53 PDT
Oooops, I forgot it. I'll check it immediately.
Comment 16 Jarred Nicholls 2011-09-07 07:46:01 PDT
(In reply to comment #15)
> Oooops, I forgot it. I'll check it immediately.

No worries!  Just checking in :)
Comment 17 Csaba Osztrogonác 2011-09-07 09:59:55 PDT
Comment on attachment 105939 [details]
Proposed DRT Patch

LGTM, r=me
Comment 18 Csaba Osztrogonác 2011-09-07 10:03:24 PDT
Comment on attachment 105939 [details]
Proposed DRT Patch

Clearing flags on attachment: 105939

Committed r94676: <http://trac.webkit.org/changeset/94676>
Comment 19 Csaba Osztrogonác 2011-09-07 10:03:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Csaba Osztrogonác 2011-09-08 09:08:17 PDT
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)
Comment 21 Jarred Nicholls 2011-09-08 09:28:39 PDT
(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!
Comment 22 Jarred Nicholls 2011-09-10 18:20:09 PDT
Ossy, is this good to close this again?
Comment 23 Csaba Osztrogonác 2011-09-13 00:52:01 PDT
(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.
Comment 24 Ademar (unprivileged account) 2011-09-13 06:11:35 PDT
Revision r94676 cherry-picked into qtwebkit-2.2 with commit 4f9120c <http://gitorious.org/webkit/qtwebkit/commit/4f9120c>
Comment 25 Jarred Nicholls 2011-09-13 07:23:19 PDT
(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?).