[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 #8 from Kentaro Hara <haraken at chromium.org>  2013-01-23 02:43:53 PST ---
(From update of attachment 184180)
View in context: https://bugs.webkit.org/attachment.cgi?id=184180&action=review

> LayoutTests/ChangeLog:6
> +        Reviewed by Kentaro Hara.

This line should be 'Reviewed by NOBODY (OOPS!)' until you get r+.

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseType.html:15
> +shouldBe("req.responseType", "''");

You can use shouldBeEqualToString().

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseType.html:18
> +// FIXME: "json" is not supported yet

Remove this. This is written in the IDL file. WebKit doesn't want to add comments as much as possible to avoid out-of-dated comments.

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseType.html:19
> +// WebKit Bug 73648

Nit: Remove this line.

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseType.html:21
> +var valid_values = ["arraybuffer", "blob", "document", "text", ""];

WebKit uses a camelCaseVariableName.

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseType.html:26
> +    shouldBe("req.responseType", "'"+value+"'");

shouldBeEqualToString(). Ditto for other parts.

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseType.html:30
> +req.responseType = [1, 2]; // wrong type

Nit: Remove the comment.

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseType.html:32
> +req.responseType = "invalidvalue"; // invalid value

Nit: Remove the comment.

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseType.html:61
> +debug("Fuzz test: random invalid assignments -- should not change value");
> +function random_string()
> +{
> +    function random_range(max) {
> +        return Math.floor(Math.random() * max)
> +    }
> +    var maxlength = 20;
> +    var minlength = 1;
> +    var charset = "ABCDEFGHIJKLMNOPQRSTUVWXYZ" +
> +                  "abcdefghijklmnopqrstuvwxyz" +
> +                  "0123456789"; // [A-Za-z0-9]
> +    var length = minlength + random_range(maxlength - minlength);
> +    var text = "";
> +
> +    for(var i = 0; i < length; i++)
> +        text += charset.charAt(random_range(charset.length));
> +    return text;
> +}
> +
> +for (var i = 0, trials = 5; i < trials; i++) {
> +    do {
> +        random_value = random_string();
> +        // check random value not accidentally valid
> +    } while (valid_values.indexOf(random_value) > -1 )
> +    req.responseType = random_value;
> +    shouldBe("req.responseType", "''");
> +}

I think you don't need this. (I mean, over-engineering.)

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseType.html:63
> +// FIXME: add tests for responseType behavior (e.g., check state, environment, etc.)

Remove this. This will be already covered by other tests.

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

Looks redundant. If any error, you will notice it by hitting some asserts in the current IDLParser.

> Source/WebCore/bindings/scripts/IDLParser.pm:446
> +        # FIXME: implement

Remove this. At least, you should describe what should be implemented. (But I don't think this FIXME is needed.) Ditto for other parts.

> Source/WebCore/bindings/scripts/IDLParser.pm:701
>  sub parseEnumValueList
>  {
>      my $self = shift;
> +    my @values = ();
>      my $next = $self->nextToken();
>      if ($next->type() == StringToken) {
> -        $self->assertTokenType($self->getToken(), StringToken);
> -        $self->parseEnumValues();
> -        return;
> +        my $enumValueToken = $self->getToken();
> +        $self->assertTokenType($enumValueToken, StringToken);
> +        my $quotedValue = $enumValueToken->value();
> +        my $unquotedValue;
> +        ($unquotedValue) = $quotedValue =~ /^"([^"]*)"$/;
> +        push(@values, $unquotedValue);
> +        push(@values, @{$self->parseEnumValues()});
> +        return \@values;
>      }
> +    # value list must be non-empty
>      $self->assertUnexpectedToken($next->value(), __LINE__);
>  }
>  
>  sub parseEnumValues
>  {
>      my $self = shift;
> +    my @values = ();
>      my $next = $self->nextToken();
>      if ($next->value() eq ",") {
>          $self->assertTokenValue($self->getToken(), ",", __LINE__);
> -        $self->assertTokenType($self->getToken(), StringToken);
> -        $self->parseEnumValues();
> +        my $enumValueToken = $self->getToken();
> +        $self->assertTokenType($enumValueToken, StringToken);
> +        my $quotedValue = $enumValueToken->value();
> +        my $unquotedValue;
> +        ($unquotedValue) = $quotedValue =~ /^"([^"]*)"$/;
> +        push(@values, $unquotedValue);
> +        push(@values, @{$self->parseEnumValues()});
> +        return \@values;
>      }
> +    return \@values; # empty list (end of enumeration-values)

Why do you need this change? The current IDLParser is written strictly following the BNF of the Web IDL spec, so I think it can already parse enums correctly.

> Source/WebCore/bindings/scripts/test/TestEnumeration.idl:31
> +interface TestEnumInterface {

How about using existing IDL files (e.g. TestObj.idl) instead of adding a new IDL file?

> Source/WebCore/bindings/scripts/test/TestEnumeration.idl:38
> +   // operations
> +   // FIXME: operations not yet implemented
> +   // TestEnumType peek();
> +   // void poke(TestEnumType pokeType);
> +   // void pokeRepeatedly(TestEnumType... pokeTypes); // no test case

Remove this. This could be added when you really implement it. Although there is no rule when you should use FIXME, basically you can add FIXME only when you really want to warn people that "Please keep this in mind".

> Source/WebCore/xml/XMLHttpRequest.idl:30
> +// WebKit Bug 73648

Nit: This line is not needed.

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