[Webkit-unassigned] [Bug 26398] Move IDL extended attributes to the location specified in WebIDL

[bugzilla-daemon at webkit.org]

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





--- Comment #39 from Kentaro Hara <haraken at chromium.org>  2012-08-02 07:04:31 PST ---
(From update of attachment 155495)
View in context: https://bugs.webkit.org/attachment.cgi?id=155495&action=review

Thank you very much for updating the patch.

> Source/WebCore/ChangeLog:33
> +        * bindings/scripts/IDLParser.pm:
> +        To parse the existing WebKit IDL. Fixed the bug when parsing long
> +        longAttr. Expected result is int longAttr, but the previous one generates
> +        long long Attr.
> +        * bindings/scripts/IDLParserEditorsDraft.pm:
> +        To parse WebIDL based on Editor's Draft. However Editor's Draft doesn't
> +        have setraises, getraises and exception list. Modified the grammar to
> +        support these exceptions.

What's your plan to land this patch? Are you intending to land both IDLParser.pm and IDLParserEditorsDraft.pm?

We want to avoid over-engineering. IDLParser.pm does not have to be strictly conformed to the spec. I think it's OK to accept some temporal inconsistency. Maintainability would be more important.

> Source/WebCore/bindings/scripts/IDLParser.pm:44
> +my $beQuiet = 0;

IMHO, messages output by $beQuiet is not so helpful. Maybe you can remove $beQuiet.

> Source/WebCore/bindings/scripts/IDLParser.pm:113
> +    die $@ . " in $fileName" if $@;

More descriptive message please, so that developers can understand what happened.

> Source/WebCore/bindings/scripts/IDLParser.pm:115
> +    die "No document found" unless @definitions;

Ditto.

> Source/WebCore/bindings/scripts/IDLParser.pm:118
> +    die "No document found" unless ref($document) eq "idlDocument";

Ditto.

> Source/WebCore/bindings/scripts/IDLParser.pm:154
> +        $self->{LineNumber}++ while ($skipped =~ m/\n/g);

Nit: 'm' is not needed.

> Source/WebCore/bindings/scripts/IDLParser.pm:157
> +        $self->{DocumentContent} =~ /^([^\n\r]+)/;
> +        # Line is used for error output.
> +        if (defined($1)) {

This can be:

    if ($self->{DocumentContent} =~ /^([^\n\r]+)/)

> Source/WebCore/bindings/scripts/IDLParser.pm:163
> +    $self->{DocumentContent} =~ s/^([\t\n\r ]+)//;

Is this needed?

> Source/WebCore/bindings/scripts/IDLParser.pm:164
> +    if (length($self->{DocumentContent}) == 0) {

if ($self->{DocumentContent} eq "")

> Source/WebCore/bindings/scripts/IDLParser.pm:199
> +    die "Failed in tokenizing at " . $self->{Line};

Can you use $self->assertUnexpectedToken()?

> Source/WebCore/bindings/scripts/IDLParser.pm:210
> +        if (($next->type() == IdentifierToken) || ($next->value() =~ /^(::|dictionary|exception|interface|module|partial|typedef)$/)) {

Let's avoid hard-coding '::|dictionary|exception|interface|module|partial|typedef'.

Nit: The parentheses around the pattern is not needed.

Nit: Extra parentheses around each condition. The same comment for other parts.

> Source/WebCore/bindings/scripts/IDLParser.pm:226
> +    if (($next->type() == IdentifierToken) || ($next->value() =~ /^(::|dictionary|exception|interface|module|typedef)$/)) {

Ditto.

> Source/WebCore/bindings/scripts/IDLParser.pm:340
> +        if (($next->type() == IdentifierToken) || ($next->value() =~ /^(::|DOMString|Date|\[|any|attribute|boolean|byte|caller|const|creator|deleter|double|float|getter|long|object|octet|omittable|readonly|sequence|setter|short|static|unsigned|void)$/)) {

Let's avoid hard-coding. The same comment for all other hard-codings.

> Source/WebCore/bindings/scripts/IDLParser.pm:444
> +    return;

Nit: Not needed. The same comment for other parts.

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