[Webkit-unassigned] [Bug 16401] [GTK] GObject/C DOM binding
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Apr 22 04:17:29 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=16401
mrowe at apple.com changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #29670|review? |review-
Flag| |
------- Comment #191 from mrowe at apple.com 2009-04-22 04:17 PDT -------
(From update of attachment 29670)
> # 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.
> +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.
> 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.
> +#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).
> ===================================================================
> --- 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 <>?
> +#include "webkit/webkitdomdefines.h"
> +#include <webkit/webkitdefines.h>
Why does one of these includes use "" and the other <>?
> +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.
> Index: WebCore/bindings/gobject/gstringconvert.h
This file name doesn't fit our naming conventions.
> ===================================================================
> --- 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?
> +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, and the function names are using lower case with underscores.
> 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, 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.
> +namespace WebKit {
This seems a bit out of place inside WebCore, but perhaps it is intentional.
> +#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.
> +
> +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.
> 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.
> +
> +#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.
> 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. ]
> 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.
> 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.
I'm going to mark this as r- for now and will try to review the remaining file
shortly.
--
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.
More information about the webkit-unassigned
mailing list