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

Fri Oct 17 13:50:38 PDT 2008

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

------- Comment #83 from mrowe at apple.com  2008-10-17 13:50 PDT -------
(In reply to comment #79)
> 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.

Thanks for taking the time to summarise this.

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

It looks as though this is an attempt to work around an issue tracked by bug
20403.  The correct solution here is probably to finish off the fix for bug
20403 and remove this code from FrameLoaderClientGtk.cpp.

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

Minor issue: getGobjType doesn't follow the naming conventions outlined in the
coding style guidelines, due to the same capitalization inconsistency that
"Gdom" has in some places in this patch.

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

The reason that I asked this is that the Mac port uses the same variable in its
Makefile (WebCore/DerivedSources.make) for the defines of both JS and Obj-C
bindings.  It should be easy to unify them for GDOM too.  Is the only
difference that at the moment the GDOM binding lacks SVG support?  At the very
least this shouldn't require duplication of assignments for each possible
feature define.  The common values could be assigned to a FEATURE_DEFINES
variable, with FEATURE_DEFINES_JAVASCRIPT taking its value and adding
LANGUAGE_JAVASCRIPT, ENABLE_SVG, etc, and FEATURE_DEFINES_GDOM only adding
LANGUAGE_GDOM.  Another alternative would be to have FEATURE_DEFINES include
all features that are enabled, and FEATURE_DEFINES_GDOM be the result of
stripping out any values that mention SVG from the FEATURE_DEFINES variable.

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

The comment could be as simple as:
  // FIXME: This file should be auto-generated in the future
<https://bugs.webkit.org/show_bug.cgi?id=some-bug-number>.

It should be moved below the license header though.

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

It would definitely be preferable to generate this automatically, but it's not
clear what the right way to do this would be at this point.  The Obj-C binding
currently has this hard-coded, so that seems fine as a first step for this
patch.

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

A grep through WebCore will show that we have numerous FIXMEs in the code.  The
typical form of the FIXME in our code is like what I suggested above: a short
description of the issue that needs to be addressed, and a link to a bug about
addressing that issue in the future.  FIXMEs are not an excuse to leave
commented-out code in the tree.  New code should follow the consistent style I
have mentioned, rather than a mismatch of styles such as the following:

 153 #if 0 /* XXX TODO - see #20586 */

 463             return NULL; /* TODO - see #20586 */

 513         return NULL; /* XXX hmmm... */

 29 /* TODO: make a GdomEventListener class, to be returned here */

 3412 #FEATURE_DEFINES_GDOM += ENABLE_SVG_USE=1 # big TODO!

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

I do recall discussing this with him on IRC.  I'll look back over his comments
shortly and add my thoughts to this bug. 

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

There are two issues here: the ref-counting problems tracked in bug 20403,
which sound as though they need to be addressed before these bindings will
function without crashing, and the ref-counting problems within the patch
itself.

> 2. Equivalence of FEATURE_DEFINES_GDOM and FEATURE_DEFINES_JAVASCRIPT.

I don't think this is a large issue that would be especially difficult to
address.

> 3. Keeping GDOMHTMLElementWrapperFactory.cpp as a manually edited file.

Nor this.

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