Bug 27426

Summary: adding GDOMBinding.cpp/h as part of GObject bindings
Product: WebKit Reporter: Luke Kenneth Casson Leighton <lkcl>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Normal CC: a9016009, eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 16401, 27430, 28096, 28097, 28098, 28099, 28101, 28102, 28103, 28104    
Attachments:
Description Flags
adds GDOMBindings
none
corrections noted from other reviews (e.g. removal of args on prototypes)
eric: review-
split to multiple files, renamed #if 0 to #if __TODO_BUG_20586__
none
corrected GDOMHTMLCollectionCustom.cpp #include order
eric: review-
uploaded eric: review-

Description Luke Kenneth Casson Leighton 2009-07-19 13:37:52 PDT
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.
Comment 1 Luke Kenneth Casson Leighton 2009-07-19 13:45:49 PDT
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
Comment 2 Luke Kenneth Casson Leighton 2009-07-22 03:25:47 PDT
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.
Comment 3 Luke Kenneth Casson Leighton 2009-07-22 03:33:39 PDT
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.
Comment 4 Luke Kenneth Casson Leighton 2009-08-03 06:40:22 PDT
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.
Comment 5 Eric Seidel (no email) 2009-08-06 19:01:06 PDT
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?
Comment 6 Luke Kenneth Casson Leighton 2009-08-07 02:48:47 PDT
(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.
Comment 7 Luke Kenneth Casson Leighton 2009-08-07 09:51:08 PDT
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.
Comment 8 Luke Kenneth Casson Leighton 2009-08-07 09:52:24 PDT
Comment on attachment 34284 [details]
split to multiple files, renamed #if 0 to #if __TODO_BUG_20586__

whoops spotted out-of-order comments.
Comment 9 Luke Kenneth Casson Leighton 2009-08-07 09:56:06 PDT
Created attachment 34285 [details]
corrected GDOMHTMLCollectionCustom.cpp #include order

... whoops :)
Comment 10 Luke Kenneth Casson Leighton 2009-08-07 09:58:03 PDT
(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 :)
Comment 11 Eric Seidel (no email) 2009-08-07 12:48:08 PDT
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.
Comment 12 Luke Kenneth Casson Leighton 2009-08-07 15:26:44 PDT
(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*...
Comment 13 Gour 2009-08-10 08:21:00 PDT
Created attachment 34468 [details]
uploaded
Comment 14 Eric Seidel (no email) 2009-08-12 13:45:45 PDT
Comment on attachment 34468 [details]
uploaded

See my comments in the mail to you.
Comment 15 Andre Klapper 2009-08-12 18:00:09 PDT
(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.
Comment 16 Eric Seidel (no email) 2009-08-12 18:04:21 PDT
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. :)