This is part of unifying text metric dependent test results across Linux and Mac (can be extended to others in the future).
Created attachment 131656 [details] Patch
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.
Created attachment 131663 [details] Patch forgot to add a file last time
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 on attachment 131663 [details] Patch Attachment 131663 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11941705
Created attachment 131700 [details] Patch
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.
(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.
An additional qt fix will be is necessary to make the plugin lib loading properly: http://codereview.qt-project.org/#change,19858.
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 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?
Created attachment 137334 [details] Patch
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 on attachment 137334 [details] Patch Attachment 137334 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12409968
(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.
(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
Created attachment 138349 [details] updated patch
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 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? :)
Committed r116299: <http://trac.webkit.org/changeset/116299>
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!
Reopen, because it broke the build on all bots. Could you fix it?
After two buildfix it still fail: TestIntegration.cpp:(.text+0xb1): undefined reference to `QPlatformIntegrationFactory::create(QString const&, QString const&)'
Re-opened since this is blocked by 85795
Rollout landed in http://trac.webkit.org/changeset/116304
(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.
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.