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

Wed Jul 8 14:17:15 PDT 2009

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

--- Comment #215 from Luke Kenneth Casson Leighton <lkcl at lkcl.net>  2009-07-08 14:17:10 PDT ---
> After re-reviewing some of the comments, I think my comment that the patch
> "must be broken up in to smaller pieces" may be worded too strongly.  In
> Comment #76, breaking up the patch is suggested as a best practice for new
> contributors and will lead to quicker turnaround (reviews) on posted patches,
> but it's not a strict requirement.

 ah, whew, that's a relief.

> However, since you're unwilling to break it up

 it's more than that, it's that by doing so i cannot make any guarantees that
the individual portions are either correct or actually do anything.  it was
only by having pretty much absolutely everything that i could get
pyjamas-desktop to work, and thus run the high-level tests (pyjamas examples,
pyjamas unit tests etc.)

> (and since willing to be patient),

 got all the time in the world.  the main reason for that is because i managed
at europython to port pyjamas-desktop to the competitor to pywebkitgtk:
python-xpdom.  it uses XUL (mozilla).  so, pyjamas developers can develop
applications to run in web browsers (compiled to javascript) _or_ they can run
them as desktop apps...

.... no longer using webkit, but using mozilla technology instead.

> the next step would be to post a new monolithic patch for review
> addressing feedback in Comment #191 and Comment #194.

 ok.  the comments are related to a (highly modified) patch which i could not
get involved in, because of the removal of the event handling.

 so, *sigh* it's necessary to take two steps back in order to take one step
forward, for which i apologise (both to you, and to adam - sorry, adam, but
removing the event handling, i know why you did it, as it took me three days to
update to the latest svn, last week, but i have to work from what i know and
what i can prove works).

 so i'll endeavour to address the comments where they are relevant.

 one immediately that can be addressed: custom attributes.

> > +    if ($attribute->signature->extendedAttributes->{"Custom"} ||
> > +    	$attribute->signature->extendedAttributes->{"CustomGetter"} ||
> > +        $attribute->signature->extendedAttributes->{"CustomSetter"}) {
> > +        return 1;
> > +    }

> [Custom] and related attributes are only used for implementing JS specific
> quirks and should not be needed for GObject bindings. 

 the assertion that they are JS-relevant-only is not true, and you can
 test this by removing the "custom" test in the generator, and trying to
 add everything.

 i found this out myself by accidentally removing it, thanks to an
 error in the generator logic, whoops :)

 certain classes do not have corresponding IDL files.  one such class is
 "Object".  there is no IDL file for "Object", therefore no Gdom file
 gets auto-generated.  but, that doesn't stop the auto-generator from
 trying to use a class "GdomObject", and from adding a header
 #include <gdom/GdomObject.h>" - which of course doesn't exist.

 there were a hell of a lot of these when i started in august - i decided
 "stuff it, i'm cutting these all out, and will deal with them later".

 so, the "custom" test is utilised to skip many of these issues, until
 the time is found to re-add them, one by one, or just try them all and
 then deal with any classes and errors that crop up.

 ... but _not_ right now.

 so - the custom-property-skipping stays, as a way to reduce the number
 of issues to be dealt with, as way to reduce the amount of auto-generated
 code, as a way to make this a more manageable project.

 many of the comments will be along these lines.  i will make a best
 effort to address as many as possible, in one go, but i _strongly_
 urge you to consider the burden of having to review such whopping
 great patches, which are so thoroughly detailed and cover at least
 five specialist technical areas all at once, again and again.

 i do have an updated patch ready, but at europython i found that the
 use of gtk dialog boxes for "alert" messages resulted in a total lock-up
 of pywebkitgtk, on some of the larger applications.  if i find that this
 is a bug _without_ the #16401 patch (on some big JS-based apps), then i
 will proceed.

 also, one of the suggestions from alp, with which i thoroughly concur,
 is that this code should reeeally consider to be ... deactivated and
 require some special #ifdef or other switch (configure?), in order to be used.

 in that way, people won't try to go developing off into the distance
 with it until the various issues, which will be raised and dealt with
 after the patch is landed, are answered to everyone's satisfaction.

 i also strongly recommend that reviewers consider raising issues in specific
 small technical areas and as _separate_ bugreports (depending on this one)
 so that interested contributors who would like to see this move forward
 can tackle the specific area with a single small patch.

 for example, one contributor might want to deal with the AddRef / unref
 issue as a single patch.  [to do that right now has already proven to be
 too challenging and time-consuming]

 ... of course, this is all only advice.  you have no reason to listen to
 my advice, and are free to ignore it and to continue to be subjected to
 considerable stress in dealing with this work - that is entirely your right :)

 l.

 p.s. btw, david, and any of the reviewers: i have a thought for you to
consider, if you have time: perhaps you might like to consider downloading the
branch i'm maintaining at github.com/lkcl/webkit/16401.master, and also the
pywebkitgtk patch #13 at code.google.com/p/pywebkitgtk, and also the
http://pyjs.org project from subversion, and try out the pyjamas examples.  it
will give you a little bit more confidence that this is something that a) works
b) is very cool. c) you'll have some way to actually test and get to grips with
the 16401 patch.

 at the moment, i'm fully aware that 16401 is a... nebulous sort-of...
non-graspable entity which you just don't have a handle on, and as such, it's
difficult to appreciate the direction that this is going.

 constant reviewing of thousand-line patches can't be fun, given the
complexities involved, and i feel that if you knew exactly what can and is
being achieved with this patch you would feel much more comfortable with it.

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