[webkit-reviews] review denied: [Bug 37369] [GTK] Enable building whatever already exists of WebKit2 : [Attachment 71887] WebKit2 GTK Patch after rework

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Amruth Raj
<amruthraj at motorola.com>'s request for review:
Bug 37369: [GTK] Enable building whatever already exists of WebKit2
https://bugs.webkit.org/show_bug.cgi?id=37369

Attachment 71887: WebKit2 GTK Patch after rework
https://bugs.webkit.org/attachment.cgi?id=71887&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71887&action=review

Phwew! This is a big patch. Thanks for contributing it. I've reviewed the build
changes so far. Hopefully I'll find some time later to look over the rest of
the patch.

> GNUmakefile.am:163
> +# WebKit2
> +if ENABLE_WEBKIT2
> +
> +libJavaScriptCore_ladir =
$(prefix)/include/webkit2- at WEBKITGTK_API_VERSION@/JavaScriptCore
> +libJavaScriptCore_la_HEADERS = $(javascriptcore_h_api)
> +
> +lib_LTLIBRARIES += \
> +    
libwebkit2gtk- at WEBKITGTK_API_MAJOR_VERSION@. at WEBKITGTK_API_MINOR_VERSION@.la
> +
>
+nodist_EXTRA_libwebkit2gtk_ at WEBKITGTK_API_MAJOR_VERSION@_ at WEBKITGTK_API_MINOR_
VERSION at _la_SOURCES = \
> +	$(webcore_built_nosources)
> +
>
+nodist_libwebkit2gtk_ at WEBKITGTK_API_MAJOR_VERSION@_ at WEBKITGTK_API_MINOR_VERSIO
N at _la_SOURCES = \
> +	DerivedSources/WebKit2/WebKit2Prefix.h \
> +	$(webcore_built_sources) \
> +	$(webkit2_built_sources)
> +
>
+libwebkit2gtk_ at WEBKITGTK_API_MAJOR_VERSION@_ at WEBKITGTK_API_MINOR_VERSION@_ladi
r = $(prefix)/include/webkit2- at WEBKITGTK_API_VERSION@/webkit2

Instead of repeating all of these source and flag lists, is it possible to
create a variable in configure.ac and use it much like
WEBKITGTK_API_MAJOR_VERSION?

> JavaScriptCore/GNUmakefile.am:-54
> -	JavaScriptCore/API/APICast.h \

Why have you removed these headers from the source list?

> JavaScriptCore/GNUmakefile.am:-71
> -	JavaScriptCore/API/JSRetainPtr.h \

Same question.

> JavaScriptCore/GNUmakefile.am:-76
> -	JavaScriptCore/API/OpaqueJSString.h \

And here.

> WebCore/ChangeLog:8
> +	   No new tests. (OOPS!)

This line should be replaced with an explaination of what sort of testing is
possible, if at all.

> WebCore/ChangeLog:14
> +	   * GNUmakefile.am: Changes to enable disable building of
WebCore/bindings/gobject when building WebKit2.
> +	   This is required since WebCore/bindings/gobject access
WebKit/gtk/webkit files, which are skipped for WebKit2.
> +	   * platform/network/soup/cache/webkit/soup-cache.h: Changes to stop
including <webkit/webkitdefines.h>
> +	   when building WebKit2. Instead the macro WEBKIT_API is defined
directly in soup-cache.h. <webkit/webkitdefines.h>
> +	   is including only when building WebKit1. This is done as entire dir
WebKit/gtk/webkit is skipped when building

Talking to Xan today, it seems that the bindings/gobject files only include
webkitdefines.h for the WEBKIT_API visibility markers. Can you try to simply
eliminate the webkitdefines.h include and reproduce WEBKIT_API in
bindings/gobject. That would eliminate a lot of the conditional stuff below.

> WebCore/GNUmakefile.am:-2916
> -	WebCore/storage/IDBIndexBackendInterface.h \

Why remove this?

> WebCore/GNUmakefile.am:-2927
> -	WebCore/storage/IDBObjectStoreBackendInterface.h \

And this?

> WebCore/platform/network/soup/cache/webkit/soup-cache.h:6
> + * Portions Copyright (c) 2010 Motorola Mobility, Inc.  Except as provided
below, all rights reserved.

It's fine for your copyright message to omit "Portions" and "Except as provided
below,". If I understand correctly, these are implied.

> WebKit2/GNUmakefile.am:481
> +# Copy header files to DerivedSources

I guess I feel that 'include' makes sense as the destination instead of
DerivedSources/WebKit2/include. These files aren't generated, they are just
copied.

> WebKit2/GNUmakefile.am:483
> +copy_wc_headers := $(filter %.h, $(webcore_sources))
> +webcore-headers: $(copy_wc_headers)

This is sort of the reverse of what I would expect. I think the rule should
have the verb. Something like this:

webcore_headers := $(filter %.h, $(webcore_sources))
copy-webcore-headers: $(webcore_headers)

I have this preference for the rest of these copying rules too.

> WebKit2/GNUmakefile.am:538
> +	$(WebKit2)/UIProcess/Plugins \
> +    $(WebKit2)/WebProcess/Plugins \
> +	$(WebKit2)/WebProcess/WebPage \

The indentation is a little funky here.

> WebKit2/GNUmakefile.am:541
> +vpath %.messages.in = $(MESSAGE_INPUT_PATH)

Is it possible to list these directly in the vpath directive instead of
spinning them off into a separate variable?

> WebKit2/GNUmakefile.am:543
> +MESSAGE_GEN_SCRIPT = \

I'd rather this be in lower-case since it's not global. Something like
message_gen_scripts.

> WebKit2/GNUmakefile.am:592
> +# .PHONY: include-headers

I think you need to add all of the header copying targets to the .PHONY rule.