[webkit-reviews] review denied: [Bug 27425] adding GDOMHTMLElementWrapperFactory[.cpp/.h] : [Attachment 33052] adds gdomhtmlelementwrapperfactory.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jul 20 19:51:39 PDT 2009


Adam Treat <treat at kde.org> has denied Luke Kenneth Casson Leighton
<lkcl at lkcl.net>'s request for review:
Bug 27425: adding GDOMHTMLElementWrapperFactory[.cpp/.h]
https://bugs.webkit.org/show_bug.cgi?id=27425

Attachment 33052: adds gdomhtmlelementwrapperfactory.
https://bugs.webkit.org/attachment.cgi?id=33052&action=review

------- Additional Comments from Adam Treat <treat at kde.org>
> +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.

> +/* 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/

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?

> +
> +
> +#include "config.h"
> +
> +#include "gdom/GdomNode.h"
> +#include "GDOMHTMLElementWrapperFactory.h"

Order of includes is wrong.   See the WebKit coding style guidelines for order
of includes.

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

Beyond that, without a more detailed Changelog it makes it hard to review.

r- for the above reasons.


More information about the webkit-reviews mailing list