[Webkit-unassigned] [Bug 27425] adding GDOMHTMLElementWrapperFactory[.cpp/.h]

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Luke Kenneth Casson Leighton <lkcl at lkcl.net>  2009-07-21 10:56:20 PDT ---
(In reply to comment #2)
> (From update of attachment 33052 [details])
> > +2008-11-30  lkcl  <lkcl at lkcl.net>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        WARNING: NO TEST CASES ADDED OR CHANGED
> > +
> > +        https://bugs.webkit.org/show_bug.cgi?id=27425
> > +
> > +        * bindings/gdom/GDOMHTMLElementWrapperFactory.cpp: Added.  Copied from the auto-generated JSHTMLElementWrapperFactory.cpp 
> > +        * bindings/gdom/GDOMHTMLElementWrapperFactory.h:  Added.  Copied from the auto-generated JSHTMLElementWrapperFactory.h
> 
> You are missing a description of what this patch actually does.  Please fill
> out some description for what this patch is meant to provide.

 ahh, ok.  done.

> > +/* Note: This file is derived by hand from an automatically generated
> > + file.
> > + 
> > + Keeping it up-to-date could potentially be done by adding a
> > + make_names.pl generator, or by writing a separate generater which
> > + takes JSHTMLElementWrapperFactory.h as input.
> > +*/
> 
> Minor nit: s/generater/generator/

 duh.

> Regular nit: why is this generated file to be landed?  If it is to be updated
> in the future with a script, then why land it in the tree as static?  A
> placeholder until said script is written?

 basically, yes.  i'm not a perl-ite.  nobody's yet volunteered to do the
 script yet, so the copied-from-autogenerated-javascript-version will have
 to do.  and it does actually work.

 also, there's some extra work that's needed (planned) - adding back in
 ExecState, which i removed (daftly) to simplify the gobject bindings.

 so i've delayed both because it's .... just.... too much extra work,
 and what's here now "does the job".


> > +#include "config.h"
> > +
> > +#include "gdom/GdomNode.h"
> > +#include "GDOMHTMLElementWrapperFactory.h"
> 
> Order of includes is wrong.

 drat, sorry. ah... ahh.... what's that doing in there?  that's
 from GDOMBinding.cpp ahh oops sorry, thank you for spotting that
 but GDOMBinding.cpp shouldn't be in this patch!

 i've corrected GDOMBinding.cpp of course, so thank you for that,
 but it won't be in the next revision.


> There a lot of commented out pieces of code.  We generally don't land patches
> with commented out pieces of code.

 ack.  well, that will be for GDOMBinding.cpp

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.