[webkit-reviews] review denied: [Bug 45140] [Qt] V8 port for QT platform: webkit project files changes : [Attachment 66431] webkit project files changes

[bugzilla-daemon at webkit.org]

Laszlo Gombos <laszlo.1.gombos at nokia.com> has denied Vlad <vladbph at gmail.com>'s
request for review:
Bug 45140: [Qt] V8 port for QT platform: webkit project files changes
https://bugs.webkit.org/show_bug.cgi?id=45140

Attachment 66431: webkit project files changes
https://bugs.webkit.org/attachment.cgi?id=66431&action=review

------- Additional Comments from Laszlo Gombos <laszlo.1.gombos at nokia.com>
> Index: ChangeLog
> ===================================================================
> --- ChangeLog (revision 66692)
> +++ ChangeLog (working copy)
> @@ -1,3 +1,17 @@
> +2010-09-02  Vlad Burlik  <volodimir.burlik at nokia.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   [Qt] V8 port for QT platform: webkit project files changes
> +	   https://bugs.webkit.org/show_bug.cgi?id=45140 
> +
> +	   Modify webkit project files to include V8 javascript engine. 
> +	   By default disabled.
> +
> +	   * WebKit.pri:
> +	   * WebKit.pro:
> +	   * common.pri:
> +
>  2010-09-01  Ryuan Choi  <ryuan.choi at samsung.com>
>  
>	   Reviewed by Antonio Gomes.
> Index: common.pri
> ===================================================================
> +QTJAVASCRIPTENGINE = $$(QTJAVASCRIPTENGINE)

Looks good.

> +QTJAVASCRIPTENGINEREV = $$(QTJAVASCRIPTENGINEREV)

I think we should use V8_DIR instead.
>  
> +contains(QTJAVASCRIPTENGINE, V8) {
> +    DEFINES *= V8_BINDING=1
> +    DEFINES *= WTF_USE_V8=1
> +    !win32-*:DEFINES += USING_V8_SHARED

I think these defines are only needed by WebCore, so it should not be in
common.pri. Move it to WebCore.pro.

> Index: WebKit.pri
> ===================================================================
> --- WebKit.pri	(revision 66692)
> +++ WebKit.pri	(working copy)
> @@ -1,5 +1,19 @@
>  # Include file to make it easy to include WebKit into Qt projects
>  
> +# Include common.pri to determine JS engine
> +include(common.pri)
> +
> +contains(QTJAVASCRIPTENGINE, V8) {
> +    unix:!symbian {
> +	   DEFINES += WTF_CHANGES=1
> +	   V8_CFLAGS += -fno-strict-aliasing
> +	   V8_CFLAGS += -fvisibility=hidden
> +	   QMAKE_CFLAGS += $$V8_CFLAGS
> +	   QMAKE_CXXFLAGS += $$V8_CFLAGS
> +    }
> +}
> +

I do not think these are needed to build WebKit, remove them.

> +
>  # Detect that we are building as a standalone package by the presence of
>  # either the generated files directory or as part of the Qt package through
>  # QTDIR_build
> Index: WebKit.pro
> ===================================================================
> --- WebKit.pro	(revision 66692)
> +++ WebKit.pro	(working copy)
> @@ -14,7 +14,7 @@
>  contains(QT_CONFIG, declarative) {
>      exists($$PWD/WebKit/qt/declarative): SUBDIRS += WebKit/qt/declarative
>  }
> -exists($$PWD/JavaScriptCore/jsc.pro): SUBDIRS += JavaScriptCore/jsc.pro
> +!contains(QTJAVASCRIPTENGINE, V8):exists($$PWD/JavaScriptCore/jsc.pro):
SUBDIRS += JavaScriptCore/jsc.pro

This part looks good.