Bug 53916 – [Qt] WebKit patches required to work with a modularized version of Qt

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Kristian Amlie]

These patches are required in order to get WebKit to compile after Qt has been modularized (see http://labs.qt.nokia.com/2011/01/21/status-of-qt-modularization/).

All patches should work also when compiling against the monolithic version of Qt.

[Kristian Amlie]

Created attachment 81472 [details]
Added-full-webkit-module-profile-and-a-syncqt-profile

[Kristian Amlie]

Created attachment 81473 [details]
Added-include-paths-for-QtScript

[Kristian Amlie]

Created attachment 81474 [details]
Avoided-UiTools-dependency-if-the-module-is-not-present

[Kristian Amlie]

Created attachment 81475 [details]
Updated-include-paths-for-phonon

[Kristian Amlie]

Created attachment 81476 [details]
Made-sure-that-the-build-webkit-qmake-argument-is-always-respected

[Benjamin Poulain]

P1 since important for release process of Qt.

Kristian: good job at making patches. Do all of that work with and without modularization?

[Eric Seidel (no email)]

One patch per bug is generally preferred.

[Eric Seidel (no email)]

Comment on attachment 81473 [details]
Added-include-paths-for-QtScript

rs=me.

[Eric Seidel (no email)]

Comment on attachment 81475 [details]
Updated-include-paths-for-phonon

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

> Source/WebCore/ChangeLog:10
> +        No new tests. (OOPS!)

You'll need to remove thsi line (generally replacing it with explaination of testing or why testing is impossible.  In this case you can just write something like "build fix, no tests.")

[Kristian Amlie]

> Do all of that work with and without modularization?

I tested with both versions, so they should work. We will probably need a couple of more patches down the line though, but this is a start.

> One patch per bug is generally preferred.

Ok. I didn't know about the one-patch-per-bug policy. Is it ok to go forward with this one, and I will remember it for next time?

> You'll need to remove thsi line (generally replacing it with explaination of testing or why testing is impossible.  In this case you can just write something like "build fix, no tests.")

Right. I guess I trusted the prepare-Changelog script too blindly. I'll fix it.

[Kristian Amlie]

Created attachment 81608 [details]
Updated-include-paths-for-phonon-v2

[Laszlo Gombos]

Comment on attachment 81608 [details]
Updated-include-paths-for-phonon-v2

Should $$QMAKE_LIBDIR_QT change as well ?

[Laszlo Gombos]

Comment on attachment 81474 [details]
Avoided-UiTools-dependency-if-the-module-is-not-present

These tools build without UiTools. There is a mechanism to control the UiTools dependency in WebKit.pri.

Would the following work ? 

-disable_uitools: DEFINES *= QT_NO_UITOOLS
+contains(QT_CONFIG, modular):!contains(QT_CONFIG, uitools)|disable_uitools: DEFINES *= QT_NO_UITOOLS

[Laszlo Gombos]

Comment on attachment 81472 [details]
Added-full-webkit-module-profile-and-a-syncqt-profile

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

> Source/WebKit/qt/qt_webkit_version.pri:10
> +QT.webkit.depends = core gui network script

Only the mandatory dependencies needs to be listed here ? What about optional dependencies like multimedia ? 

I think this list should also be aligned with the list in QMAKE_PKGCONFIG_REQUIRES (in WebCore.pro), which at the moment does not contain QtScript.

[WebKit Commit Bot]

Comment on attachment 81473 [details]
Added-include-paths-for-QtScript

Clearing flags on attachment: 81473

Committed r78013: <http://trac.webkit.org/changeset/78013>

[Kristian Amlie]

Created attachment 81941 [details]
Updated-include-paths-for-phonon-v3

> Should $$QMAKE_LIBDIR_QT change as well ?

Oops, my bad. I accidentally submitted an old version of the patch. This one is the one I meant to submit.

[Laszlo Gombos]

Comment on attachment 81941 [details]
Updated-include-paths-for-phonon-v3

r- as the I think $$QMAKE_LIBDIR_QT should change as well.

Perhaps we can set $$QT.XX.includes in WebKit.pri depending on the contains(QT_CONFIG, modular) test.

[Kristian Amlie]

Created attachment 82432 [details]
Updated-include-paths-for-phonon-v4

[Kristian Amlie]

> r- as the I think $$QMAKE_LIBDIR_QT should change as well.

Included in new patch.

> Perhaps we can set $$QT.XX.includes in WebKit.pri depending on the contains QT_CONFIG, modular) test.

This turned out to not be so trivial. The includes are inside a scope "contains(DEFINES, ENABLE_VIDEO=1)", however the define is given by the features.pri profile which is inside the WebCore directory. I'm not intimately familiar with how the building on WebKit works, but I assume that it is kind of bad to make WebKit.pri depend on something that is only present inside one of its subprojects. Or do you disagree?

[Kristian Amlie]

Created attachment 82800 [details]
Updated-include-paths-for-phonon-v5

Updated patch after IRC discussion with Laszlo.

[Laszlo Gombos]

Comment on attachment 82800 [details]
Updated-include-paths-for-phonon-v5

r=me.

[WebKit Commit Bot]

Comment on attachment 82800 [details]
Updated-include-paths-for-phonon-v5

Clearing flags on attachment: 82800

Committed r78834: <http://trac.webkit.org/changeset/78834>

[Kristian Amlie]

Created attachment 82929 [details]
Avoided-UiTools-dependency-if-the-module-is-not-present-v2

> Would the following work ? 

> -disable_uitools: DEFINES *= QT_NO_UITOOLS
> +contains(QT_CONFIG, modular):!contains(QT_CONFIG, uitools)|disable_uitools: DEFINES *= QT_NO_UITOOLS

Yes, that should work.

[WebKit Commit Bot]

Comment on attachment 82929 [details]
Avoided-UiTools-dependency-if-the-module-is-not-present-v2

Clearing flags on attachment: 82929

Committed r79072: <http://trac.webkit.org/changeset/79072>

[Kristian Amlie]

Created attachment 83295 [details]
Added-full-webkit-module-profile-and-a-syncqt-profile-v2

After discussing both with Laszlo and my own team, this is the updated patch. The depends statement has changed to include all optional modules, and is used by the Qt build system to order the link arguments correctly. If a module is missing it will simply be omitted from the link as usual.

The modules mentioned inside the sync.profile stay as they are, since they express which headers are needed, and the optional modules are not part of that.

[Kristian Amlie]

Oh, and btw multimedia is not included among the optional dependencies because it is part of mobility, not Qt.

[Kristian Amlie]

Created attachment 84029 [details]
Added-full-webkit-module-profile-and-a-syncqt-profile-v3

Rebased patch.

[Kristian Amlie]

Created attachment 84030 [details]
Made-sure-that-the-build-webkit-qmake-argument-is-always-respected-v2

Rebased patch.

[WebKit Commit Bot]

Comment on attachment 84030 [details]
Made-sure-that-the-build-webkit-qmake-argument-is-always-respected-v2

Clearing flags on attachment: 84030

Committed r79846: <http://trac.webkit.org/changeset/79846>

[WebKit Commit Bot]

Comment on attachment 84029 [details]
Added-full-webkit-module-profile-and-a-syncqt-profile-v3

Clearing flags on attachment: 84029

Committed r79848: <http://trac.webkit.org/changeset/79848>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

*** Bug 37211 has been marked as a duplicate of this bug. ***