[webkit-reviews] review denied: [Bug 27383] [Chromium] Hook up V8 bindings for DataGrid elements : [Attachment 33194] Patch, with missing file added

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Jens Alfke
<snej at chromium.org>'s request for review:
Bug 27383: [Chromium] Hook up V8 bindings for DataGrid elements
https://bugs.webkit.org/show_bug.cgi?id=27383

Attachment 33194: Patch, with missing file added
https://bugs.webkit.org/attachment.cgi?id=33194&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
Looks great overall, but there are some stylistic nits that should be fixed
before this is committed.  See below:


> Index: WebCore/bindings/v8/V8DOMWrapper.cpp
...
> +#if ENABLE(DATAGRID)
> +    case V8ClassIndex::DATAGRIDCOLUMNLIST:
> +	  
descriptor->InstanceTemplate()->SetIndexedPropertyHandler(USE_INDEXED_PROPERTY_
GETTER(DataGridColumnList));
> +	  
descriptor->InstanceTemplate()->SetNamedPropertyHandler(USE_NAMED_PROPERTY_GETT
ER(DataGridColumnList));
> +	   break;
> +#endif
> +	 default:
>	   break;

nit: 4 space indent


> +#if ENABLE(DATAGRID)
> +#define FOR_EACH_DATAGRID_TAG(macro) 	      \
> +  macro(datagrid, DATAGRID)			      \
> +  macro(dcell, DATAGRIDCELL) 		      \
> +  macro(dcol, DATAGRIDCOL)			      \
> +  macro(drow, DATAGRIDROW)

nit: 4 space indent


> Index: WebCore/bindings/v8/custom/V8DataGridColumnListCustom.cpp
...
> +#include "config.h"
> +#include "Document.h"
> +#include "DataGridColumnList.h"

^^^ See my comment below about the misplaced Document.h include.


> Index: WebCore/bindings/v8/custom/V8HTMLDataGridElementCustom.cpp
...
>  #include "config.h"
> +#include "Document.h"
>  #include "HTMLDataGridElement.h"

^^^ This doesn't match convention (I believe).	Ordinarily, you should
include config.h and then the header corresponding to the .cpp file.
Then there should be a newline, and the rest of the includes.
V8NodeCustom.cpp is a good example to follow.


>  ACCESSOR_SETTER(HTMLDataGridElementDataSource)
...
> +    PassRefPtr<DataGridDataSource> dataSource;

nit: I think this should be a RefPtr.  PassRefPtr should only be
used in argument lists and as a return value.


>  } // namespace WebCore
> +
> +#endif ENABLE(DATAGRID)

^^^ I think you meant to comment out the ENABLE(DATAGRID) text.  It is quite
common in WebKit to not comment the #endif by the way.


> Index: LayoutTests/fast/dom/HTMLDataGridElement/DataGridColumns-basic.html
...
> +	   try {
> +
>	   function log(msg)

nit: I think it is good style to indent the body of a try block.


> Index:
LayoutTests/fast/dom/HTMLDataGridElement/DataGridColumns-dom-attributes.html
...
> +	   try{
> +
>	   function log(msg)

nit: I think it is good style to indent the body of a try block.

nit: Add a space after "try"


> Index: LayoutTests/fast/dom/HTMLDataGridElement/DataGridColumns-dom.html
...
> +	   
> +	   try {
>  
>	   function log(msg)

nit: I think it is good style to indent the body of a try block.


> Index: LayoutTests/fast/dom/HTMLDataGridElement/DataGridDataSource-basic.html

...
> +	   try {
> +	       
>	   function log(msg)

nit: I think it is good style to indent the body of a try block.

I think it would be good to provide a justification for the
"==" -> "===" change in the ChangeLog.