Bug 162912 – [WebIDL] Add support for having dictionaries in their own IDL file

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

Add support for having dictionaries in their own IDL file to allow:
- Sharing of the dictionary and its generated code between several interfaces.
- Allow using the generated dictionary code from custom bindings code.

[Simon Fraser (smfr)]

👍

[Sam Weinig]

Should this be optional? Or should we require dictionaries to be in their own files?

[Chris Dumez]

I think ideally it would be optional because I think it is common for dictionaries to be used in a single interface. However, if it adds to much complexity, I think it'd be fine to be a requirement.

[Chris Dumez]

Created attachment 290763 [details]
WIP Patch

[WebKit Commit Bot]

Attachment 290763 [details] did not pass style-queue:


ERROR: Source/WebCore/page/DOMWindow.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 290776 [details]
Patch

[Darin Adler]

Comment on attachment 290776 [details]
Patch

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

Looks OK to me. I worry a little about how often we open files just based on the mention of a class name, but this, I suppose, is the strategy we have been using.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:470
> +    if (exists $cachedExternalDictionaries->{$name}) {
> +        return $cachedExternalDictionaries->{$name};
> +    }

I personally like writing these as one-liners in perl.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:479
> +    open FILE, "<", $filename or die;
> +    my @lines = <FILE>;
> +    close FILE;
> +    my $fileContents = join('', @lines);

There’s a slightly more efficient way of doing this in perl using "slurp mode".

> Source/WebCore/bindings/scripts/CodeGenerator.pm:487
> +            if ($dictionary->name eq $name) {

Maybe "next unless" here?

> Source/WebCore/bindings/scripts/CodeGenerator.pm:491
> +                if ($dictionary->extendedAttributes->{"ImplementedAs"}) {
> +                    $dictionaryTypeImplementationNameOverrides{$dictionary->name} = $dictionary->extendedAttributes->{"ImplementedAs"};
> +                }

Might read nicer as a local variable plus a one-liner. Also no need for the quotes.

    my $implementedAs = $dictionary->extendedAttributes->{ImplementedAs};
    $dictionaryTypeImplementationNameOverrides{$dictionary->name} = $implementedAs if $implementedAs;

> Source/WebCore/bindings/scripts/CodeGenerator.pm:506
> +    # Dictionary names must start with an uppercase letter.
> +    return 0 unless $type =~ /^[A-Z]/;
> +
> +    return defined($object->GetDictionaryByName($type));

How about just using && here?

> Source/WebCore/bindings/scripts/CodeGenerator.pm:515
> +    return 0 unless $object->IsDictionaryType($type);
> +    return defined($cachedExternalDictionaries->{$type});

How about just using && here?

> Source/WebCore/bindings/scripts/preprocess-idls.pl:320
> +    return 1 if $fileContents =~ /callback\s+interface\s+\w+/gs;
> +    return 1 if $fileContents =~ /interface\s+\w+/gs;
> +    return 1 if $fileContents =~ /exception\s+\w+/gs;

Seems like these patterns should all start with "\b" so they don’t match the suffix of a word.

[Chris Dumez]

Created attachment 290850 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 290850 [details]
Patch

Clearing flags on attachment: 290850

Committed r206877: <http://trac.webkit.org/changeset/206877>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

*** Bug 161932 has been marked as a duplicate of this bug. ***