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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Oct 17 16:37:18 PDT 2008


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





------- Comment #89 from lkcl at lkcl.net  2008-10-17 16:37 PDT -------
> > (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.

 #20403 is done.  solved. [but has been languishing for some weeks, even though
it's been noted as "the correct solution"]

the hack-attempt has already been removed.  the last-updated patch _should_
reflect that.  apologies if it's still included.  let's hope people aren't
working on out-of-date patches (!)


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

 i've read the guidelines - i don't understand what i'm missing.

 btw i started off with GDOM as the prefix and it blew up the pygtk
auto-generator.  i posted a bugreport about that and they said that there was
absolutely zero intention of fixing pygtk's codegen.py.

 so, you _can_ go and replace Gdom with GDOM, and it will screw things up for
people, further down the line.


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

 pretty much.

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

 i _may_ be wrong about this:

 i wouldn't recommend that _at the moment_, because the person who will be
expected to create the SVG bindings in the future may choose to implement a
_small_ subset of the SVG bindings, to begin with.

 if the above paragraph translates into "when someone comes to adding SVG
bindings, then they will either have to implement them all or not at all", then
it should be clear as to why that is undesirable.



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

 yaay.

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

 yaay!  (whew)

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

 whoops :)

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

 no.  definitely not.  i had to re-activate ... i think it was...
XMLHTTPRequest.open(String) because i couldn't find a way to create a Text
Document (there was no global constructor for one, and i hadn't put in the
equivalent of the "Global Attributes" in _order_ to make one).

 simplest solution: pass in a text string rather than a
WebCore::(Text)Document.

 so on a case-by-case basis, there are _going_ to be differences, and at no
time - as of yet - have i found that i needed to copy the ObjC bindings
behaviour (IDL-wise), but have, in each case, found that the JS bindings are a
better fit.

 of course, that may change: each case has its own set of issues to work
around.



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

 ConvertNullToNullString: agreed.  (well, i haven't used it, and
pyjamas-desktop hasn't blown up.  so i don't need it).

 toString: disagree.  _definitely_ need this one.

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

 no.

 1) the crashes caused by #20403 have been demonstrated and proven to have
absolutely nothing to do with the bindings.  at all.  nothing.

 2) #20403 has been solved.

> and the ref-counting problems within the patch
> itself.

 there have been _no_ ref-counting problems encountered. see earlier comment:
 i would be utterly shocked if there were "negative" refcounting bugs,
 because of the amount of time i've spent running pyjamas-desktop apps.

 without crashes of any kind (esp. since i patched in the solution #20403)

 i would equally be surprised if there were memory leaks, but you can clearly
 see, if you examine an auto-generated file, that each and every "add ref"
 is matched by a corresponding "dec ref".

 so, although it's not _proven_, with test code, this "refcounting problem"
 sounds much like an imaginary invention - a bogeyman in the closet :)



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

 whew.  small project for someone, at some point.  i'd recommend that
 the auto-generation be done at around the same time that DOMExceptions
 are re-added to the rest of the code.

 re-adding in the DOMExceptions (that i ripped out, so as to get a first
 version), would be an utter pain to do manually.

 much better to auto-generate a replacement for
DOMHTMLElementWrapperFactory.cpp

 btw - this _should_ be raised as a separate bugreport.

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

 i wouldn't make the differences without very good reasons and justification.

 XMLHTTPRequest open(String) is a good starting point: it's simply too much
additional work to provide the functionality of TextDocument, when just
enabling the use of open(String) instead of open(TextDocument) will do just as
well, if not better.

 each of the differences is pretty obvious and straightforward, and i believe
i've answered and explained them all.


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



More information about the webkit-unassigned mailing list