Bug 80996 - [Qt] Add test specific platform plugin to achieve unified layout test results
Summary: [Qt] Add test specific platform plugin to achieve unified layout test results
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Balazs Kelemen
URL:
Keywords: Qt, QtTriaged
Depends on: 85795
Blocks: 80272
  Show dependency treegraph
 
Reported: 2012-03-13 08:59 PDT by Balazs Kelemen
Modified: 2012-09-25 05:55 PDT (History)
7 users (show)

See Also:


Attachments
Patch (25.13 KB, patch)
2012-03-13 10:13 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (25.79 KB, patch)
2012-03-13 10:42 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (25.84 KB, patch)
2012-03-13 13:19 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (32.99 KB, patch)
2012-04-16 06:32 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
updated patch (32.93 KB, patch)
2012-04-23 07:54 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2012-03-13 08:59:26 PDT
This is part of unifying text metric dependent test results across Linux and Mac (can be extended to others in the future).
Comment 1 Balazs Kelemen 2012-03-13 10:13:09 PDT
Created attachment 131656 [details]
Patch
Comment 2 WebKit Review Bot 2012-03-13 10:21:17 PDT
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.
Comment 3 Balazs Kelemen 2012-03-13 10:42:01 PDT
Created attachment 131663 [details]
Patch

forgot to add a file last time
Comment 4 WebKit Review Bot 2012-03-13 10:45:25 PDT
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.
Comment 5 Early Warning System Bot 2012-03-13 11:52:42 PDT
Comment on attachment 131663 [details]
Patch

Attachment 131663 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11941705
Comment 6 Balazs Kelemen 2012-03-13 13:19:10 PDT
Created attachment 131700 [details]
Patch
Comment 7 WebKit Review Bot 2012-03-13 13:22:13 PDT
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.
Comment 8 Balazs Kelemen 2012-03-13 13:30:39 PDT
(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.
Comment 9 Balazs Kelemen 2012-03-14 01:38:03 PDT
An additional qt fix will be is necessary to make the plugin lib loading properly: http://codereview.qt-project.org/#change,19858.
Comment 10 Balazs Kelemen 2012-04-13 04:49:43 PDT
Simon, could you take a look? I already got an lgtm from Tor Arne but he would like you do a pass as well :)
Comment 11 Simon Hausmann 2012-04-13 05:51:48 PDT
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?
Comment 12 Balazs Kelemen 2012-04-16 06:32:43 PDT
Created attachment 137334 [details]
Patch
Comment 13 WebKit Review Bot 2012-04-16 06:36:24 PDT
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.
Comment 14 Early Warning System Bot 2012-04-16 06:36:32 PDT
Comment on attachment 137334 [details]
Patch

Attachment 137334 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12409968
Comment 15 Balazs Kelemen 2012-04-16 06:42:20 PDT
(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.
Comment 16 Balazs Kelemen 2012-04-16 06:46:31 PDT
(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
Comment 17 Balazs Kelemen 2012-04-23 07:54:51 PDT
Created attachment 138349 [details]
updated patch
Comment 18 WebKit Review Bot 2012-04-23 07:58:37 PDT
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.
Comment 19 Simon Hausmann 2012-05-04 11:13:09 PDT
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? :)
Comment 20 Balazs Kelemen 2012-05-07 04:48:29 PDT
Committed r116299: <http://trac.webkit.org/changeset/116299>
Comment 21 Balazs Kelemen 2012-05-07 04:50:44 PDT
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!
Comment 22 Csaba Osztrogonác 2012-05-07 05:06:34 PDT
Reopen, because it broke the build on all bots. Could you fix it?
Comment 23 Csaba Osztrogonác 2012-05-07 05:51:03 PDT
After two buildfix it still fail:

TestIntegration.cpp:(.text+0xb1): undefined reference to `QPlatformIntegrationFactory::create(QString const&, QString const&)'
Comment 24 WebKit Review Bot 2012-05-07 05:54:26 PDT
Re-opened since this is blocked by 85795
Comment 25 Csaba Osztrogonác 2012-05-07 05:58:42 PDT
Rollout landed in http://trac.webkit.org/changeset/116304
Comment 26 Balazs Kelemen 2012-05-07 07:45:49 PDT
(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.
Comment 27 Balazs Kelemen 2012-09-25 05:55:45 PDT
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.