Bug 27383 - [Chromium] Hook up V8 bindings for DataGrid elements
Summary: [Chromium] Hook up V8 bindings for DataGrid elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jeremy Orlow
URL:
Keywords:
: 26684 (view as bug list)
Depends on:
Blocks: 26684
  Show dependency treegraph
 
Reported: 2009-07-17 11:52 PDT by Jens Alfke
Modified: 2009-07-22 14:47 PDT (History)
3 users (show)

See Also:


Attachments
Patch to hook up the V8 bindings (30.80 KB, patch)
2009-07-17 13:02 PDT, Jens Alfke
dglazkov: review-
Details | Formatted Diff | Diff
2nd patch (43.76 KB, patch)
2009-07-17 15:53 PDT, Jens Alfke
no flags Details | Formatted Diff | Diff
3rd patch (43.77 KB, patch)
2009-07-17 16:26 PDT, Jens Alfke
no flags Details | Formatted Diff | Diff
Patch, with missing file added (47.44 KB, patch)
2009-07-21 11:44 PDT, Jens Alfke
fishd: review-
Details | Formatted Diff | Diff
5th patch (64.87 KB, patch)
2009-07-22 10:10 PDT, Jens Alfke
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jens Alfke 2009-07-17 11:52:42 PDT
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.)
Comment 1 Jens Alfke 2009-07-17 13:02:22 PDT
Created attachment 32965 [details]
Patch to hook up the V8 bindings
Comment 2 Dimitri Glazkov (Google) 2009-07-17 14:30:30 PDT
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.
Comment 3 Jens Alfke 2009-07-17 15:53:28 PDT
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.)
Comment 4 Dimitri Glazkov (Google) 2009-07-17 15:58:32 PDT
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?
Comment 5 Jeremy Orlow 2009-07-17 16:02:43 PDT
Assigned for landing.

Jens, can you roll another patch with the changes Dimitri suggested?
Comment 6 Jens Alfke 2009-07-17 16:26:45 PDT
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.
Comment 7 Jeremy Orlow 2009-07-17 18:44:17 PDT
Landed in r46077
Comment 8 Jeremy Orlow 2009-07-18 00:57:37 PDT
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
Comment 9 Jeremy Orlow 2009-07-18 01:01:27 PDT
Oops...wrong link: https://bugs.webkit.org/show_bug.cgi?id=27407
Comment 10 David Levin 2009-07-21 11:18:00 PDT
Assign to levin for landing.
Comment 11 David Levin 2009-07-21 11:38:29 PDT
Comment on attachment 32988 [details]
3rd patch

Clearing r+ b/c this broke the build and marking as obsolete.
Comment 12 David Levin 2009-07-21 11:38:57 PDT
Comment on attachment 32985 [details]
2nd patch

Clearing r+ to make this not show up in the commit queue.
Comment 13 David Levin 2009-07-21 11:41:27 PDT
Assigning back to Jens.
Comment 14 Jens Alfke 2009-07-21 11:44:40 PDT
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'.
Comment 15 Darin Fisher (:fishd, Google) 2009-07-21 16:07:25 PDT
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.
Comment 16 Jens Alfke 2009-07-22 10:10:41 PDT
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.)
Comment 17 Jeremy Orlow 2009-07-22 14:21:19 PDT
Will land.
Comment 18 Jens Alfke 2009-07-22 14:24:06 PDT
*** Bug 26684 has been marked as a duplicate of this bug. ***
Comment 19 Jeremy Orlow 2009-07-22 14:47:56 PDT
Landed in 46239