Summary: | [WebIDL] Add support for having dictionaries in their own IDL file | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
Component: | Bindings | Assignee: | Chris Dumez <cdumez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, darin, dino, ggaren, jiewen_tan, sam, simon.fraser | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 162929 | ||||||||||
Bug Blocks: | 161932 | ||||||||||
Attachments: |
|
Description
Chris Dumez
2016-10-04 09:58:27 PDT
👍 Should this be optional? Or should we require dictionaries to be in their own files? 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. Created attachment 290763 [details]
WIP Patch
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.
Created attachment 290776 [details]
Patch
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. Created attachment 290850 [details]
Patch
Comment on attachment 290850 [details] Patch Clearing flags on attachment: 290850 Committed r206877: <http://trac.webkit.org/changeset/206877> All reviewed patches have been landed. Closing bug. *** Bug 161932 has been marked as a duplicate of this bug. *** |