[Webkit-unassigned] [Bug 33590] [GTK] GObject DOM bindings
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jan 21 14:44:46 PST 2010
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.
More information about the webkit-unassigned
mailing list