[webkit-reviews] review denied: [Bug 51339] [Qt] Redesign the build system : [Attachment 82960] patch part2 v2

[bugzilla-daemon at webkit.org]

Laszlo Gombos <laszlo.1.gombos at nokia.com> has denied Andras Becsi
<abecsi at webkit.org>'s request for review:
Bug 51339: [Qt] Redesign the build system
https://bugs.webkit.org/show_bug.cgi?id=51339

Attachment 82960: patch part2 v2
https://bugs.webkit.org/attachment.cgi?id=82960&action=review

------- Additional Comments from Laszlo Gombos <laszlo.1.gombos at nokia.com>
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.