[Webkit-unassigned] [Bug 16401] [GTK] GObject/C DOM binding
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Jul 12 12:26:18 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=16401
--- Comment #216 from Luke Kenneth Casson Leighton <lkcl at lkcl.net> 2009-07-12 12:26:13 PDT ---
(In reply to comment #191)
> (From update of attachment 29670 [details])
> > # Script for creating hash tables
> > @@ -110,7 +111,8 @@ javascriptcore_cppflags += \
> > -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
>
> This and other changes to the include search path are a little bit of a concern
> due to the layering violations they imply. JavaScriptCore should not depend on
> headers in WebCore. WebCore should not depend on headers from WebKit.
no, it should not, i absolutely agree.
however, i tried very hard to make builds work without adding these in,
and got exasperated enough to give up trying and just moved forward.
core to the issue is that webkitwebframe.h now absolutely requires
GdomDOMDocument, GdomNode, GdomXMLHttpRequest, GdomEventListener and
a few others, and obviously their dependencies.
so that's a _massive_ influx of extra header files, all of which are
auto-generated and go into DerivedSources.
so... exactly how should those derived sources, on which WebKitWebFrame
is now dependent, get into the build, on which libwebkit now depends,
and on which GtkLauncher etc. now also depend?
it's a tricky one, and i look forward to this one being resolved _after_
the patch is landed.
> > +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
> > +
>
> It looks like some of these files do not follow the naming convention. This
> should be addressed.
they follow the naming conventions in the patch that i will submit
after i have replied to #191 and #194.
> > Index: WebCore/bindings/gobject/WebKitDOMObject.cpp
> > ===================================================================
> > --- WebCore/bindings/gobject/WebKitDOMObject.cpp (revision 0)
> > +++ WebCore/bindings/gobject/WebKitDOMObject.cpp (revision 0)
> > @@ -0,0 +1,28 @@
> > +/*
> > + * Copyright (C) 2008 Luke Kenneth Casson Leighton <lkcl at lkcl.net>
> > + * Copyright (C) 2008 Martin Soto <soto at freedesktop.org>
> > + * Copyright (C) 2008 Alp Toker <alp at atoker.com>
> > + * Copyright (C) 2008 Apple Inc. All Rights Reserved.
> > + */
>
> This file needs a license header.
done.
>
> > +#include <glib-object.h>
> > +
> > +#include "config.h"
> > +#include "webkitbinding.h"
> > +#include "WebKitDOMObject.h"
>
> These includes should follow the order in the style guidelines (config.h,
> WebKitDOMObject.cpp, blank line, remaining includes in sorted order).
done.
>
> > ===================================================================
> > --- WebCore/bindings/gobject/WebKitDOMObject.h (revision 0)
> > +++ WebCore/bindings/gobject/WebKitDOMObject.h (revision 0)
> > @@ -0,0 +1,56 @@
> > +/*
> > + * Copyright (C) 2008 Luke Kenneth Casson Leighton <lkcl at lkcl.net>
> > + * Copyright (C) 2008 Martin Soto <soto at freedesktop.org>
> > + * Copyright (C) 2008 Alp Toker <alp at atoker.com>
> > + * Copyright (C) 2008 Apple Inc. All Rights Reserved.
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Library General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * Library General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Library General Public License
> > + * along with this library; see the file COPYING.LIB. If not, write to
> > + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> > + * Boston, MA 02110-1301, USA.
> > + */
> > +
> > +
> > +#ifndef WebKitDOMObject_h
> > +#define WebKitDOMObject_h
> > +
> > +#include "glib-object.h"
>
> If this header is included here, why is it also included in
> WebKitDOMObject.cpp? Why does one place use "" while the other uses <>?
i have nooo idea. probably because adam fixed one but not the other.
> > +#include "webkit/webkitdomdefines.h"
> > +#include <webkit/webkitdefines.h>
>
> Why does one of these includes use "" and the other <>?
hmm, that doesn't look right.... hmmm, it's also... out-of-date.
> > +struct _WebKitDOMObject {
> > + GObject parent_instance;
> > +
> > + void *coreObject;
> > +};
> > +
> > +struct _WebKitDOMObjectClass {
> > + GObjectClass parent_class;
> > +};
> > +
>
> Some consistency on the naming style on these member variables would be nice.
yes, it would... *sigh* these are partly-autogenerated i.e. from an
early revision of the autogenerator i copied an auto-generated file
and created GdomDOMObject.cpp from it.
so... *hand-waving*... there is a convention of sorts - from the bindings
and also from glib/gobject coding....
conclusion: not going to worry about this one right now.
>
>
> > Index: WebCore/bindings/gobject/gstringconvert.h
>
> This file name doesn't fit our naming conventions.
yep adam renamed it. it'll be GStringConvert.h
> > ===================================================================
> > --- WebCore/bindings/gobject/gstringconvert.h (revision 0)
> > +++ WebCore/bindings/gobject/gstringconvert.h (revision 0)
> > @@ -0,0 +1,37 @@
> > +/*
> > + * Copyright (C) 2008 Luke Kenneth Casson Leighton <lkcl at lkcl.net>
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Library General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * Library General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Library General Public License
> > + * along with this library; see the file COPYING.LIB. If not, write to
> > + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> > + * Boston, MA 02110-1301, USA.
> > + */
> > +
> > +
> > +#ifndef GSTRING_CONVERT_H
> > +#define GSTRING_CONVERT_H
> > +
> > +/* convenience functions for converting various webkit string types into a utf8 glib string */
>
> WebKit.
>
> > +#include "AtomicString.h"
> > +#include "CString.h"
> > +#include "KURL.h"
> > +#include "PlatformString.h"
> > +#include "unicode/ustring.h"
>
> Why is the unicode/ustring.h include necessary here?
it was added in a series of random attempts to get these string conversion
routines to work. i believe that one of these gdom_gstring_converts works
_because_ unicode/ustring.h is there, and there is an implicit c++ type
conversion going on.
> > +inline gchar * gdom_gstring_convert(WebCore::String const& s) { return g_strdup(s.utf8().data()); }
> > +inline gchar * gdom_gstring_convert(WebCore::KURL const& s) { return g_strdup(s.string().utf8().data()); }
> > +inline gchar * gdom_gstring_convert(const JSC::UString & s) { return g_strdup(s.UTF8String().c_str()); }
> > +inline gchar * gdom_gstring_convert(WebCore::AtomicString const&s) { return g_strdup(s.string().utf8().data()); }
>
> This doesn't follow our coding style. The spacing around operators is
> inconsistent,
well spotted.
> and the function names are using lower case with underscores.
adam changed that. the patch i will resubmit has GdomGstringConvert.
> > Index: WebCore/bindings/gobject/webkitbinding.cpp
> > ===================================================================
> > --- WebCore/bindings/gobject/webkitbinding.cpp (revision 0)
> > +++ WebCore/bindings/gobject/webkitbinding.cpp (revision 0)
> > @@ -0,0 +1,367 @@
>
> The file name doesn't fit our convention,
[adam again]
does in the patch to be submitted.
> and the include order in this file is
> a bit whacky. It should be updated to follow the style guidelines, with the
> duplicate headers removed. It may make sense to keep the generated headers in
> a separate block, but the others should follow the style.
added an explanation. not looking forward to shuffling 100 #includes.
/* yes these are out-of-order, but they are grouped by
* purpose. the #includes needed for toGDOM(Document*)
* got added, and the ones for toGDOM(CSSRule*) got added
* separately, and then some are conditional based on
* compile-time options, and yet more will be added
* e.g. for SVG Canvas.
*/
>
> > +namespace WebKit {
>
> This seems a bit out of place inside WebCore, but perhaps it is intentional.
yes, it is. it allows the toGDOM functions to be referred to as
WebKit::toGDOM when they are used.
> > +#include <wtf/Noncopyable.h>
> > +
> > +#include "CanvasPixelArray.h"
> > +#include "CSSRule.h"
> > +#include "CSSValue.h"
> > +#include "Document.h"
> > +#include "DOMWindow.h"
> > +#include "Element.h"
> > +#include "HTMLCollection.h"
> > +#include "Node.h"
> > +#include "StyleSheet.h"
> > +#include "Text.h"
>
> Similar comment about includes here.
looks out-of-date. GDOMBinding.cpp doesn't have wtf/Noncopyable.h
> > +
> > +namespace WebCore {
> > +
> > + class AtomicString;
> > + class Document;
> > + class Event;
> > + class Frame;
> > + class KURL;
> > + class Node;
> > + class String;
>
> Given some of the files that are included above, many of these forward
> declarations are redundant.
i'm not seeing those in GDOMBinding.cpp so that must have been adam,
so this is now out-of-date.
> > Index: WebCore/bindings/gobject/webkithtmlelementwrapperfactory.cpp
> > ===================================================================
> > --- WebCore/bindings/gobject/webkithtmlelementwrapperfactory.cpp (revision 0)
> > +++ WebCore/bindings/gobject/webkithtmlelementwrapperfactory.cpp (revision 0)
>
> Is there some way we can autogenerate this file? It seems like it is almost
> entirely boilerplate.
yes. but not right now. it was copied from the auto-generated
JSHTMLElementWrapperFactory.cpp and then i removed all references
to exec*.
perl not being my favourite language i couldn't face doing yet another
adaptation of yet another perl script.
so - one for a separate patch, after this patch has been landed.
> > +
> > +#include "glib-object.h"
> > +#include <wtf/Forward.h>
> > +#include "HTMLElement.h"
> > +
> > +namespace WebCore {
> > + class HTMLElement;
> > +}
>
> Including HTMLElement.h then forward-declaring HTMLElement is a bit redundant.
nooot going to worry about it.
> > Index: WebCore/bindings/scripts/CodeGeneratorGObject.pm
> > ===================================================================
> > --- WebCore/bindings/scripts/CodeGeneratorGObject.pm (revision 0)
> > +++ WebCore/bindings/scripts/CodeGeneratorGObject.pm (revision 0)
>
> [ I'm skipping this file for now in hopes that I'll be able to convince Sam to
> look at it. ]
he he
> > Index: WebCore/bindings/scripts/gobject-generate.pl
> > ===================================================================
> > --- WebCore/bindings/scripts/gobject-generate.pl (revision 0)
> > +++ WebCore/bindings/scripts/gobject-generate.pl (revision 0)
>
> It may make more sense for this to be named in the same order as the other
> "generate" script in that directory.
[adam again]. keeping it as CodeGeneratorGObject.pl
>
> > Index: WebKitTools/GNUmakefile.am
> > ===================================================================
> > --- WebKitTools/GNUmakefile.am (revision 42662)
> > +++ WebKitTools/GNUmakefile.am (working copy)
> > @@ -4,8 +4,10 @@ noinst_PROGRAMS += \
> >
> > # GtkLauncher
> > Programs_GtkLauncher_CPPFLAGS = \
> > + -I$(srcdir)/WebCore/bindings \
> > -I$(srcdir)/WebKit/gtk \
> > -I$(top_builddir)/WebKit/gtk \
> > + -I$(srcdir)/WebKit/gtk/webkit \
> > $(global_cppflags) \
> > $(javascriptcore_cppflags)
> >
> > @@ -29,8 +31,10 @@ Programs_GtkLauncher_LDADD = \
> > 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 \
> > $(global_cppflags) \
> > $(javascriptcore_cppflags)
> >
>
> DumpRenderTree should only be using API. It should not have any dependencies
> on WebCore.
yep. i know. see first reply, again. tried building without, and it just
doesn't happen. resolving it is definitely a post-patch separate
self-contained task.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list