[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 #50 from Ravi Phaneendra Kasibhatla <ravi.kasibhatla at motorola.com>  2010-11-02 09:38:05 PST ---

(In reply to comment #44)
> (From update of attachment 72201 [details])
> 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.
Done.
> 
> > 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.
Done.
> 
> > GNUmakefile.am:84
> > +webkit2_built_nosources :=
> 
> I don't think this variable needs to exist. See below.
Done.
> 
> > 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?
This is not possible. The older automake targets points to targets stamp-po etc which are defined in WebKit/gtk/po/GNUmakefile.am folder. Since, we are excluding the entire WebKit/ folder itself for WebKit2, the inclusion of this rule is not possible.
> 
> > 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?
Done.
> 
> > 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.
Done.
> 
> > 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?
Done.
> 
> > 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.
Done.
> 
> > 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?
Done.
> 
> > 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.
Done.
> 
> > 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.
Done.

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