Bug 80996 – [Qt] Add test specific platform plugin to achieve unified layout test results

• Component: Tools / Tests
• Status: RESOLVED INVALID

[Balazs Kelemen]

This is part of unifying text metric dependent test results across Linux and Mac (can be extended to others in the future).

[Balazs Kelemen]

Created attachment 131656 [details]
Patch

[WebKit Review Bot]

Attachment 131656 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Tools/QtTestPlatformPlugin/TestIntegration.cpp:1:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kelemen]

Created attachment 131663 [details]
Patch

forgot to add a file last time

[WebKit Review Bot]

Attachment 131663 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Tools/QtTestPlatformPlugin/TestIntegration.cpp:1:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 131663 [details]
Patch

Attachment 131663 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11941705

[Balazs Kelemen]

Created attachment 131700 [details]
Patch

[WebKit Review Bot]

Attachment 131700 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Tools/QtTestPlatformPlugin/TestIntegration.cpp:1:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kelemen]

(In reply to comment #7)
> Attachment 131700 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
> Tools/QtTestPlatformPlugin/TestIntegration.cpp:1:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
> Total errors found: 1 in 16 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

We already have a bug for this: https://bugs.webkit.org/show_bug.cgi?id=65372. check-webkit-style should not complain about config.h outside of Sources.

[Balazs Kelemen]

An additional qt fix will be is necessary to make the plugin lib loading properly: http://codereview.qt-project.org/#change,19858.

[Balazs Kelemen]

Simon, could you take a look? I already got an lgtm from Tor Arne but he would like you do a pass as well :)

[Simon Hausmann]

Comment on attachment 131700 [details]
Patch

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?

[Balazs Kelemen]

Created attachment 137334 [details]
Patch

[WebKit Review Bot]

Attachment 137334 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Tools/QtTestPlatformPlugin/TestIntegration.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 137334 [details]
Patch

Attachment 137334 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12409968

[Balazs Kelemen]

(In reply to comment #11)
> (From update of attachment 131700 [details])
> 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.

Oops. Ok, for the parts where I reused Qt code I have added a slightly modified version of the original Nokia copyright sections. I added u-szeged as holder, removed reference to licence files that we don't have checked in and made it more WebKitish in style. For the rest of the files I used the u-szeged style BSD licence.

> 
> > 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.

Fixed.

> 
> > 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") :)

This doesn't work since QStringLiteral is not a type but a define :)
I have just put this to the function where it is used.

> 
> > 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.

Fixed.

> 
> > 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?

Fixed.

[Balazs Kelemen]

(In reply to comment #14)
> (From update of attachment 137334 [details])
> Attachment 137334 [details] did not pass qt-wk2-ews (qt):
> Output: http://queues.webkit.org/results/12409968

Oops. I forgot to remove the -Wl,--no-undefined from the pro file. Let me postpone it before landing. Btw the plugin will not work before this export fix lands in Qt: https://codereview.qt-project.org/#change,19858

[Balazs Kelemen]

Created attachment 138349 [details]
updated patch

[WebKit Review Bot]

Attachment 138349 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Tools/QtTestPlatformPlugin/TestIntegration.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Comment on attachment 138349 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138349&action=review

r=me. Land with care :)

> Tools/DumpRenderTree/qt/main.cpp:132
> +static void initializeTestPlatformPlugin(int argc, char* argv[])

I suppose char* argv[] should be char* argv[] const? :)

[Balazs Kelemen]

Committed r116299: <http://trac.webkit.org/changeset/116299>

[Balazs Kelemen]

Comment on attachment 138349 [details]
updated patch

Could somebody review the fix for qt: https://codereview.qt-project.org/#change,19858?
Without that this patch is not usable.
Thanks!

[Csaba Osztrogonác]

Reopen, because it broke the build on all bots. Could you fix it?

[Csaba Osztrogonác]

After two buildfix it still fail:

TestIntegration.cpp:(.text+0xb1): undefined reference to `QPlatformIntegrationFactory::create(QString const&, QString const&)'

[WebKit Review Bot]

Re-opened since this is blocked by 85795

[Csaba Osztrogonác]

Rollout landed in http://trac.webkit.org/changeset/116304

[Balazs Kelemen]

(In reply to comment #23)
> After two buildfix it still fail:
> 
> TestIntegration.cpp:(.text+0xb1): undefined reference to `QPlatformIntegrationFactory::create(QString const&, QString const&)'

Sorry for that. Last time I tried it did build even without the qt patch (that makes QPlatformIntegrationFactory exported) because it's a lib. I am going to reland after the fix land to qt.

[Balazs Kelemen]

This patch would definitely need an update, but unfortunately I could not make it produce the result we want. I'm not 100% sure but it seems like the new Liberation fonts has some differencies system wide so even if we use the same font files we don't get the same metrics. For this reason I don't really think it is worth to go with this work further.