[Webkit-unassigned] [Bug 16401] [GTK] GObject/C DOM binding

[bugzilla-daemon at webkit.org]

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





------- Comment #184 from jim at yorba.org  2009-03-09 18:43 PDT -------
(In reply to comment #183)
> (From update of attachment 28381 [review])
> Can we rename the build-related GOBJECT wording to GDOM i.e. GENSOURCESGOBJECT
> -> GENSOURCESGDOM ?

I don't think this makes sense for a couple of reasons.  First, the general
consensus is that we're moving away from GDOM as a prefix in the code, and that
logically carries over to the makefile.  Second, the other bindings are named
after the language they are for (i.e. JS, ObjC, etc.)  GObject is the best,
all-encompassing name for these new bindings.

> >  # Script for creating hash tables
> > @@ -103,7 +104,8 @@
> >  	-I$(srcdir)/JavaScriptCore/ForwardingHeaders \
> >  	-I$(srcdir)/JavaScriptCore/parser \
> >  	-I$(srcdir)/JavaScriptCore/wtf \
> > -	-I$(top_builddir)/DerivedSources
> > +	-I$(top_builddir)/DerivedSources \
> > +	-I$(srcdir)/WebCore/bindings/webkit
> 
> No need to include .../bindings/webkit to the JSC build.

Right.

> > +webcoregtk_cppflags += \
> > +	-I$(top_builddir)/DerivedSources/webkit \
> > +	-I$(srcdir)/WebCore/bindings \
> > +	-I$(srcdir)/WebCore/bindings/gobject
> > +
> 
> Please add the lines above to webcoregtk_cppflags in WebCore/GNUmakefile.am

The flags need to be here for autogeneration and compilation.

> Let's name the libtool archive to libWebCoreGDOM.la

> Ditto and libwebcoregdom_h_api.Also declare libwebcoregdom_h_api before
> libgobject_ladir above.

Again, I believe GObject's a better choice.

> > @@ -360,12 +372,14 @@
> >  	-I$(srcdir)/WebKit/gtk \
> >  	-I$(srcdir)/WebKit/gtk/WebCoreSupport \
> >  	-I$(srcdir)/WebKit/gtk/webkit \
> > -	-I$(top_builddir)/WebKit/gtk/webkit
> > +	-I$(top_builddir)/WebKit/gtk/webkit \
> > +	-I$(top_builddir)/DerivedSources
> 
> Add DerivedSources/webkit here too.

Right.

> > @@ -466,6 +526,7 @@
> >  # Build unit test
> >  noinst_PROGRAMS += Programs/UnitTests
> >  Programs_UnitTests_CPPFLAGS = \
> > +	-I$(srcdir)/WebCore/bindings \
> 
> I don't think we need to include it.

Right.

> > @@ -494,7 +555,8 @@
> >  	$(webcore_built_sources) \
> >  	$(webcore_built_nosources) \
> >  	$(webkitgtk_built_sources) \
> > -	$(webkitgtk_built_nosources)
> > +	$(webkitgtk_built_nosources) \
> > +	$(gobject_built_nosources)
> 
> gdom_built_sources.

As explained above.

> >  webcore_cppflags += \
> > +	-I$(srcdir)/WebKit/gtk \
> > +	-I$(srcdir)/WebKit/gtk/WebCoreSupport \
> > +	-I$(srcdir)/WebKit/gtk/webkit \
> Do we need to include WebKit here?

Yes.

> > @@ -57,6 +61,7 @@
> >  	DerivedSources/DocTypeStrings.cpp \
> >  	DerivedSources/tokenizer.cpp \
> >  	DerivedSources/ColorData.c \
> > +	DerivedSources/webkit/webkitdomdummy.c \
> 
> This should go to gdom_built_nosources and please add it to BUILT_SOURCES too

Again, as explained above.

> > @@ -240,6 +245,14 @@
> >  	WebCore/xml/XMLSerializer.idl \
> >  	WebCore/xml/XSLTProcessor.idl
> >  
> > +webcoregtk_sources += \
> > +	WebCore/bindings/gobject/webkitbinding.cpp \
> > +	WebCore/bindings/gobject/webkitbinding.h \
> > +	WebCore/bindings/gobject/WebKitDOMObject.cpp \
> > +	WebCore/bindings/gobject/WebKitDOMObject.h \
> > +	WebCore/bindings/gobject/webkithtmlelementwrapperfactory.cpp \
> > +	WebCore/bindings/gobject/webkithtmlelementwrapperfactory.h 
> 
> Please add the sources above to webcoregtk_sources in line ~1730
> 
> Kindly rename GOBJECT to GDOM accordingly from here on (i.e. GOBJECT_AUTO_...,
> GOBJECT_FIXED_..., GOBJECT_CLASSES, etc...)

As explained above.

> > +# All IDL files used for autogeneration in GObject.  We filter out SVG and events for now.
> 
> Please add a FIXME re SVG and events ^^^ - as we might need to add it later 

Right.

> > +gobject_built_nosources := $(GOBJECT_HEADERS_BUILT) $(GOBJECT_SOURCES_BUILT)
> 
> This should be _built_sources not _built_nosources.

Why?

> Please include the license header here.

Added.

> Why are the following lines prefixed with "webkit/"? We're already including
> DerivedSources/webkit anyway.

When headers are installed in the distribution, they'll wind up in the webkit/
directory (along with the WebKit/GTK header files).  This is how the pathing
worked in the earlier GTK code; we're just following along.

> >  # GtkLauncher
> >  Programs_GtkLauncher_CPPFLAGS = \
> > +	-I$(srcdir)/WebCore/bindings \
> >  	-I$(srcdir)/WebKit/gtk \
> >  	-I$(top_builddir)/WebKit/gtk \
> > +	-I$(srcdir)/WebKit/gtk/webkit \
> 
> I don't think we need to add those two above.

Required for compilation.

> 
> > @@ -31,8 +33,10 @@
> >  Programs_DumpRenderTree_CPPFLAGS = \
> >  	-I$(srcdir)/WebKitTools/DumpRenderTree \
> >  	-I$(srcdir)/WebKitTools/DumpRenderTree/gtk \
> > +	-I$(srcdir)/WebCore/bindings \
> >  	-I$(srcdir)/WebKit/gtk \
> >  	-I$(top_builddir)/WebKit/gtk \
> > +	-I$(srcdir)/WebKit/gtk/webkit \
> 
> Ditto.

Required for compilation.

The ones I've marked "right" or "added" are straighforward enough, but it's not
like the code doesn't build and execute without them.  I'd like to get more
input on this patch before I perform another one with these changes.

-- Jim


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