Bug 117507

Summary: Support latest Web IDL indexed property getters
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: BindingsAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, darin, haraken, laszlo.gombos
Priority: P2 Keywords: BlinkMergeCandidate
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://www.w3.org/TR/WebIDL/#dfn-support-indexed-properties
Attachments:
Description Flags
WIP Patch
none
Patch
none
Patch none

Description Chris Dumez 2013-06-11 06:29:52 PDT
We should support the latest Web IDL indexed property getters and stop using the outdated [IndexedGetter] IDL extended attribute.

This is already supported by Blink in:
https://chromiumcodereview.appspot.com/14751005
Comment 1 Chris Dumez 2013-06-11 08:25:40 PDT
Created attachment 204337 [details]
WIP Patch
Comment 2 Chris Dumez 2013-06-12 06:19:30 PDT
Created attachment 204430 [details]
Patch
Comment 3 Chris Dumez 2013-06-12 06:21:39 PDT
Created attachment 204431 [details]
Patch

Rebase on master.
Comment 4 Kentaro Hara 2013-06-12 06:26:39 PDT
Comment on attachment 204430 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204430&action=review

> Source/WebCore/bindings/scripts/CodeGenerator.pm:52
> +                       "octet" => 1);

I think "byte" and "octet" are supported neither CodeGeneratorJS.pm nor CodeGeneratorV8.pm. We might want to support them. (I think DataView's methods are written in custom bindings because of lack of support of "byte" and "octet".)

> Source/WebCore/html/RadioNodeList.idl:32
> +    getter Node ([IsIndex,Default=Undefined] optional unsigned long index);

Not related to this patch, I wonder why we need [IsIndex] in some getters and don't need it in other getters.
Comment 5 Kentaro Hara 2013-06-12 06:27:34 PDT
(In reply to comment #4)
> (From update of attachment 204430 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204430&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGenerator.pm:52
> > +                       "octet" => 1);
> 
> I think "byte" and "octet" are supported neither CodeGeneratorJS.pm nor CodeGeneratorV8.pm. We might want to support them. (I think DataView's methods are written in custom bindings because of lack of support of "byte" and "octet".)

Of course, in a follow-up patch.
Comment 6 Chris Dumez 2013-06-12 06:40:19 PDT
Comment on attachment 204430 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204430&action=review

>> Source/WebCore/bindings/scripts/CodeGenerator.pm:52
>> +                       "octet" => 1);
> 
> I think "byte" and "octet" are supported neither CodeGeneratorJS.pm nor CodeGeneratorV8.pm. We might want to support them. (I think DataView's methods are written in custom bindings because of lack of support of "byte" and "octet".)

Yes, I know. Better support will be needed later on. For now, this makes sure that numeric indexed getters are properly detected in the canvas typed arrays since I use those types now.

>> Source/WebCore/html/RadioNodeList.idl:32
>> +    getter Node ([IsIndex,Default=Undefined] optional unsigned long index);
> 
> Not related to this patch, I wonder why we need [IsIndex] in some getters and don't need it in other getters.

I was wondering the same. The only difference is that an INDEX_SIZE_ERR exception will be thrown if index < 0. I was thinking that maybe some specs specify this behavior while other do not? Just speculation, I am really not sure what the real reason is yet. However, it is important that such refactoring patch does not change the behavior.
Comment 7 WebKit Commit Bot 2013-06-12 07:41:32 PDT
Comment on attachment 204431 [details]
Patch

Clearing flags on attachment: 204431

Committed r151499: <http://trac.webkit.org/changeset/151499>
Comment 8 WebKit Commit Bot 2013-06-12 07:41:35 PDT
All reviewed patches have been landed.  Closing bug.