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

Fri Oct 17 17:19:29 PDT 2008

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

------- Comment #93 from mrowe at apple.com  2008-10-17 17:19 PDT -------
(In reply to comment #89)
> > 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.

"""
Use CamelCase. Capitalize the first letter of a class, struct, protocol, or
namespace name. Lower-case the first letter of a variable or function name.
Fully capitalize acronyms.

Use full words, except in the rare case where an abbreviation would be more
canonical and easier to understand.
"""

I'm not entirely sure which category "gobj" falls in.  I suspect it may be an
abbreviation of g_object, in which case GObject would be how it is mapped to
our naming conventions.

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

You have used both Gdom and GDOM within the same patch.  I'm also not convinced
that an external, third-party tool should be dictating to our naming
conventions either.  I'm sure that issue is fixable within that tool or in a
script that wraps the tool.

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

I'm not sure why you think it translates to that.  One way to incrementally
flesh out support is to have the scripts generate code for the entire feature,
but only compile the files that are completely implemented.  I think we do that
in some places in the Obj-C DOM in one or two places.

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

See my previous comment for why this is untrue.

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

Ok.  I was under the impression that the reason your workaround for that issue
was in a previous version of the patch was that it was required in order to
function.  Bug 20403 does not appear to have been solved to me: Darin signed
off on the concept of your fix, but some minor revisions were requested and you
have yet to post the updated patch that makes them.

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

There are problems.  I called out each individually in an earlier comment, so I
am not going to waste my time doing so again.  The fact that you convinced the
incorrect code to work does not make the code correct.  The incorrect use of
ref-counting absolutely must be addressed before the change can be landed. 

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

You have indeed outlined why you made the changes that you did.  However, as I
have pointed out several times now, the reasons that you made the changes in
many cases do not align with the WebKit project's vision for native bindings.

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