Bug 27410 – [Gtk] adding GObject Language bindings auto-generator

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebKitGTK

[Luke Kenneth Casson Leighton]

Created attachment 33020 [details]
adds codegenerator for gobject language bindings

following david's advice, this patch adds just the gobject code generator.  follow-up bug reports will make use of it.  the code-generator is the largest piece of the puzzle: once this is committed, the remaining pieces should be a breeze.

[Jan Alonzo]

Comment on attachment 33020 [details]
adds codegenerator for gobject language bindings

The patch is still is huge. If possible please trim it down to something that can build the bindings for Node.idl only. The point here is to review the generation process (and code) and not to make an outright usable GLib bindings.

[Luke Kenneth Casson Leighton]

(In reply to comment #1)
> (From update of attachment 33020 [details])
> The patch is still is huge. 

 yes.  i know.

> If possible please trim it down to something that
> can build the bindings for Node.idl only. The point here is to review the
> generation process (and code) and not to make an outright usable GLib bindings.

 sadly, as already discussed, and to reiterate here, which i am very disappointed to be having to do:

 * milestone 1 has been reached with patch #16401.  i am already in "maintenance" mode on this work - not "development".  the development milestone (1) is _completed_.

 * the generation process (and code) was developed using high-level testing on top of extreme rapid iterative process.  therefore, even _i_ don't know exactly why many of these things are in there: all i can tell you is, "it works", and, if you are willing to take the original patch, and apply it, and then also build pywebkitgtk as well, you too can prove to yourself that "it works".

 * line-by-line, blow-by-blow, bit-by-bit, piece-by-piece review is going to bore the pants off of me _and_ any reviewers wishing to take it on.  and take absolutely forever.  i've said - already - i'm _not_ willing to commit that level of time.

 * "trimming down" requires development, going "backwards" not forwards.  development "backwards" is not something that not one but _two_ separate developers have already said "no" to.


so.

if you would like less lines, i can simply cut the file in half, if you like.  if that's too few lines, i'm happy to cut it in half again, until you have an amount of lines of code that is acceptable.

but i have already made it clear: there will be _no_ time spent "developing".  and "going backwards" requires "development.

i will re-enable the review request and await your decision.

[Luke Kenneth Casson Leighton]

Comment on attachment 33020 [details]
adds codegenerator for gobject language bindings

re-enabling review.  awaiting decision.  a compromise _has_ to be reached, here, so that its acceptance does not interfere with its maintenance.

[Luke Kenneth Casson Leighton]

plus - remember, jan: you're asking me to interfere with the core of the code, by maintaining two separate versions.  one that's useable and useful; the other that's an educational process for the webkit team.  the most appropriate time for that educational process to have taken place was: when the work was being developed - or at least a reasonable time thereafter.

we're now.... eight nearly TEN months after the initial development took place.

"going backwards" that far is simply impractical, when there are so many things.

ultimately, you will, at some point, simply go, "you know what? we're just going to have to trust that this code does the job.  we don't _like_ the fact that we don't fully understand it, but we _can_ satisfy ourselves that it works, by using that stupidly-named project, pyjamas-desktop, and that will just have to do".

i realise that it's a very different development style from that which you may be used to, and i can only apologise for it.

but i am _not_ going to go "backwards".  there is far, far too much left to do - several more months of work - being held up already.

l.

[Luke Kenneth Casson Leighton]

if you would like to check the last set of review comments
(made by sam) and responses, which cover this file, they can
be extracted here:

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

[Luke Kenneth Casson Leighton]

(In reply to comment #1)
> (From update of attachment 33020 [details])
> The patch is still is huge. If possible please trim it down to something that
> can build the bindings for Node.idl only. 

the answer's always been "no".

please respond with an answer indicating that you respect that decision,
and will accommodate it and will help move this forward.

[Mark Rowe (bdash)]

(In reply to comment #4)
> plus - remember, jan: you're asking me to interfere with the core of the code,
> by maintaining two separate versions.  one that's useable and useful; the other
> that's an educational process for the webkit team.  the most appropriate time
> for that educational process to have taken place was: when the work was being
> developed - or at least a reasonable time thereafter.

Luke, with all due respect, I talked with you on IRC before you'd even started writing any of this code.  At that time I told you that contributing incremental patches for a large feature was the only reasonable approach to take.  It's a process that every other WebKit contributor has found to be reasonable, and that has worked well for many significant features (a JavaScript bytecode interpreter, a JavaScript JIT compiler, SVG support, auto-generated JavaScript and Objective-C DOM bindings, WML support, ports of WebKit to Windows, Qt, GTK, wxWidgets, Haiku, … to name only a few).

[Sam Weinig]

Comment on attachment 33020 [details]
adds codegenerator for gobject language bindings

This still needs to be broken up.

[Luke Kenneth Casson Leighton]

(In reply to comment #7)
> (In reply to comment #4)
> > plus - remember, jan: you're asking me to interfere with the core of the code,
> > by maintaining two separate versions.  one that's useable and useful; the other
> > that's an educational process for the webkit team.  the most appropriate time
> > for that educational process to have taken place was: when the work was being
> > developed - or at least a reasonable time thereafter.
> 
> Luke, 

 hellooo.

> with all due respect, I talked with you on IRC before you'd even started
> writing any of this code.

 yes.

>  At that time I told you that contributing
> incremental patches for a large feature was the only reasonable approach to
> take. 

 *sigh*... it's such a long time ago that i honestly now don't remember.
 and, i was fairly focussed and committed to making the most of the time
 that i had available and had committed.  [and, to be honest, the
 significance would not have sunk in at that very early stage]

 that meant 100% focus.  i knew full well that if i stopped, the work would
 not be completed.  not because i would be lazy or anything, but because
 simply the sheer quantity of different levels of expertise required and
 the overwhelming number of functions, properties and objects - in order
 to keep a map of everything that's going on is just... too much to have
 to re-remember _even_ after a break of two weeks.

 i don't know if you can appreciate that - going well beyond reasonable
 mental boundaries, to focus on a specialist area so intensly that it
 causes actual mental pain and in some cases physical harm.

 i can tell you that i've done that several times, before now, in order
 to get results, and have recovered afterwards and had absolutely _no_
 desire to return to that time, whatsoever.

 so - it was the only way [that i personally could have done it], and
 now it's done, i'm not going through that again - not for anyone.


> It's a process that every other WebKit contributor has found to be
> reasonable, and that has worked well for many significant features (a
> JavaScript bytecode interpreter, a JavaScript JIT compiler, SVG support,
> auto-generated JavaScript and Objective-C DOM bindings, WML support, ports of
> WebKit to Windows, Qt, GTK, wxWidgets, Haiku, … to name only a few).

 i fully appreciate that.  gosh, who was it - sam i think who did the obj-c
 bindings - said that he managed an incremental process.

 what's different in this case is that sam knew the process: i didn't.

 so, i sent in a patch, and i waited, and waited, and nobody reviewed it,
 so i moved on with the work.

 _then_ i discover that there's this obscure box you have
 to put a "?" in that brings it to peoples' attention, but by then,
 it was far too late.

 i had already moved beyond the initial stages - the ones that i am being
 asked to commit time, money and resources to to "go back" to.

 if you want to "go backwards" - fine: you can look at and review those
 earlier patches.

 but i am really really sorry - i am _not_ going to go backwards to a time
 where i will have to drag things out of long-term memory at a time where
 i was absolutely focussed for eleven hour days and i do mean on nothing
 but these bindings.

 mistakes have been made, yes.  misunderstandings, yes.  go backwards? no.
 move forwards? it's truly up to you to come up with an initiative to do that.

[Luke Kenneth Casson Leighton]

(In reply to comment #8)
> (From update of attachment 33020 [details])
> This still needs to be broken up.

 ok - what would you suggest?  [bearing in mind that if you ask for anything that requires development work, the answer's no]

 how many lines of code would be reasonable?   i am happy to submit the first 150 lines for example.  a second patch  will add the next 150 lines.

 i don't believe that this is what you mean.

 development time for this code is _over_, sam.  it works.  it's too late to turn back the clock.  mistakes have been made: we cope with them.  misunderstandings have happened: we find them, go "whoops" and move on.

 you _have_ to accept the reality of the situation, some day.  which is that it is simply _too painful_ for me to return to "significant development" mode on this.

 so - accept this, and review this, you do need to do.

 sorry, sam.  back to you.

 think of _something_.  come on.

 think of a way in which this file can be reviewed - and accepted.

[Luke Kenneth Casson Leighton]

Comment on attachment 33020 [details]
adds codegenerator for gobject language bindings

sorry, sam - breaking it up isn't an option.
re-enabling review, to give people an opportunity
to come up with another idea.
options discussed and rejected so far:
 * doing Node.idl only [rejected several times]
 * breaking file up [method not described]
options suggested (only half-joking):
 * split into 10of 150-line patches

[Mark Rowe (bdash)]

(In reply to comment #10)
> (In reply to comment #8)
> > (From update of attachment 33020 [details] [details])
> > This still needs to be broken up.
> 
>  ok - what would you suggest?  [bearing in mind that if you ask for anything
> that requires development work, the answer's no]

Since you're unwilling to do further work on this patch, please stop wasting the time of reviewers by reposting it without modifications.

>  you _have_ to accept the reality of the situation, some day.  which is that it
> is simply _too painful_ for me to return to "significant development" mode on
> this.

It's disappointing to hear that, but your unwillingness to continue work on the patch does not mean that we should land it in the current state.  We've made this very clear.  We're more than happy to wait for someone to bring the patch up to a sufficient level of quality that we're comfortable with it being landed. 

>  think of a way in which this file can be reviewed - and accepted.

We've outlined several times what changes need to be made in order to make progress on this issue.  If you're not willing to make the necessary changes, that's fine, but please stop wasting our time.

[Luke Kenneth Casson Leighton]

(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > (From update of attachment 33020 [details] [details] [details])
> > > This still needs to be broken up.
> > 
> >  ok - what would you suggest?  [bearing in mind that if you ask for anything
> > that requires development work, the answer's no]
> 
> Since you're unwilling to do further work on this patch,

 that isn't the case.  my position was stated clearly once already,
 let me see if i can find it....  nope.  too many pages to go through.

 it went something like (and this is a clearer expression, anyway):

 * yes to maintenance work
 * yes to svn update merges
 * yes to future improvements (incremental ones of course) 
 * yes to future enhancements (incremental ones of course)
 * no to "going backwards" e.g. "just Node.idl".
 * no to removing features that destroy the ability to test the overall code
 * no to removing things that make it unusable [to pyjamas developers]
 * yes to bug-fixes
 * yes to coding-standards fixes and other obvious and simple fixes

 the "yes" things are easy.  simple.  the "no" things are time-consuming.
 painful.  or... necessary.


> please stop wasting
> the time of reviewers by reposting it without modifications.

 i'm sorry that you consider it to be a waste of time.  i'm waiting
 for review comments on the actual content of the file, such as,
 for example, agreement and acceptance of the responses here:

     https://bugs.webkit.org/show_bug.cgi?id=27410#c5

 or, any additional review comments.

 so i don't consider it to be a waste of time.  let's turn this around,
 and use a careful choice of words in doing so.  had the thought occurred
 that by shutting these reviews with no comments that it might be wasting
 _my_ time - by not reading my responses?

 in mirroring the thought that had occurred to you, that caused you to
 ask me to "stop wasting reviewers time", i invite you to think about
 that.



> >  you _have_ to accept the reality of the situation, some day.  which is that it
> > is simply _too painful_ for me to return to "significant development" mode on
> > this.
> 
> It's disappointing to hear that, but your unwillingness to continue work on the
> patch 

 i am perfectly willing to work on the patch - to move it _forward_, not
 backwards.

 remember that i have a responsibility to keep it in a working - and useful -
 state.

> does not mean that we should land it in the current state.  We've made
> this very clear.  We're more than happy to wait for someone to bring the patch
> up to a sufficient level of quality that we're comfortable with it being
> landed. 

 ah.

 see... here's the thing.  i am quite happy to be the person willing to
 do that - to bring it up to a level of "sufficient quality" [without
 "going backwards"].

 but, the last two reviewers have been unwilling to make any comments
 indicating what is lacking in the "quality".

 if reviewers are willing to provide reviews indicating what the lacking
 quality is, i am happy to answer them.


> >  think of a way in which this file can be reviewed - and accepted.
> 
> We've outlined several times what changes need to be made in order to make
> progress on this issue.

 then, with patience, do consider listing them here, as relevant to the
 file that's been submitted: CodeGeneratorGObject.pm.

 we can then keep track of them, in a manageable way.  i've made an
 effort in this regard, to make references to the last review comments,
 as you can see.

 i _was_ expecting that to happen [immediately] and i'm really 
 disappointed to find that we're up to comment 12 already, with
 no technical discussions entered into.

 if that _also_ turns out to be too much, then we will just have to
 think of something else.

 i am _not_ going to go away.  i am _not_ going to give up.  i am going
 to keep on and on at you, coming up with new suggestions, new ideas,
 badgering you to do the same, until you either give in and listen to my
 advice, or we come up with a workable solution that takes everyone's
 abilities and resources into account, or someone else comes in to help
 contribute.

 that's how it's going to be, i'm afraid.  i gave up on a free software
 project once and the consequences were disastrous, costing businesses
 world-wide billions of dollars.  i'm not going to give up again.

 so we think of something, ok?

 ah.

 idea.

 split CodeGeneratorGObject.pm into separate perl files.  i can then
 maintain working code and can submit files one at a time.

 what do you think?

[Luke Kenneth Casson Leighton]

ok.

i will wait to hear from you for an idea regarding a solution,
(and/or agreement on ideas proposed), to move forward.

if i do not hear from you within 24 hours, i will assume that,
by default, the current solution is acceptable, and will re-raise
the review flag accordingly on this file, on the assumption that
silence indicates a willingness to continue with the review
process, on this file, which was left off here:

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

l.

[Luke Kenneth Casson Leighton]

ok - i have a better idea.

let's start with something really small.  to which there can really be
no objections, because it is a start, and a stub, and contains
pretty much nothing.

if you are unable to provide me with proper guidance, guidance that would save all of us a lot of time, i have to instead... "guess" what you want.

i decided against utilising a binary search (cut to 750 lines, review, cut to 375 lines, review, cut to 170 lines, review) and instead decided to take the minimalist approach, to at least get started.

at any time, please feel free to provide guidance and feedback that will help, in the long run, save you - and me - vast amounts of time.

[Luke Kenneth Casson Leighton]

Created attachment 33038 [details]
absolute basic beginnings of CodeGeneratorGObject.pm. 

this patch contains two functions - the absolute basic beginnings of the code generator.  it can be seen to have zero impact on the existing codebase.  it is not compiled in.  it is not linked in.  it is perl.  it is not used anywhere.  it is a start.

[Mark Rowe (bdash)]

Comment on attachment 33038 [details]
absolute basic beginnings of CodeGeneratorGObject.pm. 

> +package CodeGeneratorGObject;
> +
> +require "CodeGeneratorGObjectLibrary1.pm";

This file doesn't exist as far as I can see.  What is it?  Why is it named in this manner?

> +
> +# Params: 'domClass' struct

What is this comment about?  The "Params" suggests it is about the arguments to the following function, but the function appears to take three arguments that have no relation to this comment.

> +sub GenerateInterface {
> +    my $object = shift;
> +    my $dataNode = shift;
> +    my $defines = shift;
> +
> +    my $name = $dataNode->name;
> +
> +    print "$name\n";

What's the purpose of this print statement?

> +
> +    $object->Generate($dataNode);
> +
> +    # Write changes
> +    my $fname = "Gdom_" . $name; #decamelize($name);

The comment here doesn't seem to add anything.  Why is there commented-out code here?

> +    $fname =~ s/_//g;
> +    $object->WriteData($fname);
> +}
> +
> +# Params: 'idlDocument' struct

Same comment as for the previous function applies to this comment.

Marking as r- due to these issues.

[Luke Kenneth Casson Leighton]

(In reply to comment #17)
> (From update of attachment 33038 [details])
> > +package CodeGeneratorGObject;
> > +
> > +require "CodeGeneratorGObjectLibrary1.pm";
> 
> This file doesn't exist as far as I can see.  What is it?  Why is it named in
> this manner?

 it contains the remainder of the code which will be reviewed separately.
 i'll add a comment to that effect.

> > +
> > +# Params: 'domClass' struct
> 
> What is this comment about?  

 i have no idea.  it was copied from the JSBindings code, by alp.

> The "Params" suggests it is about the arguments to
> the following function, but the function appears to take three arguments that
> have no relation to this comment.
> 
> > +sub GenerateInterface {
> > +    my $object = shift;
> > +    my $dataNode = shift;
> > +    my $defines = shift;
> > +
> > +    my $name = $dataNode->name;
> > +
> > +    print "$name\n";
> 
> What's the purpose of this print statement?

 i have no idea.  i am reluctant to remove it in case it is important.


> > +
> > +    $object->Generate($dataNode);
> > +
> > +    # Write changes
> > +    my $fname = "Gdom_" . $name; #decamelize($name);
> 
> The comment here doesn't seem to add anything.  Why is there commented-out code
> here?

 i have no idea.  perhaps it is there when there was the possibility
 that the DerivedSources/gdom/ files would be named Gdom_interface_name
 rather than GdomInterfaceName.


> > +    $fname =~ s/_//g;
> > +    $object->WriteData($fname);
> > +}
> > +
> > +# Params: 'idlDocument' struct
> 
> Same comment as for the previous function applies to this comment.

 same response: it's copied from the JS code, by alp.

here is a copy of the code from which it was copied (CodeGeneratorJS.pm)

you can see the same two-word comments, and the same "Params: 'domClass' struct" comments.

if you can find an explanation from the original authors as to why
these files contain these comments, i will be happy to attach them
and add their comments.

alternatively, the comments can be left as-is, as they match up
with the file from which they were copied.  to remove them would
only confuse people who would like to compare the two files to find
out how they work.

i am changing the comment to be the same as in CodeGeneratorJS.pm



sub finish
{
    my $object = shift;

    # Commit changes!
    $object->WriteData();
}

sub leftShift($$) {
    my ($value, $distance) = @_;
    return (($value << $distance) & 0xFFFFFFFF);
}

# Params: 'domClass' struct
sub GenerateInterface
{
    my $object = shift;
    my $dataNode = shift;
    my $defines = shift;

    # Start actual generation
    $object->GenerateHeader($dataNode);
    $object->GenerateImplementation($dataNode);

    my $name = $dataNode->name;

    # Open files for writing
    my $headerFileName = "$outputDir/JS$name.h";
    my $implFileName = "$outputDir/JS$name.cpp";

    open($IMPL, ">$implFileName") || die "Couldn't open file $implFileName";
    open($HEADER, ">$headerFileName") || die "Couldn't open file $headerFileName";
}

# Params: 'idlDocument' struct
sub GenerateModule
{
    my $object = shift;
    my $dataNode = shift;

    $module = $dataNode->module;
}

[Luke Kenneth Casson Leighton]

Created attachment 33086 [details]
answered review comments

answering review comments.  made functions in file more like the original from which it was copied (CodeGeneratorJS.pm).  added explanation of CodeGeneratorGObjectLibrary1.pm which is included in another patch.

[Luke Kenneth Casson Leighton]

print $name doesn't seem to be needed so it's been removed.
no consequences yet observed.

[Luke Kenneth Casson Leighton]

Comment on attachment 33086 [details]
answered review comments

cancelling review whilst auto-patch-maintenance script is written which puts ChangeLog at top of file

[Luke Kenneth Casson Leighton]

Comment on attachment 33086 [details]
answered review comments

cancelling review whilst auto-patch-maintenance script is written which puts ChangeLog at top of file

[Gour]

Created attachment 34467 [details]
uploaded

[Xan Lopez]

*** This bug has been marked as a duplicate of bug 33590 ***