[webkit-reviews] review denied: [Bug 27425] adding auto-generator support for GDOMHTMLElementWrapperFactory[.cpp/.h] : [Attachment 34060] answers review comments.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Aug 4 08:51:56 PDT 2009


Eric Seidel <eric at webkit.org> has denied Luke Kenneth Casson Leighton
<lkcl at lkcl.net>'s request for review:
Bug 27425: adding auto-generator support for
GDOMHTMLElementWrapperFactory[.cpp/.h]
https://bugs.webkit.org/show_bug.cgi?id=27425

Attachment 34060: answers review comments.  
https://bugs.webkit.org/attachment.cgi?id=34060&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
I wasn't clear, sorry.	I agree that JS should be default.  I just wanted all
the places where you assumed !JS was Gdom to turn into explicit GDom checks.

passing no --bindingType can/should default to JS.  Otherwise you'd have to
change lots more files here. :)

327	    if ($binding eq 'Gdom') {
 328		 # skip DATAGRID for now
 329		 next if ($conditional eq 'DATAGRID');
 330	     }

Those can all go away if you just remove DATAGRID from the list of tags at the
start, no?  Removing it once instead of skipping it in every for loop seems
like a cleaner solution, or?  Also, don't you all have ENABLE_DATAGRID off
anyway?  Won't make_names just print #defines around all the DATAGRID stuff
which will be compiled out?

$F really should have a better name, per our style guidelines:
http://webkit.org/coding/coding-style.html.  Since you're just moving existing
code, I guess it's OK as is though.

So the GNUMakefile.am changes are fine, but not required if you restore the JS
default.

This in general looks great.  I think we still can fix the DATAGRID with a
cleaner hack.


More information about the webkit-reviews mailing list