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

Wed Feb 11 07:10:29 PST 2009

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

------- Comment #144 from lkcl at lkcl.net  2009-02-11 07:10 PDT -------
(In reply to comment #93)
> (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.

 ohhh, i see.  ok, now i understand.  it wasn't the camel case that was
 the issue, it was the abbreviation of the naming.

 that's the bit i didn't quite follow.  apologies.

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

 fortunately, the consensus is to use "WebKit" as the prefix, thus
 side-stepping this issue, thank goodness.

 as i mentioned before, the python-pygobject auto-generator, h2defs.py,
 is sufficiently complex that the developers have made it clear that
 any interfaces that do not conform to the StandardConvention will NOT
 be supported.

 leaving a rather awkward pre-processing step, just like the vala bindings.

 fortunately, using WebKit as the prefix (or WebKitDom) keeps everybody
 happy, with WebKit being the "preferred" choice.

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

 oh right.  that works.

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

 i did.  but to double-check that i have read it, a cross-reference
 would be appreciated (we're up to comment #144 so far, so it's
 difficult to make absolutely sure).

 when you get to it, could you kindly review the references to
 de-facto standards implementations of toString in all other
 competitor products to webkit?

 for convenience, the researched evidence presented is in
 comments numbered #121 to #127.

 comments #121 to #124 are specific research on "toString", indicating
 that even wine's implementation of MSHTML provides "toString", and it
 does so by using Gecko's "toString" function.

 if the wine developers had attempted to use webkit's glib / gobject
 bindings to do the job, they would FAIL, and would give up, and
 abandon webkit, because  "toString" is "not part of the standard".

 it's not in the W3C standard, it's part of the de-facto standard, as
 shown by the evidence presented.

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

 i'm happily maintaining a branch of webkit, for a client, and they're
 happy with it.  my apologies for dropping the ball on #20403.

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

 i would ask you kindly to provide a link to which of the 144 comments
 in this thread that is, but see below.

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

 ok - fortunately, adam's work involves removal of all c++ objects - entirely.
 thus making it _essential_ that ref() and deref() are utilised.

 so, fortunately, this issue can be dropped.  three people have now
 worked on this issue, and three people have all used ref() and
 deref().  two of them exclusively used ref() and deref() (martin and adam).

 so, i am quite happy to let them continue discussions with you on
 resolving and addressing this issue.

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

 that vision is a broken vision, one in which webkit fails to be utilised
 for many projects, and gecko is utilised instead as the only other viable
 option, thanks to its compliance with de-facto up-to-date standards.

 i trust that you will review the evidence presented in comments #121-#127
 and that the webkit vision will be updated accordingly, so that the
 webkit vision for native bindings includes conformance to de-facto standards
 as part of the vision.

 strict adherence to W3C standards as part of the webkit project vision
 will result in the need to rip out non-W3C-compliant features such as
 "offsetWidth" and "offsetHeight".

 i trust that the webkit team are reasonable people whom, when presented
 with good evidence, will be happy to work with people who would like to
 see webkit's vision and webkit's usefulness expanded.

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