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

Mon Feb 1 05:39:53 PST 2010

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

--- Comment #27 from Xan Lopez <xan.lopez at gmail.com>  2010-02-01 05:39:49 PST ---
(In reply to comment #26)
> > +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.

That's exactly the case, as I told Gustavo. Since you are the second one to ask
I've changed this into an if as you suggest, we can make it a switch again in
the future.

> > 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.

Right, when I was intending to make this stuff used all over the place I
renamed it to ConvertToUTF8String.h, which I think reflects better what it's
about. I don't plan to make it public again, but I think we should go with that
name anyway.

> > +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.

gchar* and char* are exactly the same thing, one is just a typedef for the
other. The functions return UTF8-encoded NULL-terminated char arrays, that need
to be freed manually.

> 
> 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.

OK, makes sense to me. I guess the intent here was to "keep things the same",
since this operation is usually done inline elsewhere in the code.

> 
> 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

I'd say the DOM name is simply wrong here, since what it does is simply go from
WebKit string types to UTF8-encoded C-strings that glib wants. So, as I said,
I'll go with convertToUTF8String if it sounds ok.

> 
> > 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.

OK, I'll give it another go. I'm not aware of any official perl style guide for
webkit, so I'll just try to copy as best as I can the style in the other
scripts.

> > 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.

I've explained where this comes from in a previous comment in the bug, but you
are right in saying that it should be mentioned in the ChangeLog.

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

Yup. :)

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