Bug 27383 – [Chromium] Hook up V8 bindings for DataGrid elements

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Jens Alfke]

The V8 JavaScript bindings for the HTML5 DataGrid elements are not hooked up. This causes the DataGrid layout tests to all fail. What needs to be done:

1. Make the JS objects inherit from the V8HTMLDataGrid etc. classes
2. Add indexed property handler for DataGridColumnList
3. Add named property handler for DataGridColumnList
4. Implement V8DataGridDataSource (as equivalent of JSC's JSDataGridDataSource)
5. Implement the custom getter/setter for HTMLDataGridElement.dataSource to use V8DataGridDataSource
6. Properly conditionalize everything so it builds when ENABLE(DATAGRID) is turned off (because Chromium wants this off in dev channel releases until DataGrid is more fully implemented.)

[Jens Alfke]

Created attachment 32965 [details]
Patch to hook up the V8 bindings

[Dimitri Glazkov (Google)]

Comment on attachment 32965 [details]
Patch to hook up the V8 bindings

Looks great except for needing to modify CodeGeneratorV8.pm to correctly handle the [Conditional] attr.

[Jens Alfke]

Created attachment 32985 [details]
2nd patch

Changes in this patch:
At Dimitri's request I took out the #ifdef I added to DOMWindow.idl, and instead fixed the V8 code generator to add #ifs around conditionally-enabled attributes (in GenerateBatchedAttributeData).
This Perl function had really messed up indentation that obscured the flow of control, so I fixed it; but that introduces a bunch of spurious diffs that are just whitespace changes. Sorry about that (but I felt this needed fixing, as it confused me on first reading.)

[Dimitri Glazkov (Google)]

Comment on attachment 32985 [details]
2nd patch

Great! Just one nit:

> Index: WebCore/bindings/v8/custom/V8HTMLDataGridElementCustom.cpp
> ===================================================================
> --- WebCore/bindings/v8/custom/V8HTMLDataGridElementCustom.cpp	(revision 46037)
> +++ WebCore/bindings/v8/custom/V8HTMLDataGridElementCustom.cpp	(working copy)
> @@ -29,7 +29,9 @@
>   */
>  
>  #include "config.h"
> +#include "Document.h"
>  #include "HTMLDataGridElement.h"
> +#include "V8DataGridDataSource.h"
>  
>  #include "NotImplemented.h"

We don't need this one anymore, right?

[Jeremy Orlow]

Assigned for landing.

Jens, can you roll another patch with the changes Dimitri suggested?

[Jens Alfke]

Created attachment 32988 [details]
3rd patch

OK, this patch simply gets rid of the #include "NotImplemented.h" that Dimitri correctly pointed out is no longer needed.

[Jeremy Orlow]

Landed in r46077

[Jeremy Orlow]

I believe you forgot a file in your patch (bindings/v8/custom/V8DataGridColumnListCustom.cpp) so we had to back it out in https://bugs.webkit.org/show_bug.cgi?id=27383

[Jeremy Orlow]

Oops...wrong link: https://bugs.webkit.org/show_bug.cgi?id=27407

[David Levin]

Assign to levin for landing.

[David Levin]

Comment on attachment 32988 [details]
3rd patch

Clearing r+ b/c this broke the build and marking as obsolete.

[David Levin]

Comment on attachment 32985 [details]
2nd patch

Clearing r+ to make this not show up in the commit queue.

[David Levin]

Assigning back to Jens.

[Jens Alfke]

Created attachment 33194 [details]
Patch, with missing file added

This is the patch that I mistakenly added to 27407 yesterday; sorry about that.
I've also added the '//' after '#endif'.

[Darin Fisher (:fishd, Google)]

Comment on attachment 33194 [details]
Patch, with missing file added

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

[Jens Alfke]

Created attachment 33266 [details]
5th patch

Addresses Darin's review comments above.
Note that the diffs of the layout tests are a lot messier because I've now indented most of the contents (since I wrapped them in 'try' blocks.)

[Jeremy Orlow]

Will land.

[Jens Alfke]

*** Bug 26684 has been marked as a duplicate of this bug. ***

[Jeremy Orlow]

Landed in 46239