Bug 33193

Summary:

[V8] CodeGeenratorV8 can't find item() or namedItem() in parent classes

Product:

WebKit

Reporter:

Kent Tamura <tkent>

Component:

WebCore JavaScript

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, commit-queue, dglazkov, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

32854, 33190

Attachments:

Description	Flags
Proposed patch	tkent: review-
Proposed patch (rev.2)	none

Description Kent Tamura 2010-01-04 19:42:14 PST

WebCore/CSS/WebKitCSSTransformValue.idl looks like having IndexGetter extended attributes. However, the attribute is ignored by Bug#33190.
If Bug#33190 is fixed or Bug#32854 is committed, we need to have a WebKitCSSTransformValue index-getter implementation for V8.

Comment 1 Kent Tamura 2010-01-04 21:07:28 PST

I investigated how to fix this and concluded it's a bug that CodeGeneratorV8.pm didn't understand WebKitCSSTransformValue had item() inherited from it's parent interface.

Comment 2 Kent Tamura 2010-01-04 21:41:53 PST

Created attachment 45862 [details]
Proposed patch

Comment 3 WebKit Review Bot 2010-01-04 21:59:57 PST

style-queue ran check-webkit-style on attachment 45862 [details] without any errors.

Comment 4 Adam Barth 2010-01-04 22:06:40 PST

Comment on attachment 45862 [details]
Proposed patch

I'm willing to believe that we need this patch, but does this fix a specific test or do we need to add a new test to show the improvement?

Comment 5 Kent Tamura 2010-01-05 02:28:41 PST

(In reply to comment #4)
> (From update of attachment 45862 [details])
> I'm willing to believe that we need this patch, but does this fix a specific
> test or do we need to add a new test to show the improvement?

I'm making a test, and probably we don't need to add index access setting to WebKitCSSTransformValue like this patch because its parent interface already has index access setting.

Comment 6 Dimitri Glazkov (Google) 2010-01-05 09:23:46 PST

Thanks so much for picking finding this! However, I don't think this is the right approach. We should instead work on making our template configuration aware of inheritance.

In the meantime, let's just add an exception to GenerateImplementationIndexer (see HTMLOptionCollection).

Comment 7 Kent Tamura 2010-01-05 20:40:15 PST

Created attachment 45953 [details]
Proposed patch (rev.2)

(In reply to comment #6)
> Thanks so much for picking finding this! However, I don't think this is the
> right approach. We should instead work on making our template configuration
> aware of inheritance.
> 
> In the meantime, let's just add an exception to GenerateImplementationIndexer
> (see HTMLOptionCollection).

Thank you for the comment.
Ok, the updated patch just adds a workaround for WebKitCSSTransformValue.

Comment 8 WebKit Review Bot 2010-01-05 20:41:10 PST

style-queue ran check-webkit-style on attachment 45953 [details] without any errors.

Comment 9 Adam Barth 2010-01-05 22:04:44 PST

Comment on attachment 45953 [details]
Proposed patch (rev.2)

Can we add a test for the behavior change?

Comment 10 Kent Tamura 2010-01-05 22:13:00 PST

(In reply to comment #9)
> (From update of attachment 45953 [details])
> Can we add a test for the behavior change?

Adding "," to "HasIndexGetter" in css/WebKitCSSTransformValue.idl causes a build error of Chromium. This change will fix it. I have no idea how to test this without a build error :-)

Comment 11 Adam Barth 2010-01-07 00:50:10 PST

Comment on attachment 45953 [details]
Proposed patch (rev.2)

tkent explained the issue to me over IRC and I understand now that this fixes a build error, which is even better than a test.  :)

Comment 12 WebKit Commit Bot 2010-01-07 02:52:11 PST

Comment on attachment 45953 [details]
Proposed patch (rev.2)

Clearing flags on attachment: 45953

Committed r52913: <http://trac.webkit.org/changeset/52913>

Comment 13 WebKit Commit Bot 2010-01-07 02:52:19 PST

All reviewed patches have been landed.  Closing bug.