Bug 33590 – [GTK] GObject DOM bindings

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Xan Lopez]

Opening a new bug for this, since the original one is insanely long.

[Xan Lopez]

Created attachment 46446 [details]
featuredefines.patch

A trivial patch, generalizing the feature defines list in preparation for the DOM bindings.

[Xan Lopez]

Created attachment 46448 [details]
inputfilecheck.patch

A trivial cleanup in the generate-bindings.pl script.

[Xan Lopez]

*** Bug 16401 has been marked as a duplicate of this bug. ***

[WebKit Review Bot]

Attachment 46446 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/185281

[Xan Lopez]

Created attachment 46449 [details]
featuredefines.patch

Missed a new feature when rebasing the patch.

[Xan Lopez]

Created attachment 46453 [details]
gobjectdombindings.patch

The main patch.

As was suggested in the original bug, I have reduced the scope of the patch, and I'm just generating bindings for a fairly small subset of the IDL files. Some comments:

- I basically provide Node.idl and all its dependencies (a ~dozen files) with two exceptions:
  * The ownerDocument method is excluded for the GObject bindings, since it brings along a lot of other files (through Document.idl) and thus defeats the purpose of a reduced initial patch. This would be the first thing to correct in a follow-up.
  * The EventListener feature is also excluded.
- I changed the prefix for the DOM methods to "WebKitDOM", as discussed in the old bug.
- I have removed lots of corner cases and hacks from the generator script, plus some heavy refactoring in some places. It's still not the prettiest thing, but I think it's more readable and it's about 300 lines shorter than the previous shortest version.
- Overall the patch is about 40K smaller than the previous smallest patch (which was about ~110K). I'm sure it can be made more clear/better in many places, but I'm not sure it can be made smaller in scope than it is, so this is pretty much it as far as complexity is concerned I'd say.

I'll attach another patch that adds a 'inner-node' property to WebKitHitTestResult as an example and proof that the whole thing works :)

[WebKit Review Bot]

Attachment 46453 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/gobject/webkitdomstringconvert.h:21:  #ifndef header guard has wrong style, please use: webkitdomstringconvert_h  [build/header_guard] [5]
WebCore/bindings/gobject/webkitdomstringconvert.h:32:  webkit_dom_gstring_convert is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/gobject/webkitdomstringconvert.h:33:  webkit_dom_gstring_convert is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/gobject/webkitdomstringconvert.h:34:  webkit_dom_gstring_convert is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/gobject/webkitdomstringconvert.h:35:  webkit_dom_gstring_convert is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/gobject/WebKitDOMBinding.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/gobject/WebKitDOMBinding.cpp:82:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/bindings/gobject/WebKitDOMObject.cpp:16:  webkit_dom_object_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/gobject/WebKitDOMObject.cpp:20:  webkit_dom_object_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/gobject/WebKitDOMBinding.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/gobject/WebKitDOMBinding.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/bindings/gobject/WebKitDOMObject.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/gobject/WebKitDOMObject.h:44:  parent_instance is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/gobject/WebKitDOMObject.h:50:  parent_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/gobject/WebKitDOMObject.h:54:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 15

[Xan Lopez]

Created attachment 46465 [details]
gobjectdombindings.patch

This should fix all style issues that are fixable/makes sense to fix.

[WebKit Review Bot]

Attachment 46465 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/gobject/WebKitDOMObject.cpp:12:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/gobject/WebKitDOMObject.cpp:16:  webkit_dom_object_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/gobject/WebKitDOMObject.cpp:20:  webkit_dom_object_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/gobject/WebKitDOMBinding.h:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/bindings/gobject/WebKitDOMObject.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/gobject/WebKitDOMObject.h:54:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6

[Xan Lopez]

Created attachment 46466 [details]
gobjectdombindings.patch

OK, checked it locally this time, only warnings should be the ones for the init/class_init functions in DOMObject, which have to be that way.

[WebKit Review Bot]

Attachment 46466 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/gobject/WebKitDOMObject.cpp:12:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/gobject/WebKitDOMObject.cpp:16:  webkit_dom_object_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/gobject/WebKitDOMObject.cpp:20:  webkit_dom_object_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3

