Bug 33193 - [V8] CodeGeenratorV8 can't find item() or namedItem() in parent classes
Summary: [V8] CodeGeenratorV8 can't find item() or namedItem() in parent classes
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: Nobody
URL:
Keywords:
Depends on:
Blocks: 32854 33190
  Show dependency treegraph
 
Reported: 2010-01-04 19:42 PST by Kent Tamura
Modified: 2010-01-07 02:52 PST (History)
4 users (show)

See Also:


Attachments
Proposed patch (3.52 KB, patch)
2010-01-04 21:41 PST, Kent Tamura
tkent: review-
Details | Formatted Diff | Diff
Proposed patch (rev.2) (2.15 KB, patch)
2010-01-05 20:40 PST, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.