Bug 27426

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

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.

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

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

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 on attachment 34284 [details] split to multiple files, renamed #if 0 to #if __TODO_BUG_20586__ whoops spotted out-of-order comments.

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

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

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

Created attachment 34468 [details] uploaded

Comment on attachment 34468 [details] uploaded See my comments in the mail to you.

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

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