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

[bugzilla-daemon at webkit.org]

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





--- Comment #15 from Gustavo Noronha (kov) <gns at gnome.org>  2010-01-21 14:44:45 PST ---
(From update of attachment 46466)
 29 2010-01-13  Xan Lopez  <xlopez at igalia.com>
 30 
 31         Reviewed by NOBODY (OOPS!).
 32 
 33         No need to check twice if there's an input file.
 34 
 35         * bindings/scripts/generate-bindings.pl:

I think this is from the other patch =).

 79     switch (node->nodeType()) {
 80     default:
 81         ret = wrapNode(node);
 82     }

This doesn't seem to make sense. Are we going to add case statements here some
day, and that's why this is a switch? I am not opposed, but a comment would be
good.

WebCore/bindings/gobject/webkitdomstringconvert.h -> perhaps this should
actually go inside WebCore/platform/gtk, since it is useful everywhere =).

171 sub GetGlibTypeName {

Why use char* and gchar elsewhere here? We are also using gint, and such here,
it would be good to be consistent

 427     WebKitDOMObject* dom_object = WEBKIT_DOM_OBJECT(object);
 428     
 429     if(dom_object != NULL)

Isn't this impossible? I mean, if dom_object is already NULL, why are we on its
finalize? And the type-safe cast would have failed already =).

 433             WebCore::${interfaceName} *coreObject =
static_cast<WebCore::${interfaceName} *>(dom_object->coreObject);
 434             coreObject->deref();
 435             
 436             dom_object->coreObject = NULL;
 437             
 438             WebKit::DOMObjectCache::forget(coreObject);

The cache doesn't seem to ref the object; can this be its last reference? Would
it be safer to first tell the cache to forget about it, then deref and set the
local variable to NULL?

 680         if ($functionName eq "webkit_dom_xml_http_request_open" &&
 681         $param->name eq "url") {

This line break seems odd

134 136 #if !defined(LANGUAGE_OBJECTIVE_C) || !LANGUAGE_OBJECTIVE_C
    137 #if !defined(LANGUAGE_GOBJECT) || !LANGUAGE_GOBJECT

I think adding the conditions to the same line would be more readable here.
Overall the GTK+/GObject parts of the patch look really good to me, except for
these comments. Except for the the ref count issue, which might be serious, I
see no blockers to going forward with this. I'll leave the review? like it is,
though, for it would be useful to have someone who has written bindings before
have a quick look at the generator.

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