[webkit-reviews] review denied: [Bug 80996] [Qt] Add test specific platform plugin to achieve unified layout test results : [Attachment 131700] Patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Balazs Kelemen
<kbalazs at webkit.org>'s request for review:
Bug 80996: [Qt] Add test specific platform plugin to achieve unified layout
test results
https://bugs.webkit.org/show_bug.cgi?id=80996

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

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=131700&action=review


In general this looks good. I have a few nitpicks/comments and the bigger
remaining issue is that none of the new files have an appropriate license and
copyright header.

> Source/WebKit2/qt/MainQt.cpp:44
> +    QByteArray pluginPath = qgetenv("QT_WEBKIT2_TEST_PLATFORM_PLUGIN_PATH");

> +    if (pluginPath.isEmpty())
> +	   return;
> +
> +    QPluginLoader loader(QString::fromLatin1(pluginPath.data()));

The value of the environment variable is a path, so instead of
QString::fromLatin1 you should use QFile::decodeName.

> Tools/QtTestPlatformPlugin/mac/TestFontDatabase.mm:47
> +const char* defaultWebKitTestFontFamily = "Nimbus Sans L";

This declares a variable that points to an array of read-only chars, but the
variable itself is still read-write (and therefore ends up in the data segment
with relocations). I suggest to simply use

const QStringLiteral defaultWebKitTestFontfamily("Nimbus Sans L") :)

> Tools/QtTestPlatformPlugin/mac/TestFontDatabase.mm:54
> +void TestFontDatabase::populateFontDatabase()

A whole bunch of code in this function is copied from Qt code. This needs to be
reflected in the copyright and at least a more verbose comment.

> Tools/QtTestPlatformPlugin/main.cpp:24
> +    if (system.toLower() == QStringLiteral("testplatform"))

Just a nitpick, but please use

    if (system.compare(QStringLiteral("testplatform"), Qt::CaseInsensitive) ==
0)

to avoid the unnecessary conversion and allocation to lower-case.

> Tools/QtTestPlatformPlugin/main.cpp:38
> +void TestIntegrationPlugin::initialize()
> +{
> +    QStaticPlugin plugin;
> +    plugin.instance = &qt_plugin_instance;
> +    plugin.metaData = &qt_plugin_query_metadata;
> +    qRegisterStaticPluginFunction(plugin);
> +}

Interesting idea... :)

One thought regarding the use of QMetaObject::invoke to call this function: Why
not use Q_CONSTRUCTOR_FUNCTION instead to ensure the
initialization/registration?