Currently we build the lib in one pass from the jscore static lib, WebCore and the API. For WebKit2 we followed that policy so we created a static lib target to link into the final lib. Obviously, that does not match the real dependencies between the components so we end up in weird problems: https://bugs.webkit.org/show_bug.cgi?id=50519, https://bugs.webkit.org/show_bug.cgi?id=50861#c12. I think we should solve this by adapting the build targets to the dependencies.
My idea is to build a static lib from JSC, WebCore and WebKit2 and then creating the shared lib from the static libs and the sources of the API (with or without the WK2 API). What do you think?
I like this idea. In fact this is exactly how we're building WebKit2 for Symbian at the moment as the "whole-archive" machinery to expose symbols does not seems to be available for the current Symbian toolchain.
Created attachment 78354 [details] first try This approach is slightly simpler than proposed earlier in https://bugs.webkit.org/show_bug.cgi?id=51339#c1. In the attached patch there is no separate .pro file for building WebCore library, instead that is combined with building the final WebKit library. With this patch (and some other patches under review) WebKit2 links with the Symbian toolchain.
Comment on attachment 78354 [details] first try View in context: https://bugs.webkit.org/attachment.cgi?id=78354&action=review > Source/WebCore/WebCore.pro:192 > + > + HEADERS += \ > + $$WK2_DIR/UIProcess/API/C/qt/WKNativeEvent.h \ Our WebCore.pro is becoming too polluted... here we are adding more WebKit2 stuff in WebCore, we want to move away from that.
(In reply to comment #1) > My idea is to build a static lib from JSC, WebCore and WebKit2 and then > creating the shared lib from the static libs and the sources of the API (with or without the WK2 API). > What do you think? Good plan.
(In reply to comment #5) > (In reply to comment #1) > > My idea is to build a static lib from JSC, WebCore and WebKit2 and then > > creating the shared lib from the static libs and the sources of the API (with or without the WK2 API). > > What do you think? > > Good plan. Fine. bbandix has been started working on the this, currently he is waiting for the source tree reorganizing to be finished.
Comment on attachment 78354 [details] first try Canceling patch review, bbandix can take it from here.
(In reply to comment #7) > (From update of attachment 78354 [details]) > Canceling patch review, bbandix can take it from here. Thanks. I agree with Kenneth that WebCore.pro is getting too chaotic that's why we discussed a new approach with Balazs. In the coming weeks the remaining directories should move to Source, then I'll resume working on this.
Created attachment 82353 [details] Patch
The patch needs a clean build, since the whole build system got revamped. Adding WebKit2 API files to the final build phase would be dangerous because of name clashes between WebCore and WebKit2 sources and headers, so I left the WebKit2 build as it was and link to the static lib with whole-archive. I hope this also works on Symbian, if not, then we need to export symbols.
Attachment 82353 [details] did not build on qt: Build output: http://queues.webkit.org/results/7908697
Created attachment 82452 [details] patch part1 After a discussion with ossy I split the patch. So this is now the first part, which moves code generator rules to CodeGenerators.pri and makes WebCore.pri to a project include file for commonly needed options.
Comment on attachment 82452 [details] patch part1 View in context: https://bugs.webkit.org/attachment.cgi?id=82452&action=review Good cleanup! r=me. Please do a clean build on the buildbots if it is necessary. > Source/WebCore/CodeGenerators.pri:655 > +inspectorBackendStub.wkAddOutputToSources = false Please add a comment to ChangeLog about this change before landing. > Source/WebCore/CodeGenerators.pri:662 > +injectedScriptSource.wkAddOutputToSources = false ditto > Source/WebCore/CodeGenerators.pri:671 > +tokenizer.wkAddOutputToSources = false ditto
Comment on attachment 82452 [details] patch part1 Thanks Ossy. First part landed in http://trac.webkit.org/changeset/78566
Created attachment 82486 [details] patch part2 The second part. This also needs a clean build.
> Adding WebKit2 API files to the final build phase would be dangerous because of name clashes between WebCore and WebKit2 sources and headers, so I left the WebKit2 build as it was and link to the static lib with whole-archive. > > I hope this also works on Symbian, if not, then we need to export symbols. I do not think this will work for Symbian, but perhaps we can address WebKit2 in a subsequent patch (step 3). I think we should keep building bindings as part of WebCore.pro. What is the rational of moving bindings in QtWebKit.pro ?
(In reply to comment #16) > > Adding WebKit2 API files to the final build phase would be dangerous because of name clashes between WebCore and WebKit2 sources and headers, so I left the WebKit2 build as it was and link to the static lib with whole-archive. > > > > I hope this also works on Symbian, if not, then we need to export symbols. > > I do not think this will work for Symbian, but perhaps we can address WebKit2 in a subsequent patch (step 3). > > I think we should keep building bindings as part of WebCore.pro. What is the rational of moving bindings in QtWebKit.pro ? Moving bindings and bridge to the final build step ensures linking to the jsc static library without getting some symbols stripped away and without using whole-archive. When I left them in WebCore.pro I got linking problems with WebKit2 and minimal builds.
Created attachment 82960 [details] patch part2 v2 I finally managed to also move the WebKit2 API to the final step, and moved bridge and bindings back to WebCore.pro as Laszlo suggested. WebKit2/WebProcess/qt/WebProcessMainQt.cpp had to be moved to the final step as well, so the WK2 and JSC library don't need whole-archive any more. In theory later changes could introduce trouble with the clashing filenames of WebCore and WebKit2 (especially with header includes), but this risk is minimal since only the API is in the final step. This patch should make it possible to also work on the Windows version of Qt WebKit2. This most certainly needs a clean build.
Attachment 82960 [details] did not build on qt: Build output: http://queues.webkit.org/results/7934232
Comment on attachment 82960 [details] patch part2 v2 View in context: https://bugs.webkit.org/attachment.cgi?id=82960&action=review Great! This is what we need. > Source/WebKit/qt/QtWebKit.pro:41 > +webkit2:addWebKit2Lib(../../WebKit2) > + > +addWebCoreLib(../../WebCore) > + > +!v8:addJavaScriptCoreLib(../../JavaScriptCore) Is this the best order? I believe that this determines the location of each unit in the final shared lib and that can affect to performance. Do somebody have an idea what would be the ideal order? > Source/WebKit/qt/QtWebKit.pro:124 > +} else { > + DESTDIR = $$OUTPUT_DIR/lib > + !static: DEFINES += QT_MAKEDLL > + symbian: TARGET =$$TARGET$${QT_LIBINFIX} > +} > + > +unix|win32-g++*:QMAKE_PKGCONFIG_REQUIRES = QtCore QtGui QtNetwork > +unix:!mac:*-g++*:QMAKE_CXXFLAGS += -ffunction-sections -fdata-sections > +unix:!mac:*-g++*:QMAKE_LFLAGS += -Wl,--gc-sections > +linux*-g++*:QMAKE_LFLAGS += $$QMAKE_LFLAGS_NOUNDEF > + > +!static: DEFINES += QT_MAKEDLL Once defined QT_MAKEDLL conditionally and then unconditionally. I guess it should be removed from the else case.
(In reply to comment #20) > (From update of attachment 82960 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=82960&action=review > > Great! This is what we need. > > > Source/WebKit/qt/QtWebKit.pro:41 > > +webkit2:addWebKit2Lib(../../WebKit2) > > + > > +addWebCoreLib(../../WebCore) > > + > > +!v8:addJavaScriptCoreLib(../../JavaScriptCore) > > Is this the best order? I believe that this determines the location of each unit in the final shared lib > and that can affect to performance. Do somebody have an idea what would be the ideal order? > I'm not sure whether this can affect the performance. I can experiment with the order and measure the performance if this is a concern of yours. > > Source/WebKit/qt/QtWebKit.pro:124 > > +} else { > > + DESTDIR = $$OUTPUT_DIR/lib > > + !static: DEFINES += QT_MAKEDLL > > + symbian: TARGET =$$TARGET$${QT_LIBINFIX} > > +} > > + > > +unix|win32-g++*:QMAKE_PKGCONFIG_REQUIRES = QtCore QtGui QtNetwork > > +unix:!mac:*-g++*:QMAKE_CXXFLAGS += -ffunction-sections -fdata-sections > > +unix:!mac:*-g++*:QMAKE_LFLAGS += -Wl,--gc-sections > > +linux*-g++*:QMAKE_LFLAGS += $$QMAKE_LFLAGS_NOUNDEF > > + > > +!static: DEFINES += QT_MAKEDLL > > Once defined QT_MAKEDLL conditionally and then unconditionally. I guess it should be removed from the else case. Good catch, I can correct this when landing. Laszlo, could you take a look whether this approach could work on Symbian too?
(In reply to comment #21) > (In reply to comment #20) > > (From update of attachment 82960 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=82960&action=review > > > > Great! This is what we need. > > > > > Source/WebKit/qt/QtWebKit.pro:41 > > > +webkit2:addWebKit2Lib(../../WebKit2) > > > + > > > +addWebCoreLib(../../WebCore) > > > + > > > +!v8:addJavaScriptCoreLib(../../JavaScriptCore) > > > > Is this the best order? I believe that this determines the location of each unit in the final shared lib > > and that can affect to performance. Do somebody have an idea what would be the ideal order? > > > > I'm not sure whether this can affect the performance. I can experiment with the order and measure the performance if this is a concern of yours. Fine, but it should not block the patch, we can change the order later.
Comment on attachment 82960 [details] patch part2 v2 View in context: https://bugs.webkit.org/attachment.cgi?id=82960&action=review Looks great. I have to say r- because this badly breaks Symbian and because this approach repeats a few build rules (in WebCore.pro and QtWebKit.pro) instead of reused (e.g. WebCore.pri). Also the patch no longer applies against the trunk. > Source/WebCore/WebCore.pro:-25 > - QMAKE_CXXFLAGS.ARMCC += --fpu softvfp+vfpv2 --fpmode fast CFLAGS should stay in WebCore.pro (or perhaps WebCore.pri) - that's where v8 object files are built. > Source/WebCore/WebCore.pro:-56 > - MMP_RULES += ALWAYS_BUILD_AS_ARM I think MMP_RULES should stay in WebCore.pro (or perhaps WebCore.pri) as well. > Source/WebCore/WebCore.pro:-59 > - QMAKE_CXXFLAGS -= --thumb Ditto. > Source/WebCore/WebCore.pro:-61 > - CONFIG(release, debug|release): QMAKE_CXXFLAGS.ARMCC += -OTime -O3 Ditto. > Source/WebCore/WebCore.pro:-77 > - !static: DEFINES += QT_MAKEDLL Removing QT_MAKEDLL define will cause PluginViewSymbian.cpp no longer build - as QWEBKIT_EXPORT will no be defined. qtwebkit_setPluginCreatedCallback is the only exported symbol from WebCore (only on Symbian). I think the change in pro file is OK, however the exported symbol should be moved to QtWebKit.pro. > Source/WebKit/qt/QtWebKit.pro:120 > +unix:!mac:*-g++*:QMAKE_CXXFLAGS += -ffunction-sections -fdata-sections I think this CFLAGS rule should go to WebCore.pri as it should be applied to both WebCore and QtWebKit. >>> Source/WebKit/qt/QtWebKit.pro:124 >>> +!static: DEFINES += QT_MAKEDLL >> >> Once defined QT_MAKEDLL conditionally and then unconditionally. I guess it should be removed from the else case. > > Good catch, I can correct this when landing. > > Laszlo, could you take a look whether this approach could work on Symbian too? Happy to continue to help; Ossy has a working WebKit(1) Symbian environment as well. > Source/WebKit/qt/QtWebKit.pro:161 > + webkitbackup.sources = ../WebKit/qt/symbian/backup_registration.xml This relative path needs to be now relative to QtWebKit.pro - should be "symbian/backup_registration.xml". > Source/WebKit/qt/QtWebKit.pro:166 > + declarativeImport.sources += ../WebKit/qt/declarative/qmldir This relative path needs to be now relative to QtWebKit.pro - should be "declarative/qmldir". > Source/WebKit/qt/QtWebKit.pro:404 > +contains(DEFINES, ENABLE_NETSCAPE_PLUGIN_API=1) { Most of this block should go to WebCore.pri instead. > Source/WebKit/qt/QtWebKit.pro:405 > + unix { WebCore.pro (where this block is copied from) had a symbian block before the unix block - as the symbian block is omitted here and symbian will test true for unix which will break the symbian build. This is a good example why we should not copy these rules into both pro files and instead move them to WebCore.pri. Use unix:!symbian instead. > Source/WebKit/qt/QtWebKit.pro:412 > + DEFINES += MOZ_PLATFORM_MAEMO=5 Should go to WebCore.pri instead of repeating it here. MOZ_PLATFORM_MAEMO is used in npapi.h which is included in both WebCore and QtWebKit directory. > Source/WebKit/qt/QtWebKit.pro:417 > + DEFINES += MOZ_PLATFORM_MAEMO=6 Ditto. > Source/WebKit/qt/QtWebKit.pro:420 > + DEFINES += ENABLE_NETSCAPE_PLUGIN_METADATA_CACHE=1 Ditto. > Source/WebKit/qt/QtWebKit.pro:485 > + } Section above is old. This block is already newer in r78996 which was used to create this patch. This is also a good example why we should move this rules from WebCore.pro into WebCore.pri and not copy them around. > Source/WebKit/qt/QtWebKit.pro:505 > + MOBILITY *= sensors I think these CONFIG and MOBILITY rules should go to WebCore.pri as well. > Source/WebKit/qt/QtWebKit.pro:642 > +*-g++*:QMAKE_CXXFLAGS -= -std=c++0x -std=gnu++0x Should stay in WebCore.pro or moved to WebCore.pri.
Comment on attachment 82960 [details] patch part2 v2 View in context: https://bugs.webkit.org/attachment.cgi?id=82960&action=review > Source/WebCore/WebCore.pro:15 > +DEFINES += BUILDING_WEBCORE This define is not used anywhere I think we should remove this line.
Comment on attachment 82960 [details] patch part2 v2 View in context: https://bugs.webkit.org/attachment.cgi?id=82960&action=review One more.. > Source/WebKit/qt/QtWebKit.pro:414 > + SOURCES += $PWD/WebCoreSupport/QtMaemoWebPopup.cpp Should be $$PWD instead of $PWD. > Source/WebKit/qt/QtWebKit.pro:495 > + $PWD/WebCoreSupport/DeviceOrientationProviderQt.h Ditto. > Source/WebKit/qt/QtWebKit.pro:502 > + $PWD/WebCoreSupport/DeviceOrientationProviderQt.cpp Ditto.
Created attachment 83168 [details] patch part2 v3 Addressed the mentioned issues. Ossy launched a build for Symbian, but since it will last a while I upload the patch now.
Created attachment 83206 [details] patch part2 Corrected the mobility issues, thanks Laszlo for spotting them.
Comment on attachment 83206 [details] patch part2 View in context: https://bugs.webkit.org/attachment.cgi?id=83206&action=review Looks really good. This version of the patch no longer breaks the Symbian WebKit(1) builds. Made a few more comments for consideration. > Source/WebCore/WebCore.pri:162 > + !CONFIG(production):CONFIG-=def_files This line should go to QtWebKit.pro as it is only needed for final linking. > Source/WebKit/qt/QtWebKit.pro:380 > + CONFIG += x11 CONFIG += x11 changes the include path so I think it should go to WebCore.pri. > Source/WebKit/qt/QtWebKit.pro:381 > + LIBS += -lXrender We might want to consider keeping some (or all) LIB rules together (in WebCore.pri) with the corresponding CONFIG or DEFINE rules just to increase the readability (it seems LIBS is ignored for static libs). I do not really have a strong opinion on this one. > Source/WebKit/qt/QtWebKit.pro:400 > +contains(DEFINES, ENABLE_SQLITE=1) { This whole block should be moved to WebCore.pri instead; some of the rules below are manipulating the includepath for files under WebCore. > Source/WebKit/qt/QtWebKit.pro:546 > +win32:!win32-g++*:contains(QMAKE_HOST.arch, x86_64):{ Shouldn't this block stay in WebCore.pro ?
Created attachment 83290 [details] patch part2 v4 Addressed Laszlo's suggestions, WebCore.pri contains now the LIB and CONFIG rules. Thanks Laszlo for the comprehensive review.
Comment on attachment 83290 [details] patch part2 v4 Looks great, r=me.
Committed r79320: <http://trac.webkit.org/changeset/79320>
Comment on attachment 83290 [details] patch part2 v4 Clearing flags.
(In reply to comment #31) > Committed r79320: <http://trac.webkit.org/changeset/79320> Had to do a follow-up (http://trac.webkit.org/changeset/79334), because I screwed up something with webkit-patch.
Andras, this patch broke inspector tests, because somehow qrc_InspectorBackendStub.o isn't linked to libQtWebKit.so :-/ qrc_InspectorBackendStub.o is in libwebcore.a, but missing from so. Reopen to fix this regression.
(In reply to comment #34) > Andras, this patch broke inspector tests, because somehow qrc_InspectorBackendStub.o isn't linked to libQtWebKit.so :-/ > > qrc_InspectorBackendStub.o is in libwebcore.a, but missing from so. > > Reopen to fix this regression. Fix landed in http://trac.webkit.org/changeset/79352. Sorry for the inconvenience.