Bug 33190

Summary: Missing commas in IDL extended attributes
Product: WebKit Reporter: Kent Tamura <tkent>
Component: WebCore JavaScriptAssignee: Kent Tamura <tkent>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, dglazkov, eric, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 33193    
Bug Blocks: 32854    
Attachments:
Description Flags
Proposed patch none

Description Kent Tamura 2010-01-04 18:13:14 PST
Some IDL files lack "," delimiters for multiple extended attributes.
For example,
WebCore/css/WebKitCSSTransformValue.idl

    interface [
            HasIndexGetter
            InterfaceUUID=303fe632-5dcf-4472-b977-33a5481e1d12,
            ImplementationUUID=eb49e5c6-6075-45b8-b5c4-7e775c01e7c4
    ] WebKitCSSTransformValue : CSSValueList {

There is no "," which should follow HasIndexGetter. In this case, both of HasIndexGetter and InterfaceUUID are not processed because they are recognized as a single attribute with spaces like "HasIndexGetter  InterfaceUUID".
Comment 1 Kent Tamura 2010-01-05 17:24:07 PST
Created attachment 45946 [details]
Proposed patch

- Fix commas
- Check such attributes by IDLParser.pm

Note: This patch will break Chromium build because of Bug#33193.
Comment 2 Darin Adler 2010-01-05 17:34:59 PST
Comment on attachment 45946 [details]
Proposed patch

Thanks for finding this. I would like to see test cases to demonstrate the bad thing that was happening due to HasIndexGetter and OmitConstructor being ignored in these two classes.
Comment 3 WebKit Review Bot 2010-01-05 17:56:44 PST
style-queue ran check-webkit-style on attachment 45946 [details] without any errors.
Comment 4 Kent Tamura 2010-01-05 18:07:14 PST
(In reply to comment #2)
> (From update of attachment 45946 [details])
> Thanks for finding this. I would like to see test cases to demonstrate the bad
> thing that was happening due to HasIndexGetter and OmitConstructor being
> ignored in these two classes.

This issue is not causing any real problems at this moment.

WebKitCSSTransformValue is missing HasIndexGetter and generated code for it had no code for index getter. Fortunately the parent interface of WebKitCSSTransformValue has HasIndexGetter code. So WebKitCSSTransformValue objects supprot index getter.

OmitConstructor is used only by CodeGeneratorJS. But JS binding excludes EventTarget.idl. (See JS_DOM_HEADERS in WebCore/DerivedSources.make)
Comment 5 WebKit Review Bot 2010-01-05 18:38:31 PST
Attachment 45946 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/163557
Comment 6 Eric Seidel (no email) 2010-01-06 09:21:01 PST
Comment on attachment 45946 [details]
Proposed patch

OK.  Please don't land until the chromium build fix is in.

Won't this patch fail to apply now anyway?  I thoguth we killed all of the COM stuff?
Comment 7 Eric Seidel (no email) 2010-01-06 09:31:06 PST
Attachment 45946 [details] was posted by a committer and has review+, assigning to Kent Tamura for commit.
Comment 8 Kent Tamura 2010-01-06 20:55:00 PST
(In reply to comment #6)
> (From update of attachment 45946 [details])
> OK.  Please don't land until the chromium build fix is in.
> 
> Won't this patch fail to apply now anyway?  I thoguth we killed all of the COM
> stuff?

We can apply this patch for now because the patch of Bug#32854 (Kill COM stuff) is not landed yet. Both of this and Bug#32854 trigger a Chromium build failure.
I'll commit Bug#33193 first, then this, and Bug#32854.
Comment 9 WebKit Commit Bot 2010-01-07 03:35:50 PST
Comment on attachment 45946 [details]
Proposed patch

Clearing flags on attachment: 45946

Committed r52915: <http://trac.webkit.org/changeset/52915>
Comment 10 WebKit Commit Bot 2010-01-07 03:36:02 PST
All reviewed patches have been landed.  Closing bug.