[webkit-reviews] review denied: [Bug 16401] [GTK] GObject/C DOM binding : [Attachment 29670] patch updated to revision 42662

[bugzilla-daemon at webkit.org]

Mark Rowe (bdash) <mrowe at apple.com> has denied Xan Lopez
<xan.lopez at gmail.com>'s request for review:
Bug 16401: [GTK] GObject/C DOM binding
https://bugs.webkit.org/show_bug.cgi?id=16401

Attachment 29670: patch updated to revision 42662
https://bugs.webkit.org/attachment.cgi?id=29670&action=review

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
>  # 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.