Bug 65470

Created attachment 103173 [details] Example to re-enable visibility This patch depends on #27551 introducing USE(EXPORT_MACROS) which would fix this but would normally also export more symbols than we want/need for Qt. X86 build (using -2 on the build) 35499072 2011-08-07 04:38 libQtWebKit.so.4.10.0.before 29595673 2011-08-07 06:52 lib/libQtWebKit.so.4.10.0.after

I think this applies to QtWebKit2.2 as well and I wonder if this is a show stopper for the release. I'm not sure which parts of WebKit2 (Testing utilities) link against libQtWebKit.so (instead of libWebCore directly). I am not really sure how to move forward for QtWebKit2.2, for trunk it is probably waiting for bug #27551.

Created attachment 103181 [details] Reduce symbols (force enabling of export macros) X86: 29616930 2011-08-07 07:48 lib/libQtWebKit.so.4.10.0 this patch requires the patch from bug #65826.

(In reply to comment #3) > Created an attachment (id=103181) [details] > Reduce symbols (force enabling of export macros) > > X86: > 29616930 2011-08-07 07:48 lib/libQtWebKit.so.4.10.0 > > this patch requires the patch from bug #65826. There's something wrong with this patch, it's not ASCII/cleartext, it contains lots of escape sequences.

(In reply to comment #2) > I think this applies to QtWebKit2.2 as well and I wonder if this is a show stopper for the release. I'm not sure which parts of WebKit2 (Testing utilities) link against libQtWebKit.so (instead of libWebCore directly). > Regarding QtWebKit-2.2, your last patch appears to work and indeed reduces the size of the library (webkit1 only, linux, x86, release): 27M libQtWebKit.so.4.9.0 (no-visibility) 31M libQtWebKit.so.4.9.0 (full) > I am not really sure how to move forward for QtWebKit2.2, for trunk it is probably waiting for bug #27551. Since WebKit2 is not supported in QtWebKit-2.2, I think just the patch from this bug should suffice, but I may be missing something. I'm adding it to the "nice-to have" meta bug for now.

(In reply to comment #4) > There's something wrong with this patch, it's not ASCII/cleartext, it contains lots of escape sequences. Oh sorry this is coming from git diff > /tmp/diff. I can create a patch with the escape sequences removed.

(In reply to comment #1) > Created an attachment (id=103173) [details] > Example to re-enable visibility > > This patch depends on #27551 introducing USE(EXPORT_MACROS) which would fix this but would normally also export more symbols than we want/need for Qt. Holger: The latest patch attached to bug 27551 doesn't contain any references to EXPORT_MACROS. Are you sure it's still needed? Regarding WebKit1 (QtWebKit-2.2), would it make sense to land your patch AS IS?

(In reply to comment #7) > (In reply to comment #1) > > Created an attachment (id=103173) [details] [details] > > Example to re-enable visibility > > > > This patch depends on #27551 introducing USE(EXPORT_MACROS) which would fix this but would normally also export more symbols than we want/need for Qt. > > Holger: The latest patch attached to bug 27551 doesn't contain any references to EXPORT_MACROS. Are you sure it's still needed? Regarding WebKit1 (QtWebKit-2.2), would it make sense to land your patch AS IS? Export macros landed as part of r81135 but are turned off on the transition phase.

(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #1) > > > Created an attachment (id=103173) [details] [details] [details] > > > Example to re-enable visibility > > > > > > This patch depends on #27551 introducing USE(EXPORT_MACROS) which would fix this but would normally also export more symbols than we want/need for Qt. > > > > Holger: The latest patch attached to bug 27551 doesn't contain any references to EXPORT_MACROS. Are you sure it's still needed? Regarding WebKit1 (QtWebKit-2.2), would it make sense to land your patch AS IS? > > Export macros landed as part of r81135 but are turned off on the transition phase. So my understanding is that the only thing currently blocking this for trunk is bug 65826 (which is WK2 related). In other words, it could be applied to 2.2 AS IS, right? (I'm not saying I want to apply it, just asking if there's anything missing)

(In reply to comment #9) > So my understanding is that the only thing currently blocking this for trunk is bug 65826 (which is WK2 related). In other words, it could be applied to 2.2 AS IS, right? > > (I'm not saying I want to apply it, just asking if there's anything missing) Yes, it could be applied as is.

Created attachment 105353 [details] Reduce symbols (force enabling of export macros) [ascii version] The same patch as before, but without the ansi escape chars.

Created attachment 105357 [details] Patch for qtwebkit-2.2

This appears to be related to this QT bug: https://bugreports.qt.nokia.com/browse/QTBUG-20556

Comment on attachment 105357 [details] Patch for qtwebkit-2.2 rs=me for 2.2

Patch added to qtwebkit-2.2 with commit 16cbdf3 <http://gitorious.org/webkit/qtwebkit/commit/16cbdf3>

(In reply to comment #15) > Patch added to qtwebkit-2.2 with commit 16cbdf3 <http://gitorious.org/webkit/qtwebkit/commit/16cbdf3> Unfortunately the patch broke most plugin layout tests: http://build.webkit.sed.hu/results/QtWebKit2.2-branch%20x86-32%20Linux%20Release%20Qt%204.8.x/rfb3b4700aa5abbb2db471b4f0eb7946ce4850cc4%20%28184%29/results.html After a few tests, I figured this is the culprit: WebKit.pro: contains(QT_CONFIG, reduce_exports):CONFIG += hide_symbols Any objections to the removal of this line but keeping the rest of the patch? (BTW, I couldn't find documentation about this CONFIG option... any pointers are welcome).

(In reply to comment #16) > (In reply to comment #15) > > Patch added to qtwebkit-2.2 with commit 16cbdf3 <http://gitorious.org/webkit/qtwebkit/commit/16cbdf3> > > Unfortunately the patch broke most plugin layout tests: > http://build.webkit.sed.hu/results/QtWebKit2.2-branch%20x86-32%20Linux%20Release%20Qt%204.8.x/rfb3b4700aa5abbb2db471b4f0eb7946ce4850cc4%20%28184%29/results.html > > After a few tests, I figured this is the culprit: > > WebKit.pro: > contains(QT_CONFIG, reduce_exports):CONFIG += hide_symbols > > Any objections to the removal of this line but keeping the rest of the patch? BTW, as far as library size is concerned, there's no advantage in keeping the other changes. Withoug hide_symbols, the library size ends up the same as before the patch was applied.

Since the original change broke several tests and after removing CONFIG += hide_symbols (that fixes the tests) we're back to the original library size, I'm reverting the whole patch in 2.2 (I need the bots green). If it makes sense to add a partial patch, please let me know.

Okay, I am sadly on vacation with a netbook. I will try to propose another patch for webkit 2.2.x. doc: It is complicated. It starts with the Qt config tests for the visibility that add reduce symbols to the IIRC features.pri (just grep inside the QMAKESPEC directory). The other part is the magic of mapping the Qt config option to a NAME.prf (e.g common/features/unix/hide_symbols.prf)

(In reply to comment #19) > Okay, I am sadly on vacation with a netbook. I will try to propose another patch for webkit 2.2.x. > Just a heads up: QtWebKit-2.2-RC1 is planned for this week (today or tomorrow).

Not part of 2.2, so no need to block bug 32653.

This is fixed in trunk actually with bug #60951 and it's still the case after man y build system changes :)