[Webkit-unassigned] [Bug 33590] [GTK] GObject DOM bindings

Sun Jan 31 15:10:02 PST 2010

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

--- Comment #26 from Mark Rowe (bdash) <mrowe at apple.com>  2010-01-31 15:09:59 PST ---
(From update of attachment 46466)
> diff --git a/WebCore/bindings/gobject/WebKitDOMBinding.cpp b/WebCore/bindings/gobject/WebKitDOMBinding.cpp
> new file mode 100644
> index 0000000..10dc6e6
> --- /dev/null
> +++ b/WebCore/bindings/gobject/WebKitDOMBinding.cpp
> @@ -0,0 +1,99 @@
> +// gcc 3.x can't handle including the HashMap pointer specialization
> +// in this file
> +#if defined __GNUC__ && !defined __GLIBCXX__ // less than gcc 3.4
> +#define HASH_MAP_PTR_SPEC_WORKAROUND 1
> +#endif

This isn’t needed any more, and hasn’t done anything useful for a long time.

> +static gpointer createWrapper(Node* node)
> +{
> +    ASSERT(node);
> +    ASSERT(!ScriptInterpreter::getDOMObject(node));
> +
> +    gpointer ret = 0;
> +
> +    switch (node->nodeType()) {
> +    default:
> +        ret = wrapNode(node);
> +    }

“ret” is a poor name for a local variable.  Something like “wrappedNode” would
be more descriptive.  The switch statement with only a default case is odd, but
presumably it was done like that as further patches will add cases to the
switch?  I think it’d be preferable to remove the switch until it’s needed so
that the code doesn’t look so odd.

> +gpointer kit(Node* node)
> +{
> +    if (!node)
> +        return 0;
> +
> +    gpointer ret = DOMObjectCache::get(node);
> +    if (ret)
> +        return ret;

Similar comment about “ret” as above.

> +
> +    return createWrapper(node);
> +}
> +

> diff --git a/WebCore/bindings/gobject/WebKitDOMBinding.h b/WebCore/bindings/gobject/WebKitDOMBinding.h
> new file mode 100644
> index 0000000..025bdba
> --- /dev/null
> +++ b/WebCore/bindings/gobject/WebKitDOMBinding.h
> @@ -0,0 +1,54 @@
> +#ifndef WebKitDOMBinding_h
> +#define WebKitDOMBinding_h
> +
> +#include "Event.h"
> +#include "EventTarget.h"
> +#include "Node.h"
> +#include "WebKitDOMNode.h"
> +#include "WebKitDOMNodePrivate.h"
> +#include <wtf/Noncopyable.h>

I don’t think any of these headers need to be included from this header file.

> +
> +namespace WebCore {
> +class AtomicString;
> +class Event;
> +class Frame;
> +class KURL;
> +class Node;
> +class String;
> +} // namespace WebCore

The only forward-declaration that looks to be needed here is for Node.

> diff --git a/WebCore/bindings/gobject/webkitdomstringconvert.h b/WebCore/bindings/gobject/webkitdomstringconvert.h
> new file mode 100644
> index 0000000..63ea17b
> --- /dev/null
> +++ b/WebCore/bindings/gobject/webkitdomstringconvert.h

The naming of this file isn’t consistent with others in the patch, particular
in the aspect of case.   The WebKit prefix on it feels odd too.  The other
files with this prefix seem to be part of the API, while this is a set of
internal helper functions.

> +
> +#ifndef webkitdomstringconvert_h
> +#define webkitdomstringconvert_h
> +
> +/* convenience functions for converting various webkit string types into a utf glib string */

Comments should be treated as full sentences, and capitalized and punctuated as
such.

> +
> +#include "AtomicString.h"
> +#include "CString.h"
> +#include "KURL.h"
> +#include "PlatformString.h"
> +#include "unicode/ustring.h”

This include of "unicode/ustring.h” does not seem to be used here.  The file
appears to use glib functionality so it would presumably need to include the
relevant headers in order to stand alone.

> +inline char * domStringConvert(WebCore::String const& s) { return g_strdup(s.utf8().data()); }
> +inline char * domStringConvert(WebCore::KURL const& s) { return g_strdup(s.string().utf8().data()); }
> +inline char * domStringConvert(const JSC::UString& s) { return g_strdup(s.UTF8String().c_str()); }
> +inline char * domStringConvert(WebCore::AtomicString const& s) { return g_strdup(s.string().utf8().data()); }

There’s some extra whitespace around the * on these lines.  g_strdup returns
gchar*.  Is there a reason the return types here are char* rather than gchar*? 
The latter seems to better communicate what type the strings are and how they
need to be disposed of.

Do these functions need to be inlined?  They look to be relatively expensive
functions to call by virtue of the memory allocation that they perform, so
inlining would seem to have little performance benefit.  It may be preferable
to move the implementations to a .cpp file so that the number of includes that
are pulled in by this header can be reduced.

I’m not crazy about the name “domStringConvert” either.  It feels like it’s
trying to say “convert to DOM string”, which would be better expressed by a
name like convertToDOMString or toDOMString

> diff --git a/WebCore/bindings/scripts/CodeGeneratorGObject.pm b/WebCore/bindings/scripts/CodeGeneratorGObject.pm
> new file mode 100644
> index 0000000..e9d38ee
> --- /dev/null
> +++ b/WebCore/bindings/scripts/CodeGeneratorGObject.pm

Perl isn’t my most favorite language in the world so it’s probably best for
someone deeply familiar to look over this too.  One thing that does jump out is
the range of inconsistency in variable naming, many of which deviate from the
style guidelines.  Cleaning that up would make it a lot easier for someone with
the style guidelines ingrained in their mind to focus on the substance of the
patch.

> +        # HACK!

This should be replaced with a FIXME explaining what it’s doing, why it’s doing
it, and what it should do in the future instead.

> diff --git a/WebCore/dom/Node.idl b/WebCore/dom/Node.idl
> index c6cd4b9..19546be 100644
> --- a/WebCore/dom/Node.idl
> +++ b/WebCore/dom/Node.idl
> @@ -61,7 +61,9 @@ module core {
>          readonly attribute Node             previousSibling;
>          readonly attribute Node             nextSibling;
>          readonly attribute NamedNodeMap     attributes;
> +#if !defined(LANGUAGE_GOBJECT) || !LANGUAGE_GOBJECT
>          readonly attribute Document         ownerDocument;
> +#endif
>  
>          [OldStyleObjC, Custom] Node insertBefore(in [Return] Node newChild,
>                                                   in Node refChild)
> @@ -132,6 +134,7 @@ module core {
>  #endif /* defined(LANGUAGE_OBJECTIVE_C) */
>  
>  #if !defined(LANGUAGE_OBJECTIVE_C) || !LANGUAGE_OBJECTIVE_C
> +#if !defined(LANGUAGE_GOBJECT) || !LANGUAGE_GOBJECT
>          [Custom] void addEventListener(in DOMString type, 
>                                         in EventListener listener, 
>                                         in boolean useCapture);
> @@ -141,6 +144,7 @@ module core {
>          boolean dispatchEvent(in Event event)
>              raises(EventException);
>  #endif
> +#endif
>      };
>  
>  }

I don’t see anything in the ChangeLog about why these modifications are here. 
If there’s a good reason for them it really needs to be documented so that
people looking at the code in the future know why they’re there and whether
they can remove them.

Speaking of which… you appear to have two ChangeLog entries in
WebCore/ChangeLog.

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