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

[bugzilla-daemon at webkit.org]

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





--- Comment #18 from Xan Lopez <xan.lopez at gmail.com>  2010-01-22 01:01:22 PST ---
(In reply to comment #15)
> (From update of attachment 46466 [details])
>  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 =).

Oops :)

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

That's exactly the case, yes. I will clarify the situation in the code.

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

Yeah, I think I even said the same thing in the old bug. Maybe we can do
another patch adding that file only and changing code in genera to use it?

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

I'm not sure I understand the comment. Do you think we should NOT use the
g-prefixed types here? Or we should? I don't like them in general when they are
useless (char vs gchar, etc), but I stuck with them because that's our code
style in WebKit as far as I remember.

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

You are right, the outer if is pointless.

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

I'm not sure there will be any difference, since I think we are only using the
address and that should still be the same, but I agree it makes more sense to
switch the order just in case.

> 
>  680         if ($functionName eq "webkit_dom_xml_http_request_open" &&
>  681         $param->name eq "url") {
> 
> This line break seems odd

Yup.

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

I was actually just copying the existing style, in other files when there were
(say) COM and OBJECTIVE_C #ifs, each one would be in one line IIRC. I have no
strong opinion about this, other than noting than putting them in the same line
makes for a really long line.

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

Yeah, we should have Sam or Mark take a look at it.

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