[Webkit-unassigned] [Bug 106553] [V8] Add IDL Enum support WIP

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=106553





--- Comment #13 from Kentaro Hara <haraken at chromium.org>  2013-01-28 20:22:00 PST ---
(From update of attachment 185136)
View in context: https://bugs.webkit.org/attachment.cgi?id=185136&action=review

Looks almost OK. If you want to have the patch be reviewed, please set r?.

> LayoutTests/ChangeLog:12
> +        * http/tests/xmlhttprequest/xmlhttprequest-responseType.html: Added.
> +        * http/tests/xmlhttprequest/xmlhttprequest-responseType-expected.txt: Added.

Remove this change until you make a change to XMLHttpRequest.idl (you cannot make the change until you fix other code generators).

> Source/WebCore/ChangeLog:3
> +        [V8] Add IDL Enum support WIP

[V8] Add IDL Enum support

> Source/WebCore/ChangeLog:19
> +        Test: http/tests/xmlhttprequest/xmlhttprequest-responseType.html

Remove the line.

> Source/WebCore/ChangeLog:20
> +        Test: bindings/scripts/test/TestEnumeration.idl (run-bindings-test)

bindings/scripts/test/TestObj.idl

> Source/WebCore/ChangeLog:57
> +        * bindings/scripts/test/TestEnumeration.idl: Added.
> +         - New test for IDL enumeration support

TestObj.idl. It looks like you need to regenerate and update ChangeLog.

> Source/WebCore/ChangeLog:61
> +        * xml/XMLHttpRequest.idl:
> +         - Change responseType attribute from DOMString to enumeration, as
> +           test case

Remove this change from the patch. Otherwise, JSC will fail.

> Source/WebCore/bindings/scripts/IDLParser.pm:272
> +    my $unquotedString;
> +    ($unquotedString) = $quotedString =~ /^"([^"]*)"$/;
> +    die "Failed to parse string \"" . $quotedString . "\" at " . $self->{Line} if not defined $unquotedString;
> +    return $unquotedString;

In code generators we normally write:

  if ($quotedString =~ /^"(.*)"$/) {
    return $1;
  }
  die ...;

> Source/WebCore/bindings/scripts/IDLParser.pm:656
> +    my $numberOfExtendedAttributes = keys %{$extendedAttributeList};
> +    die "Extended attributes are not applicable to enumerations, but " . $numberOfExtendedAttributes . " present at " . $self->{Line} if $numberOfExtendedAttributes;

I don't think this check is needed. (If you want to insert this kind of checks, you have to insert more checks, which will mess up the code.)

> Source/WebCore/bindings/scripts/IDLParser.pm:687
> +    # value list must be non-empty

Really? (I'm not sure. Please check the spec.)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.