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.")
> 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.
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
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.
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.
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.
> 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?
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.
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.
2011-02-07 07:51 PST, Kristian Amlie
2011-02-07 07:51 PST, Kristian Amlie
2011-02-07 07:52 PST, Kristian Amlie
2011-02-07 07:53 PST, Kristian Amlie
2011-02-07 07:53 PST, Kristian Amlie
2011-02-08 00:14 PST, Kristian Amlie
2011-02-10 02:29 PST, Kristian Amlie
2011-02-15 02:47 PST, Kristian Amlie
2011-02-17 07:35 PST, Kristian Amlie
2011-02-18 00:29 PST, Kristian Amlie
2011-02-22 03:56 PST, Kristian Amlie
2011-02-28 01:19 PST, Kristian Amlie
2011-02-28 01:20 PST, Kristian Amlie