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

[bugzilla-daemon at webkit.org]

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





--- Comment #44 from Martin Robinson <mrobinson at webkit.org>  2010-10-29 13:12:30 PST ---
(From update of attachment 72201)
View in context: https://bugs.webkit.org/attachment.cgi?id=72201&action=review

Looks good, but I think the build file changes can be simplified even more.

> GNUmakefile.am:74
> +webcoregtk_gdom_sources :=
> +webcoregtk_gdom_cppflags :=

I don't think these are necessary.

> GNUmakefile.am:82
> +webkit2gtk_static_h_api :=
> +webkit2_cppflags :=
> +webkit2gtk_cppflags :=

GTK+ is the only autotools user and probably will be for the future. I think we can get rid of  webkit2gtk_sources and webkit2gtk_cppflags.

> GNUmakefile.am:84
> +webkit2_built_nosources :=

I don't think this variable needs to exist. See below.

> GNUmakefile.am:827
> +if !ENABLE_WEBKIT2
>  # Older automake versions (1.7) place Plo files in a different place so we need
>  # to create the output directory manually.
>  all-local: stamp-po
>  	$(mkdir_p) $(top_builddir)/$(DEPDIR)/DerivedSources
> +endif #!ENABLE_WEBKIT2

Why is this work-around for older versions of automake conditional on the WebKit2 build?

> WebCore/GNUmakefile.am:88
> +webcoregtk_gdom_cppflags += \

I don't understand the need for webcoregtk_gdom_cppflags here. Why not just add it directly to webcoregtk_cppflags?

> WebCore/GNUmakefile.am:3651
> +webcoregtk_gdom_sources += \

Please just add these directly to webcoregtk_sources and scrap the webcoregtk_gdom_sources variable. I think keeping the number of makefile variables low is key to keeping it understandable.

> WebCore/platform/network/soup/cache/webkit/soup-cache.h:30
> +#if !defined(ENABLE_WEBKIT2_BUILD)
>  #if BUILDING_GTK__
>  #include <webkit/webkitdefines.h>
>  #else

Instead of a workaround for WebKit2, why not just define WEBKIT_API unconditionally to simplify the #ifdefs?

> WebKit2/GNUmakefile.am:3
> +ForwardingHeaders := $(GENSOURCES_WEBKIT2)/include
> +HttpStack := @HTTP_STACK@

If these need to be in a make variable, please move them down near where you use them.

> WebKit2/GNUmakefile.am:9
> +	$(srcdir)/WebKit2/Shared/API/c/WKArray.h \
> +	$(srcdir)/WebKit2/Shared/API/c/WKBase.h \
> +	$(srcdir)/WebKit2/Shared/API/c/WKCertificateInfo.h \
> +	$(srcdir)/WebKit2/Shared/API/c/WKData.h \

Why not use the $(WebKit2) variable defined above for this source list?

> WebKit2/GNUmakefile.am:401
> +	$(webkit2_built_nosources)

I think I'd rather see generate-forwarding-headers here explicitly rather than having a webkit2_built_nosources variable.

> WebKit2/GNUmakefile.am:471
> +vpath %.messages.in = $(WebKit2)/PluginProcess $(WebKit2)/UIProcess $(WebKit2)/UIProcess/Plugins $(WebKit2)/WebProcess/Plugins $(WebKit2)/WebProcess/WebPage $(WebKit2)/WebProcess

Please split this across multiple lines, if possible.

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