Bug 27426 – adding GDOMBinding.cpp/h as part of GObject bindings

• Status: RESOLVED
• Resolution: INVALID
• Component: WebKit Misc.

[Luke Kenneth Casson Leighton]

this is part of a series of patches to add gobject bindings.
it is split into a series under an agreement arranged by david.

this patch adds the essential mapping/bindings between gobject objects and webkit c++ objects.  it is absolutely critical that an existing c++ object NOT be allocated a different gobject object, even if the c++ object has been typecast to something wildly different (further down the base class hierarchy).

the exact same absolutely essential trick is deployed in both obj-c and JS bindings.

[Luke Kenneth Casson Leighton]

Created attachment 33053 [details]
adds GDOMBindings

from memory, review comments regarding these files:

* why [were] #ifdef 0s added around some code?

  answer: because they might be needed for debugging purposes, just as they might be needed in JSDOMBindings.cpp

after a few attempts to keep that code in, just in case, reviewers were so insistent that i decided it wasn't worth messing around with, and removed them.

* why are the headers not in strict alphabetical order?

 answer: whereas in the JS Bindings, the functions are split across several files (toJS), it was decided to simply cut/paste the copies of the functions from which the toGDOM versions are derived into one single file.  the headers were therefore grouped together from the places from whence they came, and "s/JS/GDOM/g" was then applied.  it _may_ make sense to re-split GDOMBinding.cpp back into separate files, which would be made all the easier by keeping the header files grouped together.  a comment [somewhat] to this effect has been placed at the top of the file GDOMBinding.cpp

[Luke Kenneth Casson Leighton]

Comment on attachment 33053 [details]
adds GDOMBindings

this file was accidentally submitted as part of another patch, and got accidentally reviewed :)

to save reviewer time, review is being removed pending update of the comments from the other accidental review.

[Luke Kenneth Casson Leighton]

in the following:

https://bugs.webkit.org/show_bug.cgi?id=27425#c2

it was noted that the accidentally added GDOMBinding.cpp to that patch contained out-of-order #includes.  these will be corrected.

[Luke Kenneth Casson Leighton]

Created attachment 33972 [details]
corrections noted from other reviews (e.g. removal of args on prototypes)

please note: the ordering of the #includes is noted as being out-of-order [strictly speaking] but there is a comment advising that they are not out-of-order, they're grouped together by purpose.  the reason is because this file is equivalent to and was constructed out of many JS*Binding*.cpp files.  if ever this one file GDOMBindings.cpp is split down into many files, it will be extra work for developers to re-construct the header dependencies of the groupings.

[Eric Seidel (no email)]

Comment on attachment 33972 [details]
corrections noted from other reviews (e.g. removal of args on prototypes)

Bunch of Style violations.

Please don't post patches with #if 0 code.

There are a billion includes in this file.  maybe we should split some of this out into separate files?

[Luke Kenneth Casson Leighton]

(In reply to comment #5)
> (From update of attachment 33972 [details])
> Bunch of Style violations.
> 
> Please don't post patches with #if 0 code.

 mmm... the job of the next person who works on this will be made that much harder if that code is removed.  with the #if 0 all they need do is go, "oh, that's all i need to do, remove the #if 0".

without the #if 0 they will go, "wtf am i supposed to be doing?  err, where did this file come from originally, oh, it was cut/paste together from a ton of JSBinding files, where are those, err, what's the difference between this file and that one"....

so, to save someone a lot of hassle, what is the best procedure to leave code in that will make peoples' lives a lot easier [e.g. when adding SVG Canvas]

 
> There are a billion includes in this file.  maybe we should split some of this
> out into separate files?

 mmm... yehhh.

[Luke Kenneth Casson Leighton]

Created attachment 34284 [details]
split to multiple files, renamed #if 0 to #if __TODO_BUG_20586__

ok the other alternative would be #if ENABLE(TODO_GOBJECT_SVG) or maybe #if ENABLE(GOBJECT_SVG) which would of course not exist, which would be the whole point: disable the code until gobject bindings get 2D SVG Canvas [in a few months time].

i'm _really_ reluctant to have that code cut out.  NULL is returned at the moment, deliberately, to enforce non-availability of 2D Canvas objects.  anyone hunting through the code _without_ the #ifdef'd TODO on it will definitely have a "wtf" moment.

[Luke Kenneth Casson Leighton]

Comment on attachment 34284 [details]
split to multiple files, renamed #if 0 to #if __TODO_BUG_20586__

whoops spotted out-of-order comments.

[Luke Kenneth Casson Leighton]

Created attachment 34285 [details]
corrected GDOMHTMLCollectionCustom.cpp #include order

... whoops :)

[Luke Kenneth Casson Leighton]

(In reply to comment #5)
 
> There are a billion includes in this file.  maybe we should split some of this
> out into separate files?

 done.  took fiiive houuurs :)

[Eric Seidel (no email)]

Comment on attachment 34285 [details]
corrected GDOMHTMLCollectionCustom.cpp #include order

The purpose of splitting it out into separate files was so that they coudl be reviewed individually.

[Luke Kenneth Casson Leighton]

(In reply to comment #11)
> (From update of attachment 34285 [details])
> The purpose of splitting it out into separate files was so that they coudl be
> reviewed individually.

ohhhh.

*sheepish*...

[Gour]

Created attachment 34468 [details]
uploaded

[Eric Seidel (no email)]

Comment on attachment 34468 [details]
uploaded

See my comments in the mail to you.

[Andre Klapper]

(In reply to comment #14)
> (From update of attachment 34468 [details])
> See my comments in the mail to you.

I did not receive that email. For the sake of transparency please keep technical debates in public comments. Thanks.

[Eric Seidel (no email)]

A second mail was sent to webkit-dev, which is public.
https://lists.webkit.org/pipermail/webkit-dev/2009-August/009456.html

The first mail, mentioned above, was not a technical discussion, but rather a private disparagement for having spammed the patch system. :)