[webkit-reviews] review denied: [Bug 67254] [Qt][DRT] Normalize file:///tmp/LayoutTests in LayoutTestController::pathToLocalResource() : [Attachment 105837] Proposed DRT Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Jarred Nicholls
<jarred at sencha.com>'s request for review:
Bug 67254: [Qt][DRT] Normalize file:///tmp/LayoutTests in
LayoutTestController::pathToLocalResource()
https://bugs.webkit.org/show_bug.cgi?id=67254

Attachment 105837: Proposed DRT Patch
https://bugs.webkit.org/attachment.cgi?id=105837&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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()