[Webkit-unassigned] [Bug 16401] [GTK] GObject/C DOM binding

Fri Oct 17 07:57:42 PDT 2008

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

------- Comment #79 from soto at informatik.uni-kl.de  2008-10-17 07:57 PDT -------
(In reply to comment #66)
> (In reply to comment #65)
...
> Please read back over the review comments I have left on a previous version of
> the patch.  There are many issues which have not yet been addressed and which
> need to be addressed before this can be landed.  In general though, this patch
> is trying to do far too much at once which makes it very hard to get right
> without going through many iterations, and it also makes it a lot of work to
> review.  An initial cut at the bindings that supports a subset of the
> functionality would be much quicker to clean up and get into a usable state,
> and would make it simple for others to finish off the work in parallel.

I suppose you're referring here to comment #46. In order to concentrate
on technical issues, as you wisely suggest in a later comment, let me go
through the points you made there. I'll mark the issues as follows:

(A) Already addressed issue.
(O) Open, serious issue, which probably requires further work.
(E) Easy to fix, probably something Kevin can do in his current clean-up
job (by the way, thanks for the offer!)

>From comment #46:

> Can you please name the generated files with a format that matches our coding
> style?  "GdomBarInfo.h" doesn't match our naming conventions, and should
> probably be GDOMBarInfo.h.  If these files are to be part of the WebKit GTK API
> then they should match the GTK naming convention, which I believe does not use
> upper-case letters at all.  Alp may be able to provide suggestions about what
> the right naming scheme is here.

(E) As far as I see, generated files are still being named using the
Gdom prefix instead of GDOM, which is inconsistent with the convention
used for the JavaScript files (they use "JS" not "Js" as prefix). This
must be corrected.

(E) This is also a problem for identifiers in general: they sometimes
start with GDOM and sometimes with Gdom. I propose to use GDOM
consistently. This would probably also imply renaming GdomDOMObject to
GDOMObject, because GDOMDOMObject would look sort of funny.

> The changes in FrameLoaderClientGtk.cpp look bogus.  Why are they in this
> patch?

(O) As Luke states in comment #48, these changes were needed for the
patch to run at all (he didn't receive any answer to that, by the way).
The way it looks, there seems to be a refcounting problem somewhere,
either introduced, or exposed by the patch. Unfortunately, I'm too much
of a WebKit novice to be able to debug this.

> Why does GDOMObject.h have a commented out 'namespace WebCore'?  Why does it
> have a function named get_gobj_type which doesn't match our naming conventions?

(A) No commented out namespace anymore. The function was renamed to
getGobjType, so this was addressed.

> The includes in webkitwebframe.cpp should be sorted per our style conventions. 
> They also shouldn't have the "gdom/" prefix.  The private header files should
> be have Private as a suffix to match the convention used elsewhere in the
> project (PrivateGdomDOMWindow.h -> GDOMWindowPrivate.h).  It's not clear to me
> why some of the files have "Int" in their name.  Is that short for "Internal"? 
> If so, the file should use that as a suffix in place of Private, such as
> GDOMWindowInternal.h.

(A) Includes are now sorted, and "Internal" and "Private" are being used
as stated. This part was addressed.

(E) I still see quite a few includes with a gdom/ prefix, though. This
must be fixed.

> There are commented-out printf's in webkitwebevent.cpp which shouldn't be
> there.

(E) There are still three commented print statements in the patch. 

> PrivateIntGdomEventTargetNode.h lacks a license header, and the Apple Inc.
> copyright line lacks the "All rights reserved." note that should be present. 
> The file also uses strange formatting for the include guard.  Please see
> existing code for an example of the style that should be used.

(E) This wasn't addressed at all. Also, there are more instances where
the Apple copyright lacks the "all rights reserved" sentence.

> The code in GStringConvert.h lacks a license header.  I'm not sure what the
> point of the GStringConvert class is.  It seems like it could easily be
> implemented using a simple overloaded function rather than the strange class +
> macro combination.  The "extern C" around a C++ class doesn't make a lot of
> sense either.  The header includes in this file also need sorted.

(A) GStringConvert is now a set of overloaded functions. This being the
case, (E) the name shouldn't be capitalized, though. There is now a
license header.

> Do the values for FEATURE_DEFINES_GDOM and FEATURE_DEFINES_JAVASCRIPT ever
> differ?  If not, is there any point in having both?

(O) I cannot address this so easily.

> GDOMHTMLElementWrapperFactory.cpp says that "THIS FILE IS DERIVED FROM AN
> AUTOMATICALLY GENERATED FILE."  what file is it derived from, and why is it
> being checked in if it is a generated file?

This file now contains a yelling comment stating that

+ * THIS FILE IS DERIVED BY HAND FROM AN AUTOMATICALLY GENERATED FILE.
+ * IT IS NOT AN AUTO-GENERATED FILE IT WAS CREATED MANUALLY *FROM*
+ * AN AUTO-GENERATED FILE.

(E) We probably should restate this comment to make it more friendly.
(O) Also, even if the file can probably be generated, would people agree
with keeping it in the current for for the time being before we can
write a generator?

> GDOMBinding.cpp has some FIXME's, TODO's and a lot of commented-out code that
> should be cleaned up before this can be landed.

(E) This seems to still be the case. Luke has stated elsewhere that he
wants all of these comments and pieces of dead code as reminders, but we
probably have to remove them in order for the patch to be accepted.

By the way, are TODO or FIXME remarks completely banned from WebKit? Are
they acceptable in any form? I can't find any project documentation
covering this point.

> Some of the IDL file changes do not look correct.  I would think that the
> GObject bindings would want to match the behaviour of the Obj-C DOM bindings
> rather than JS in the cases that they differ.  Many of these instances are due
> to behavioral peculiarities of JS, especially attributes such as
> ConvertNullToNullString and toString functions.

(O) Luke has answered to this issue in a number of further comments, but
I don't see any replies to them. A good deal of communication seems to
have happened outside of Bugzilla, so it is hard for me to determine the
status of this issue.

So, provided Kevin manages to get rid of the "easy" problems, four large
issues remain:

1. Refcounting problems.
2. Equivalence of FEATURE_DEFINES_GDOM and FEATURE_DEFINES_JAVASCRIPT.
3. Keeping GDOMHTMLElementWrapperFactory.cpp as a manually edited file.
4. Changes to the IDL files.

Any additional problems I'm missing? Could people please post their
thoughts on the open issues so that we can make some progress? I'll try
to look into issue 4 and see if I can understand what's going on, but
cannot promise anything for the moment.

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