[webkit-reviews] review denied: [Bug 67254] [Qt][DRT] Normalize file:///tmp/LayoutTests in LayoutTestController::pathToLocalResource() : [Attachment 105896] 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 105896: Proposed DRT Patch
https://bugs.webkit.org/attachment.cgi?id=105896&action=review

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