Qt DRT should translate file:///tmp/LayoutTests paths into a local URL that's derived from the path of the DRT application exec.
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 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()
(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.
Created attachment 105896 [details] Proposed DRT Patch Reviewer's comments
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://").
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
Created attachment 105939 [details] Proposed DRT Patch QLatin1Strings
(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 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.
I'll check it and verify today evening.
(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.
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.
(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.
Ossy did you get a few moments to verify? Thanks.
Oooops, I forgot it. I'll check it immediately.
(In reply to comment #15) > Oooops, I forgot it. I'll check it immediately. No worries! Just checking in :)
Comment on attachment 105939 [details] Proposed DRT Patch LGTM, r=me
Comment on attachment 105939 [details] Proposed DRT Patch Clearing flags on attachment: 105939 Committed r94676: <http://trac.webkit.org/changeset/94676>
All reviewed patches have been landed. Closing bug.
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)
(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!
Ossy, is this good to close this again?
(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.
Revision r94676 cherry-picked into qtwebkit-2.2 with commit 4f9120c <http://gitorious.org/webkit/qtwebkit/commit/4f9120c>
(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?).