|
Description
Kristian Amlie
2011-02-07 07:40:11 PST
Created attachment 81472 [details]
Added-full-webkit-module-profile-and-a-syncqt-profile
Created attachment 81473 [details]
Added-include-paths-for-QtScript
Created attachment 81474 [details]
Avoided-UiTools-dependency-if-the-module-is-not-present
Created attachment 81475 [details]
Updated-include-paths-for-phonon
Created attachment 81476 [details]
Made-sure-that-the-build-webkit-qmake-argument-is-always-respected
P1 since important for release process of Qt. Kristian: good job at making patches. Do all of that work with and without modularization? One patch per bug is generally preferred. Comment on attachment 81473 [details]
Added-include-paths-for-QtScript
rs=me.
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. Created attachment 81608 [details]
Updated-include-paths-for-phonon-v2
Comment on attachment 81608 [details]
Updated-include-paths-for-phonon-v2
Should $$QMAKE_LIBDIR_QT change as well ?
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. Comment on attachment 81473 [details] Added-include-paths-for-QtScript Clearing flags on attachment: 81473 Committed r78013: <http://trac.webkit.org/changeset/78013> 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.
Created attachment 82432 [details]
Updated-include-paths-for-phonon-v4
> 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 82800 [details]
Updated-include-paths-for-phonon-v5
Updated patch after IRC discussion with Laszlo.
Comment on attachment 82800 [details]
Updated-include-paths-for-phonon-v5
r=me.
Comment on attachment 82800 [details] Updated-include-paths-for-phonon-v5 Clearing flags on attachment: 82800 Committed r78834: <http://trac.webkit.org/changeset/78834> 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. 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> 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.
Oh, and btw multimedia is not included among the optional dependencies because it is part of mobility, not Qt. Created attachment 84029 [details]
Added-full-webkit-module-profile-and-a-syncqt-profile-v3
Rebased patch.
Created attachment 84030 [details]
Made-sure-that-the-build-webkit-qmake-argument-is-always-respected-v2
Rebased patch.
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> 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> All reviewed patches have been landed. Closing bug. *** Bug 37211 has been marked as a duplicate of this bug. *** |