[Webkit-unassigned] [Bug 37369] [GTK] Enable building whatever already exists of WebKit2

Tue Oct 26 08:52:47 PDT 2010

https://bugs.webkit.org/show_bug.cgi?id=37369

Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #71887|review?                     |review-
               Flag|                            |

--- Comment #30 from Martin Robinson <mrobinson at webkit.org>  2010-10-26 08:52:43 PST ---
(From update of attachment 71887)
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@_la_SOURCES = \
> +	$(webcore_built_nosources)
> +
> +nodist_libwebkit2gtk_ at WEBKITGTK_API_MAJOR_VERSION@_ at WEBKITGTK_API_MINOR_VERSION@_la_SOURCES = \
> +	DerivedSources/WebKit2/WebKit2Prefix.h \
> +	$(webcore_built_sources) \
> +	$(webkit2_built_sources)
> +
> +libwebkit2gtk_ at WEBKITGTK_API_MAJOR_VERSION@_ at WEBKITGTK_API_MINOR_VERSION@_ladir = $(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.