[Gustavo Noronha (kov)]

(In reply to comment #10)
> Created an attachment (id=46466) [details]
> gobjectdombindings.patch
> 
> OK, checked it locally this time, only warnings should be the ones for the
> init/class_init functions in DOMObject, which have to be that way.

I reported this as a bug to the style checker here: https://bugs.webkit.org/show_bug.cgi?id=33606

[Xan Lopez]

*** Bug 27410 has been marked as a duplicate of this bug. ***

[Gustavo Noronha (kov)]

Comment on attachment 46449 [details]
featuredefines.patch

Looks good to me!

[Gustavo Noronha (kov)]

Comment on attachment 46466 [details]
gobjectdombindings.patch

 29 2010-01-13  Xan Lopez  <[e-mail]>
 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.

[Xan Lopez]

Comment on attachment 46448 [details]
inputfilecheck.patch

Landed in r53686.

[Xan Lopez]

Comment on attachment 46449 [details]
featuredefines.patch

Landed in r53685.

[Xan Lopez]

(In reply to comment #15)
> (From update of attachment 46466 [details])
>  29 2010-01-13  Xan Lopez  <[e-mail]>
>  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.

[Gustavo Noronha (kov)]

(In reply to comment #18)
> > 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?

Sounds like a great idea to me!

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

No, I agree on using them. I'm just pointing out that you are using non-g-prefixed char in the first line:

+    my %types = ("DOMString", "char* ",
+                 "float", "gfloat",
+                 "double", "gdouble",
+                 "boolean", "gboolean",
+                 "char", "gchar",

DOMString is mapped to char*, while char is mapped to gchar =). Make the DOMString a gchar*, perhaps.

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

I don't have a strong opinion either. If that is what current code already does, I'm fine.

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

Now we just need to grab one of them.

[Xan Lopez]

Adding Sam and Mark in case they want to have a look at the patch.

[Xan Lopez]

(In reply to comment #19)
> (In reply to comment #18)
> > > 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?
> 
> Sounds like a great idea to me!
> 

A quick update on this: on further reflection this is not such a great idea because, a) the functions can only be made to work if they strdup the char data (otherwise the char data dies when you temporary CString we create goes out of scope) b) the cases were we want to explicitly strdup the data are really few, in the vast majority of cases the data is duped in the function you pass the const char* data too.

So after talking with Gustavo we have decided to leave things like they are for now.

[Brian Tarricone]

Glad I found this bug -- this turns out to be exactly what I need for some DOM manipulations in my webkit-gtk based app.

I applied the patch and I'm looking through the code it generates... I assume this will also require some API additions to webkit-gtk proper to be able to e.g. retrieve the gobject-wrapped DOM tree from a WebKitWeb(View|Frame)'s window/document object?  If so, is there a separate bug tracking that work?

[Xan Lopez]

(In reply to comment #22)
> Glad I found this bug -- this turns out to be exactly what I need for some DOM
> manipulations in my webkit-gtk based app.
> 
> I applied the patch and I'm looking through the code it generates... I assume
> this will also require some API additions to webkit-gtk proper to be able to
> e.g. retrieve the gobject-wrapped DOM tree from a WebKitWeb(View|Frame)'s
> window/document object?  If so, is there a separate bug tracking that work?

You are right, that's needed and would come in the next patch to the one flagged for review, when we wrap Document.idl and everything that it brings. We tried to keep the first patch as small as possible to facilitate review, as explained in the previous comments.

[Brian Tarricone]

I came across this patch here:
http://github.com/lkcl/webkit/blob/16401.master/16401.master.patch

... which looks to be much more complete.  Any reason why that work isn't being used as is, or at least as a base for what you're doing, Xan?  Seems to be an unfortunate duplication of effort :(

[Xan Lopez]

(In reply to comment #24)
> I came across this patch here:
> http://github.com/lkcl/webkit/blob/16401.master/16401.master.patch
> 
> ... which looks to be much more complete.  Any reason why that work isn't being
> used as is, or at least as a base for what you're doing, Xan?  Seems to be an
> unfortunate duplication of effort :(

It is being used as a base, and the new patches are less complete *on purpose*, since previous reviewers asked for this explicitly to ease their work.

[Mark Rowe (bdash)]

Comment on attachment 46466 [details]
gobjectdombindings.patch

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

[Xan Lopez]

(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. :)

[Christian Dywan]

(In reply to comment #27)
> (In reply to comment #26)
> > > +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.

There is a subtle difference. g_strdup may be using a different memory allocator, such as GSlice, as opposed to strdup. So using gchar* for the return type makes it clear that g_free rather than free should be used to release the memory.

[Xan Lopez]

(In reply to comment #28)
> > 
> > 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.
> 
> There is a subtle difference. g_strdup may be using a different memory
> allocator, such as GSlice, as opposed to strdup. So using gchar* for the return
> type makes it clear that g_free rather than free should be used to release the
> memory.

When inside a glib/gobject project all memory should be freed using the glib functions, g_free, g_slice_free, etc. This is IMHO an orthogonal issue wrt whether you use 'gchar' or 'char', which are identical and basically considered bad ideas even by the people who first thought it was desirable to introduce extra names like that when portability is not an issue. We could, and probably should, stop using 'gchar' or 'gint' inside WebKitGTK+, and you'd still need to use g_free to free our strings, so the idea that this is somehow related to what name you use for your types is not correct IMHO.

[Christian Dywan]

(In reply to comment #29)
> (In reply to comment #28)
> > > 
> > > 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.
> > 
> > There is a subtle difference. g_strdup may be using a different memory
> > allocator, such as GSlice, as opposed to strdup. So using gchar* for the return
> > type makes it clear that g_free rather than free should be used to release the
> > memory.
> 
> When inside a glib/gobject project all memory should be freed using the glib
> functions, g_free, g_slice_free, etc. This is IMHO an orthogonal issue wrt
> whether you use 'gchar' or 'char', which are identical and basically considered
> bad ideas even by the people who first thought it was desirable to introduce
> extra names like that when portability is not an issue. We could, and probably
> should, stop using 'gchar' or 'gint' inside WebKitGTK+, and you'd still need to
> use g_free to free our strings, so the idea that this is somehow related to
> what name you use for your types is not correct IMHO.

This is not correct. If memory is allocated using Glib, it needs to be released using Glib. Otherwise you can end up trying to free something with an allocator unaware of that memory. Think of xmlGetProp/ xmlFree or sqlite3_malloc/ sqlite3_free which have the same requirement.

Arguably gchar is "just" an alias. But if used consistently it can serve as a reminder to use the correct memory functions.

[Xan Lopez]

(In reply to comment #30)
> > When inside a glib/gobject project all memory should be freed using the glib
> > functions, g_free, g_slice_free, etc. This is IMHO an orthogonal issue wrt
> > whether you use 'gchar' or 'char', which are identical and basically considered
> > bad ideas even by the people who first thought it was desirable to introduce
> > extra names like that when portability is not an issue. We could, and probably
> > should, stop using 'gchar' or 'gint' inside WebKitGTK+, and you'd still need to
> > use g_free to free our strings, so the idea that this is somehow related to
> > what name you use for your types is not correct IMHO.
> 
> This is not correct. If memory is allocated using Glib, it needs to be released
> using Glib. Otherwise you can end up trying to free something with an allocator
> unaware of that memory. Think of xmlGetProp/ xmlFree or sqlite3_malloc/
> sqlite3_free which have the same requirement.

This is exactly what I meant. When using glib in a project most/all of your functions will use the allocation functions from glib, so in the common case you'll have to use the corresponding release functions, regardless of your style preference between 'gchar' and 'char'.

> 
> Arguably gchar is "just" an alias. But if used consistently it can serve as a
> reminder to use the correct memory functions.

I don't agree that we need to do this to be constantly reminded that you have to use 'g_free' to free strings instead of simply 'free'. That should be the default assumption (certainly we don't document it all over the place), and special care should be given, documentation-wise, to the cases where for some reason this cannot be done (like, for example, if we need to return strings allocated by different libraries).

In any case I'm reasonable sure that this not the origin of the 'gchar' typedef, whether or not some people choose to use for that purpose is another issue.

Also, this is offtopic in any case. For now I'll use gchar everywhere since that is our current style, we can discuss whether or not we want to change in the future.

[Xan Lopez]

Created attachment 47928 [details]
dombindings.diff

Hopefully this addresses all the comments about the patch so far.

[WebKit Review Bot]

Attachment 47928 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/gobject/WebKitDOMObject.cpp:12:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 46448 [details]
inputfilecheck.patch

Clearing r+ on this obsolete patch.

[Eric Seidel (no email)]

Comment on attachment 46449 [details]
featuredefines.patch

Clearing r+ on this obsolete patch.

[Grant Gayed]

Hi, just to confirm, is this bug still targetted for WebKitGTK 1.2?  (I say "still" because I think I remember seeing a blog post indicating that this was the plan).  I ask because I either need to decide whether to wait for the new API if its coming by then, or starting working on a workaround approach otherwise.  Thanks!

[Adam Barth]

Comment on attachment 47928 [details]
dombindings.diff

I struggled with what to do with this patch.  On the one hand, it's much to large to give anything but a syntactic review, but on the other hand, the patch has been up for review for a while and a number of people have commented.  The GDOM bindings seem to be in demand by a number of people and we've got to start somewhere.

I'm surprised that there's so much code in the make files.  I would have expected those snippets to be broken out into a separate file somehow.

In expanding the GDOM bindings, please consider using the generic bindings, when possible, to improve maintainability.  The generic bindings don't have much in them yet (and certainly nothing that's helpful for this patch), but we're hoping to grow them over time.

On balance, I think we should take this patch and iterate on this code rather than leaving the patch in review forever.

[Xan Lopez]

(In reply to comment #36)
> Hi, just to confirm, is this bug still targetted for WebKitGTK 1.2?  (I say
> "still" because I think I remember seeing a blog post indicating that this was
> the plan).  I ask because I either need to decide whether to wait for the new
> API if its coming by then, or starting working on a workaround approach
> otherwise.  Thanks!

1.2 definitely won't have this in a usable form, the next stable release will (it seems).

[Xan Lopez]

(In reply to comment #37)
> (From update of attachment 47928 [details])
> I'm surprised that there's so much code in the make files.  I would have
> expected those snippets to be broken out into a separate file somehow.

This might be a good idea, yes.

> 
> In expanding the GDOM bindings, please consider using the generic bindings,
> when possible, to improve maintainability.  The generic bindings don't have
> much in them yet (and certainly nothing that's helpful for this patch), but
> we're hoping to grow them over time.

OK, have to say I haven't really looked into it. Will check how other ports use it.

> 
> On balance, I think we should take this patch and iterate on this code rather
> than leaving the patch in review forever.

Yep, that's the plan, thank you.

Now I'll check with kov about whether we want to branch for 1.2 before landing this, which I think is the most sensible thing to do.

[Eric Seidel (no email)]

Attachment 47928 [details] was posted by a committer and has review+, assigning to Xan Lopez for commit.

[Xan Lopez]

Comment on attachment 47928 [details]
dombindings.diff

Landed this as r57985, more patches to follow.

[Xan Lopez]

Created attachment 53964 [details]
dombindingsdocument.diff

Second patch, binding Document.idl and finally exposing the API to access all the bindings.

This is still very big (55K), but unfortunately this is a bit of a all-or-nothing thing. I think I might be able to split the HTML*Element stuff into a different (useless like the first one) patch if this seems too overwhelming.

[WebKit Review Bot]

Attachment 53964 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/gobject/WebKitHTMLElementWrapperFactory.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/gobject/WebKitHTMLElementWrapperFactory.h:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/bindings/gobject/WebKitHTMLElementWrapperFactory.cpp:36:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
WebCore/bindings/gobject/WebKitHTMLElementWrapperFactory.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/gobject/WebKitHTMLElementWrapperFactory.cpp:46:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/gobject/WebKitHTMLElementWrapperFactory.cpp:51:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/gobject/WebKitHTMLElementWrapperFactory.cpp:61:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/gobject/WebKitHTMLElementWrapperFactory.cpp:69:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/gobject/WebKitHTMLElementWrapperFactory.cpp:77:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/gobject/WebKitHTMLElementWrapperFactory.cpp:97:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/gobject/WebKitHTMLElementWrapperFactory.cpp:104:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/gobject/WebKitHTMLElementWrapperFactory.cpp:109:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/gobject/WebKitHTMLElementWrapperFactory.cpp:118:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/gobject/WebKitHTMLElementWrapperFactory.cpp:126:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/gobject/WebKitHTMLElementWrapperFactory.cpp:136:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/gtk/webkit/webkitwebview.cpp:43:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/gtk/webkit/webkitwebview.h:386:  Extra space before ( in function call  [whitespace/parens] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/gtk/tests/testwebview.c"
Total errors found: 17 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 53964 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1814032

[Xan Lopez]

Created attachment 54045 [details]
dombindingsdocument.diff

Fix the build with geolocation and fix all style warnings except the one ni webkitwebview.h, since fixing that one would make the file internally inconsistent.

[Gustavo Noronha (kov)]

Comment on attachment 54045 [details]
dombindingsdocument.diff

You can't r+ your own patches, that's cheating! lol =)

[WebKit Review Bot]

Attachment 54045 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/gtk/webkit/webkitwebview.h:386:  Extra space before ( in function call  [whitespace/parens] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/gtk/tests/testwebview.c"
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

Comment on attachment 54045 [details]
dombindingsdocument.diff

72      if (node->nodeType())
 74     switch (node->nodeType()) {
 75     case Node::ELEMENT_NODE:
 76         if (node->isHTMLElement())
 77             wrappedNode = createHTMLElementWrapper(static_cast<HTMLElement*>(node));
 78         else
 79             wrappedNode = wrapNode(node);
 80         break;
 81     default:
7382         wrappedNode = wrapNode(node);
 83         break;
 84     }

Thisss seems to change the meaning of the default condition. Is it possible that node->nodeType() will return false boolean-wise? What if t does? You'll try to wrap something that makes no sense? Perhaps we should add an ASSERT there, since I believe it should not be possible that nodeType will return 0 there. http://trac.webkit.org/browser/trunk/WebCore/dom/Node.h

 108     $CLASS_NAME =~ s/DOMX_PATH/DOM_X_PATH/;

hrm I wonder why this is called DOMX_PATH. I would have expected XPATH to be a single word. Can we go DOM_XPATH here?

 163 
 164 

Needless double blank lines ;D

 171     # Skip all custom methods; is this ok?
 172     if ($function->signature->extendedAttributes->{"Custom"}) {
 173         return 1;
 174     }

Add a FIXME note.

 184     if ($function->signature->type eq "Event") {
 185         return 1;
 186     }

This condition appears twice.

 4445 webkit_web_view_get_document(WebKitWebView* webView)

I prefer get_dom_document. Let me post what I have already before I go hunt more information about this Reflect/ReflectURL thing.

[Gustavo Noronha (kov)]

Comment on attachment 54045 [details]
dombindingsdocument.diff

OK, I read the thread, and I remembered reading it last year. I believe this patch looks good, so I'm OK with landing it with the concerns I raised addressed!

[Xan Lopez]

Comment on attachment 54045 [details]
dombindingsdocument.diff

Addressed all of Gustavo's comments and pushed as r58108.

[Xan Lopez]

Created attachment 54166 [details]
headerfix.diff

This should fix the warnings in the tests, the main DOM header was not included in webkit.h

[Xan Lopez]

Created attachment 54206 [details]
propertywarnings.diff

This should fix all the warnings we have in the generated code for properties. I'm not sure if what I'm doing passing around references to arrays is very orthodox, so I'd appreciate a double check by someone who actually knows perl.

[Xan Lopez]

Created attachment 54309 [details]
testdomdocument.diff

This moves the Document tests to their own file.

[Xan Lopez]

Created attachment 54310 [details]
testelementsbytagname.diff

Adds a test for getElementsByTagName

[Xan Lopez]

Created attachment 54311 [details]
testgetelementsbyclassname.diff

Adds a test for getElementsByClassName.

[Xan Lopez]

Created attachment 54312 [details]
testgetelementbyid.diff

Add a test for getElementbyId.

[Gustavo Noronha (kov)]

Comment on attachment 54206 [details]
propertywarnings.diff

 351 EOF
 352 push(@txtSetProps, $txtSetProps);
 353     if (scalar @writeableProperties > 0) {
 354         $txtSetProps = << "EOF";
306355     ${className} *self = WEBKIT_DOM_${clsCaps}(object);
307356     $privFunction
 357 EOF
 358 push(@txtSetProps, $txtSetProps);
 359     }
308360 

Looks like you have some indentation problems here, except for that looks good.

[Gustavo Noronha (kov)]

Comment on attachment 54309 [details]
testdomdocument.diff

It would be awesome if we added JSC to the mix in this test, to make sure the title is reflected in what javascript sees, but this can come later.

[Gustavo Noronha (kov)]

Comment on attachment 54310 [details]
testelementsbytagname.diff

LGTM

[Gustavo Noronha (kov)]

Comment on attachment 54311 [details]
testgetelementsbyclassname.diff

Looks good, too.

[Gustavo Noronha (kov)]

Comment on attachment 54312 [details]
testgetelementbyid.diff

Nice.

[Xan Lopez]

Comment on attachment 54311 [details]
testgetelementsbyclassname.diff

Landed in r58513

[Xan Lopez]

Comment on attachment 54310 [details]
testelementsbytagname.diff

Landed in r58512

[Xan Lopez]

Comment on attachment 54309 [details]
testdomdocument.diff

Landed in r58511

[Xan Lopez]

Comment on attachment 54206 [details]
propertywarnings.diff

Landed in r58510

[Xan Lopez]

Comment on attachment 54166 [details]
headerfix.diff

Landed in r58509

[Xan Lopez]

Created attachment 54697 [details]
skipfunctioncleanup.diff

A small cleanup to the skip function logic and whitelist a couple of methods in HTMLCollection.idl

[Xan Lopez]

Created attachment 54698 [details]
getlinks.diff

Unit test for webkit_dom_document_get_links

[WebKit Review Bot]

Attachment 54698 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1914074

[Xan Lopez]

(In reply to comment #69)
> Attachment 54698 [details] did not build on gtk:
> Build output: http://webkit-commit-queue.appspot.com/results/1914074

Well, of course, since it needs the previous patch :/

[Oliver Hunt]

Comment on attachment 54697 [details]
skipfunctioncleanup.diff

r=me

[Oliver Hunt]

Comment on attachment 54698 [details]
getlinks.diff

r=me

[Xan Lopez]

Comment on attachment 54697 [details]
skipfunctioncleanup.diff

Committed as r58633

[Xan Lopez]

Comment on attachment 54698 [details]
getlinks.diff

Committed as r58634.

[Xan Lopez]

Created attachment 54850 [details]
isgdomclass.diff

Small cleanup to use more code from CodeGenerator.pm

[Eric Seidel (no email)]

Comment on attachment 54045 [details]
dombindingsdocument.diff

Cleared Gustavo Noronha Silva's review+ from obsolete attachment 54045 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 54166 [details]
headerfix.diff

Cleared Gustavo Noronha Silva's review+ from obsolete attachment 54166 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 54206 [details]
propertywarnings.diff

Cleared Gustavo Noronha Silva's review+ from obsolete attachment 54206 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 54309 [details]
testdomdocument.diff

Cleared Gustavo Noronha Silva's review+ from obsolete attachment 54309 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 54310 [details]
testelementsbytagname.diff

Cleared Gustavo Noronha Silva's review+ from obsolete attachment 54310 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 54311 [details]
testgetelementsbyclassname.diff

Cleared Gustavo Noronha Silva's review+ from obsolete attachment 54311 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 54312 [details]
testgetelementbyid.diff

Cleared Gustavo Noronha Silva's review+ from obsolete attachment 54312 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 54697 [details]
skipfunctioncleanup.diff

Cleared Oliver Hunt's review+ from obsolete attachment 54697 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 54698 [details]
getlinks.diff

Cleared Oliver Hunt's review+ from obsolete attachment 54698 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Xan Lopez]

Created attachment 54930 [details]
geolocationdom.diff

Pay attention to the available WebCore features when generating the bindings (well, to geolocation so far).

[Adam Barth]

Is there some reason we're working on this code with an epic uber bug?  This bug has 86 comments and is pretty intimidating to catch up on.  Using one patch per bug would be easier for the rest of us to follow.

[Xan Lopez]

(In reply to comment #86)
> Is there some reason we're working on this code with an epic uber bug?  This
> bug has 86 comments and is pretty intimidating to catch up on.  Using one patch
> per bug would be easier for the rest of us to follow.

Should I use this as a tracker bug then? Or will you just find the new bugs? I was doing it mostly so that the non-GTK+ people can easily see the patches while we are in the early stages, but I can do anything else if that works best for you.

[Adam Barth]

> Should I use this as a tracker bug then? Or will you just find the new bugs?

Yeah, a tracking bug seems like a good idea.  Sometimes I'm tempted to read these patches, but I never know how many of the preceding comments are related to the patch.  By breaking things up into individual bugs, it's easier for me to see what context is relevant for a given patch.

[Eric Seidel (no email)]

I suggest closing this bug as a dupe of some new clean master bug, and then relating each patch bug to said master.  Master bugs occasionally get comments.  Sorting through zillions of comments is impossible on any bug, even a master/tracking bug.

[Holger Freyther]

Comment on attachment 54850 [details]
isgdomclass.diff

Looks fine. Is the GObject generation covered by unit tests introduced by Adam?

[Holger Freyther]

Comment on attachment 54930 [details]
geolocationdom.diff


>  
> +# Some methods' body (only the body, since the public API can't be
> +# conditional) should be guarded by #ifdefs depending on whether
> +# certain features in WebKit are enabled.
> +my %conditionalMethods = ("webkit_dom_geolocation_clear_watch" => "GEOLOCATION");

I think the current code generation is dangerous. We are not retuning anything from these methods now. We will need to have a default return type or such.

[Xan Lopez]

(In reply to comment #90)
> (From update of attachment 54850 [details])
> Looks fine. Is the GObject generation covered by unit tests introduced by Adam?

Yep, I run it when I do changes to the script.

[Xan Lopez]

(In reply to comment #91)
> (From update of attachment 54930 [details])
> 
> >  
> > +# Some methods' body (only the body, since the public API can't be
> > +# conditional) should be guarded by #ifdefs depending on whether
> > +# certain features in WebKit are enabled.
> > +my %conditionalMethods = ("webkit_dom_geolocation_clear_watch" => "GEOLOCATION");
> 
> I think the current code generation is dangerous. We are not retuning anything
> from these methods now. We will need to have a default return type or such.

You mean that if they return values the info should also include the default return type? Makes sense, although in this particular case it does not apply since it does not return anything. We can do that when we cover the first method with a return type.

[Xan Lopez]

Comment on attachment 54850 [details]
isgdomclass.diff

Landed as r58749

[Gustavo Noronha (kov)]

Comment on attachment 54930 [details]
geolocationdom.diff

Looks sane to me.

[Xan Lopez]

Comment on attachment 54930 [details]
geolocationdom.diff

Landed as r60223

[Xan Lopez]

All patches landed, closing the bug.

[webkit censorship bypass 0005]

(In reply to comment #89)
> I suggest closing this bug as a dupe of some new clean master bug, and then relating each patch bug to said master.  Master bugs occasionally get comments.  Sorting through zillions of comments is impossible on any bug, even a master/tracking bug.

eric, that action was recommended by another webkit developer, for the 16401 bug development, which topped a whopping 300+ comments.  following that recommendation in good faith, the 16401 bugreport was then split into 30 separate bugreports. a lot of effort was made in creating dependencies so that the bugreports could be reviewed in the correct order.  however, as a result of following the recommended and approved course of action, _another_ webkit developer - mark rowe - took unilateral action to close all 30 bugreports, and terminated the account of the submitter.

fortunately, now that this bugreport has _at last_ been accepted, further work can be carried out on an incremental basis.

the inter-dependencies between the DOM objects always meant that this work was pretty much going to have to be accepted "on faith", and fortunately it doesn't have any impact or dependencies "per se" on the core webkit functionality (it never did).

there is a lot that still needs to be done, but i think you will find that there are a large number of people who will be extremely grateful to xan, gustav and all the reviewers for finally making this happen.