Created attachment 182112 [details] initial WIP ? Is this the correct Component (changing binding generation code) ? (My first patch here; very much WIP.) Working on responseType attribute of XMLHttpResponse interface, with V8 binding, to start. Patch currently allows IDL to include Enum declarations, and generates code treating it as a DOMString (current IDLs instead list Enums as DOMStrings). Needs to also create a C++ enumeration and validation code (currently done manually in WebCore files).
Comment on attachment 182112 [details] initial WIP View in context: https://bugs.webkit.org/attachment.cgi?id=182112&action=review Please add a ChangeLog. You can generate a ChangeLog by ./Tools/Scripts/prepare-ChangeLog. > Source/WebCore/bindings/scripts/CodeGenerator.pm:56 > +my %enumTypeHash = (); # populated from IDL WebKit is not likely to add such a non-helpful comment. Ditto for other parts. Please leave helpful comments only. This patch also contains a lot of debug comments. I'd be happy if you could remove them for review. > Source/WebCore/bindings/scripts/IDLParser.pm:1 > -# > +# Please do not edit unrelated code. Ditto for other style changes in this patch. Conventionally you are allowed to make style changes when you make some meaningful changes around it.
(In reply to comment #1) > (From update of attachment 182112 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=182112&action=review > > Please add a ChangeLog. You can generate a ChangeLog by ./Tools/Scripts/prepare-ChangeLog. Ok, will do. > > Source/WebCore/bindings/scripts/CodeGenerator.pm:56 > > +my %enumTypeHash = (); # populated from IDL > > WebKit is not likely to add such a non-helpful comment. Ditto for other parts. Please leave helpful comments only. Got it -- the reason for this comment is that all the other hashes (for various types) are static lists like: my %stringTypeHash = ("DOMString" => 1, "AtomicString" => 1); ...and only this one is (initially) empty, b/c we don't know what the Enum types are until we've parsed the IDL. It's clearer in context (bunch of hashes, one blank one which looks weird, with this comment explaining why it's empty), and I was trying to keep it terse; perhaps different wording or a bit more detail would help? > This patch also contains a lot of debug comments. I'd be happy if you could remove them for review. Of course -- the debug code is there while I'm working on it (and for own reference), but I'll remove it before submitting for review. > > Source/WebCore/bindings/scripts/IDLParser.pm:1 > > -# > > +# > > Please do not edit unrelated code. Ditto for other style changes in this patch. Conventionally you are allowed to make style changes when you make some meaningful changes around it. Ok, I was wondering about that; obviously actual patch shouldn't have unrelated style changes (I'll remove these before submitting actual patch), but I was wondering whether there was a place for style fixes. (These blank-looking changes are removing trailing whitespace, though I see that's clear from the colorized diff.)
Created attachment 182325 [details] Updated to address haraken's comments Updated to address concerns (i.e., style and noise). Still need to write Changelog (and of course implement C++ validation etc.). (Details below.) 1. Comment: > Source/WebCore/bindings/scripts/CodeGenerator.pm:56 > my %stringTypeHash = ("DOMString" => 1, "AtomicString" => 1); > > +my %enumTypeHash = (); # initially empty b/c populated from IDL > + > my %nonPointerTypeHash = ("DOMTimeStamp" => 1, "CompareHow" => 1); Hopefully comment is useful in context! As noted above: other hashes statically initialized (non-empty) and immutable, though there's no way to mark this in code, as you can't make constant hashes in core Perl. 2. Debug comments Removed notes to self (wrote in separate doc to self), except for debugging libraries and "working here", as currently using these. Only remaining note is: > + # nbarth: this never happens, as parseDefinitions returns > + # array of references to domInterface, not another document. > + # Maybe useful for inherits? > + # ...but can be simply implemented at parseDefinitions level > +# if ($#definitions == 0 && ref($definitions[0]) eq "idlDocument") { > +# $document = $definitions[0]; > +# } ...as I'm not 100% sure about it. This was a section of code that is never called, and assumes a weird "document within document", so I think it's safe to remove, as I've re-written top-level doc/definition processing, but I wanted to flag it for review. 3. Formatting Removed all unrelated formatting fixes. Only remaining formatting fix is: > struct( idlDocument => { > - interfaces => '@', # All parsed interfaces > - fileName => '$' # file name > + interfaces => '@', # All parsed interfaces > + enumerations => '@', # All parsed enumerations > + fileName => '$', # file name ...which is b/c I'm substantively editing this code. Formatting is make comment spacing follow WebKit "one space before EOL comments": http://www.webkit.org/coding/coding-style.html#comments-eol ...and add trailing comma (,) as other structs use it.
Created attachment 183905 [details] add validation, new files, and Changelog Updated patch to include validation code, the LayoutTest, and Changelog. Should be good for overall review; will break up into more digestible pieces.
Comment on attachment 183905 [details] add validation, new files, and Changelog View in context: https://bugs.webkit.org/attachment.cgi?id=183905&action=review Thanks for the patch! First round of comments. > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseType.html:4 > +<html> > +<head> > +<title>Test XmlHttpRequest responseType attribute handling</title> > +<meta http-equiv="content-type" content="text/html;charset=utf-8"> Once you include fast/resources/js/js-test-pre.js, you can use a bunch of utility functions for tests. For example, please refer to fast/events/constructors/progress-event-constructor.html. > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseType.html:11 > + // Based on: > + // response-encoding.html > + // xmlhttprequest-setrequestheader-no-name.html I think you can remove almost all comments from the file. WebKit is likely to leave really helpful comments only. > Source/WebCore/ChangeLog:10 > + Binding generators currently cannot handle IDL enumerations. Nit: Correct indentation please. Ditto for other parts. > Source/WebCore/ChangeLog:19 > + Test: http/tests/xmlhttprequest/xmlhttprequest-responseType.html You need to add run-bindings-tests. You can just add some enum to TestInterface.idl. See https://trac.webkit.org/wiki/WebKitIDL#RunBindingsTests > Source/WebCore/bindings/scripts/CodeGenerator.pm:56 > +my %enumTypeHash = (); # initially empty b/c populated from IDL Nit: Remove the comment. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1257 > + my $valBoolExp = join(" || ", map { "sv == \"$_\"" } @enumValues); $valBoolExp => $enumValidationExpression (WebKit uses fully qualified variable names) For readability, we're unlikely to use complicated syntax of Perl. You can write it using a for loop. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1259 > + const String sv = String(v); sv => string > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:4095 > + if ($codeGenerator->IsStringType($type) || > + $codeGenerator->IsEnumType($type)) { Nit: You don't need to wrap the line. > Source/WebCore/bindings/scripts/IDLParser.pm:47 > +# Used to represent definition with kind > +struct( domDefinition => { > + kind => '$', # Kind of definition e.g., interface, enum, etc. > + definition => '$', # Contents of definition > }); Why do you need this? > Source/WebCore/bindings/scripts/IDLParser.pm:100 > + extendedAttributes => '$', # Extended attributes (none in Web IDL, but potentially in Webkit IDL) This is not speced, and we have no use cases. So let's remove it. > Source/WebCore/bindings/scripts/IDLParser.pm:186 > +# Helper function for assertEnumerationValuesDistinct > +sub duplicateValue > +{ > + my %hash = (); > + foreach my $key (@_) { > + if (exists $hash{$key}) { > + return $key; # duplicate found > + } > + $hash{$key} = 1; > + } > + return; # no duplicates > +} > + > +sub assertEnumerationValuesDistinct > +{ > + my $self = shift; > + my $enum = shift; > + my $line = shift; > + my $value = duplicateValue(@{$enum->values}); > + if (defined $value) { > + my $msg = "Duplicate value \"" . $value . "\" in enumeration " . $enum->name . " at " . $self->{Line}; > + if (defined ($line)) { > + $msg .= " IDLParser.pm:" . $line; > + } > + die $msg; > + } > +} > + I think you can remove this. Per the Web IDL spec, it looks like value duplication is not a problem. > Source/WebCore/bindings/scripts/IDLParser.pm:321 > sub parseDefinitions > { > my $self = shift; > - my @definitions = (); > + my $document = idlDocument->new(); Per the Web IDL spec, parseDefinition should return an array of definitions. Why do you need this change? > Source/WebCore/bindings/scripts/IDLParser.pm:482 > + my $definition = domDefinition->new(); > $self->assertTokenValue($self->getToken(), "interface", __LINE__); > $self->assertTokenType($self->getToken(), IdentifierToken); > $self->assertTokenValue($self->getToken(), "{", __LINE__); > $self->parseInterfaceMembers(); > $self->assertTokenValue($self->getToken(), "}", __LINE__); > $self->assertTokenValue($self->getToken(), ";", __LINE__); > - return; > + $definition->kind("partial interface"); > + # FIXME: implement > + return $definition; It looks like you're introducing 'domDefinition' and saying "FIXME". Unless there is any strong need, I don't want to introduce 'domDefinition'. > Source/WebCore/xml/XMLHttpRequest.idl:29 > +enum ResponseType { "", "arraybuffer", "blob", "document", "json", "text" }; Remove "json" because we've not yet implemented it.
(In reply to comment #5) > (From update of attachment 183905 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183905&action=review > > Thanks for the patch! First round of comments. Thanks for the detailed comments! I'll answer below, then upload updated patch addressing them. Question: Once json is supported by responseType, I'll need to update the IDL and the layout test. This depends on WebKit Bug 73648. Where's the proper place to make a note of this to fix it? (Should I add a comment over at that bug once we've applied this patch, saying "don't close until update IDL & test"?) https://bugs.webkit.org/show_bug.cgi?id=73648 The most substantive comment below is regarding struct domDefinition Basically, I can use ref() instead. I was just using a wrapper instead of doing proper type introspection. http://en.wikipedia.org/wiki/Type_introspection#Perl Detailed (and at times lengthy) comments follow. > > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseType.html:4 > > +<html> > > +<head> > > +<title>Test XmlHttpRequest responseType attribute handling</title> > > +<meta http-equiv="content-type" content="text/html;charset=utf-8"> > > Once you include fast/resources/js/js-test-pre.js, you can use a bunch of utility functions for tests. For example, please refer to fast/events/constructors/progress-event-constructor.html. Thanks -- that's really useful! I just copied the simplest (i.e., shortest) existing test and worked from that; I'll rewrite so it's clean & proper. > > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseType.html:11 > > + // Based on: > > + // response-encoding.html > > + // xmlhttprequest-setrequestheader-no-name.html > > I think you can remove almost all comments from the file. WebKit is likely to leave really helpful comments only. Will do -- this was my first code (from Dec), so lots of "notes to self". > > Source/WebCore/ChangeLog:10 > > + Binding generators currently cannot handle IDL enumerations. > > Nit: Correct indentation please. Ditto for other parts. Oops! (Didn't realize WebKit uses spaces even in ChangeLogs! Fixed these, and fixed vim indent/highlighting to give correct behavior.) > > Source/WebCore/ChangeLog:19 > > + Test: http/tests/xmlhttprequest/xmlhttprequest-responseType.html > > You need to add run-bindings-tests. You can just add some enum to TestInterface.idl. See https://trac.webkit.org/wiki/WebKitIDL#RunBindingsTests Done locally, will upload. > > Source/WebCore/bindings/scripts/CodeGenerator.pm:56 > > +my %enumTypeHash = (); # initially empty b/c populated from IDL > > Nit: Remove the comment. Done. > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1257 > > + my $valBoolExp = join(" || ", map { "sv == \"$_\"" } @enumValues); > > $valBoolExp => $enumValidationExpression (WebKit uses fully qualified variable names) > > For readability, we're unlikely to use complicated syntax of Perl. You can write it using a for loop. Thanks -- I didn't know where to draw the line on variable names and concise/fancy code; I'll keep it clear. > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1259 > > + const String sv = String(v); > > sv => string (As above: tnx, didn't have good feel for conventions) > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:4095 > > + if ($codeGenerator->IsStringType($type) || > > + $codeGenerator->IsEnumType($type)) { > > Nit: You don't need to wrap the line. ! Ok -- looks like we don't wrap long lines. > > Source/WebCore/bindings/scripts/IDLParser.pm:47 > > +# Used to represent definition with kind > > +struct( domDefinition => { > > + kind => '$', # Kind of definition e.g., interface, enum, etc. > > + definition => '$', # Contents of definition > > }); > > Why do you need this? See discussion at top; I just added a wrapper instead of doing type introspection. In more detail: The fundamental problem is that the current IDLParser.pm code assumes all definitions are interfaces, and thus if you have a non-interface definition (such as an enumeration) there's no data structure to store it (or what *kind* of definition it is), nor any code path to handle it. So I made a new domEnumeration struct etc. for new definitions, and then I wrapped the existing domInterface and domEnumeration in domDefinition so we could just have a simple (typed) array of definitions (e.g., to return from parseDefinitions), and then handle them differently at the document level. This allowed pretty generic code and minimized changes to existing code (replace "interface" with "generic definition"), and just requires one packing and unpacking step per definition, but now that you mention it, it is clearer to just check the type. > > Source/WebCore/bindings/scripts/IDLParser.pm:100 > > + extendedAttributes => '$', # Extended attributes (none in Web IDL, but potentially in Webkit IDL) > > This is not speced, and we have no use cases. So let's remove it. Ok -- I'd just included for consistency, since the parser checked for them and other data structures had them, but as you note, spec says there are none, and but w/o use cases, it's just cruft. As per spec: http://www.w3.org/TR/WebIDL/#idl-enums "No extended attributes defined in this specification are applicable to enumerations." ...so I've removed this and make the parser bail if it finds any. > > Source/WebCore/bindings/scripts/IDLParser.pm:186 > > +# Helper function for assertEnumerationValuesDistinct > > +sub duplicateValue > > +{ > > + my %hash = (); > > + foreach my $key (@_) { > > + if (exists $hash{$key}) { > > + return $key; # duplicate found > > + } > > + $hash{$key} = 1; > > + } > > + return; # no duplicates > > +} > > + > > +sub assertEnumerationValuesDistinct > > +{ > > + my $self = shift; > > + my $enum = shift; > > + my $line = shift; > > + my $value = duplicateValue(@{$enum->values}); > > + if (defined $value) { > > + my $msg = "Duplicate value \"" . $value . "\" in enumeration " . $enum->name . " at " . $self->{Line}; > > + if (defined ($line)) { > > + $msg .= " IDLParser.pm:" . $line; > > + } > > + die $msg; > > + } > > +} > > + > > I think you can remove this. Per the Web IDL spec, it looks like value duplication is not a problem. Ok -- we're not validating the IDL (i.e., it's a non-validating parser). Spec says: http://www.w3.org/TR/WebIDL/#idl-enums "The list of enumeration values MUST NOT include duplicates." ...which is why I added a check, but I've removed it. > > Source/WebCore/bindings/scripts/IDLParser.pm:321 > > sub parseDefinitions > > { > > my $self = shift; > > - my @definitions = (); > > + my $document = idlDocument->new(); > > Per the Web IDL spec, parseDefinition should return an array of definitions. Why do you need this change? Oops -- I messed up; easy to fix. You're right, parseDefinition should just return an array of definitions, and then "sub Parse" should handle them. I incorrectly moved the boundary between code in "sub parseDefinition" and in "sub Parse" (parse the whole document) -- given a list of all the definitions, we want to sort this into interfaces, exceptions, enumerations, dictionaries etc. Previously the code assumed "definition = interface", but when I fixed it to distinguish these, I distinguished them too early. > > Source/WebCore/bindings/scripts/IDLParser.pm:482 > > + my $definition = domDefinition->new(); > > $self->assertTokenValue($self->getToken(), "interface", __LINE__); > > $self->assertTokenType($self->getToken(), IdentifierToken); > > $self->assertTokenValue($self->getToken(), "{", __LINE__); > > $self->parseInterfaceMembers(); > > $self->assertTokenValue($self->getToken(), "}", __LINE__); > > $self->assertTokenValue($self->getToken(), ";", __LINE__); > > - return; > > + $definition->kind("partial interface"); > > + # FIXME: implement > > + return $definition; > > It looks like you're introducing 'domDefinition' and saying "FIXME". Unless there is any strong need, I don't want to introduce 'domDefinition'. (See discussion at top.) I'm looking into this, to see if we can get rid of domDefinition. I do use domDefinition to distinguish interfaces, exceptions, and enumerations, and then other definitions in future. (Thus I added "domDefinition" to all definition kinds.) The added "FIXME" notes are a bit separate -- currently the code just silently ignores (parses but doesn't record) other types of definition, which confused me at first; adding a FIXME flags this for unfamiliar readers, and also shows where we've still got work in future, so I figured it was useful. > > Source/WebCore/xml/XMLHttpRequest.idl:29 > > +enum ResponseType { "", "arraybuffer", "blob", "document", "json", "text" }; > > Remove "json" because we've not yet implemented it. Ok -- didn't know where to handle "not yet implemented". This'll turn the layout test to a pass; I'll add a note in the IDL.
Created attachment 184180 [details] address concerns (rewrite parser changes, rewrite layout test), add bindings test Updated to address concerns - thanks for the comments, they've made the code much clearer and simpler! Parser changes (IDLParser.pm) now much simpler b/c no domDefinition wrapper. Layout test also much clearer and shorter due to using library. A few questions about FIXMEs (marking where current code is incomplete): Still have "FIXME" at a few places in the parser; this is exactly where the parser discards data (finishes parsing a definition and then has an empty return), so this marks where we'll need to work in future. Also put notes in IDL and layout test b/c WebCore doesn't yet support "json" as a responseType value, referencing the blocking bug. Also put note in bindings test IDL that we're not testing operations yet. (I looked around, and there are test cases out there -- interfaces using Web IDL with operations using enums -- but until we start converting these over, no practical test case.) My question is: Is this is the correct place for these? Should I have notes in the code? Raise a bug here on Bugzilla? Put it in personal "to do later" list? Some/all of the above? Thanks!
Comment on attachment 184180 [details] address concerns (rewrite parser changes, rewrite layout test), add bindings test 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.
(In reply to comment #8) Revised, mostly cleaned up layout test and merged bindings test to TestObj.idl. (Layout test now very simple.) Also, understood re: point of code comments -- only to explain tricky parts of code, not general “TODO (in future)”. Most significant issue is "why am I making parser changes at all?" (comment follows code below) Also a *question* (below) on how to deal with extended attributes on enumerations. Also a *question* (below) on whether we should only do bindings test for JSC and V8? > > 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. The current parser _parses_ IDLs, but _discards_ the data, except for interfaces and exceptions. In order to _record_ the data for enums, I have to make these changes -- they're essentially identical to the existing code for parseInterface. Contrast: - interface (where the data is recorded) and - dictionary (where the data is parsed but discarded). The code looks like: sub parseInterface ... my $interfaceNameToken = $self->getToken(); $self->assertTokenType($interfaceNameToken, IdentifierToken); $interface->name($interfaceNameToken->value()); ... return $interface; sub parseDictionary ... $self->assertTokenType($self->getToken(), IdentifierToken); ... return; parseInterface stores the token value in the $interface data structure and returns it, while parseDictionary just checks that it’s a valid identifier but discards its value and doesn’t return anything from the function -- it just validates the syntax. This confused me at first -- the IDLParser code clearly strictly follows the grammar in parsing, but at first I didn’t notice that it didn’t *do anything* with the data for unimplemented definitions. So to implement enum (or other definition kinds), at the parser level you have to change the parseEnum function (and related parseEnumValueList etc.), which I did following parseInterface. > > 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. Extended attributes on enums do *not* hit any asserts on current parser. The current parser parses them but discards them. I'm not sure the correct behavior: Web IDL _grammar_ allows extended attributes for any definition, but spec also says no extended attributes apply to enums. “No extended attributes defined in this specification are applicable to enumerations.” http://www.w3.org/TR/WebIDL/#idl-enums What should we do if run into extended attributes for an enum? Two natural options: (1) Silently ignore (current behavior); (2) Throw an error (via an assert). (My *guess* is (1) Silently ignore, as we seem to only be throwing errors for syntax errors, not for semantic errors (e.g., duplicate values in enumeration value list).) For now I’ve moved the checks into parseEnum/parseEnumOld themselves for better modularity, but can remove the checks if that’s preferred. > > 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? Ok -- didn’t know if this would be considered “getting in the way”. I put them in TestObj.idl, with an #if guard to only check for JSC and V8 (still need to implement JSC). Is this correct? (I understand we should implement for JSC and V8, but not others?) > > LayoutTests/ChangeLog:6 > > + Reviewed by Kentaro Hara. > > This line should be 'Reviewed by NOBODY (OOPS!)' until you get r+. Got it. > > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseType.html:61 > > [...fuzz test with random strings functions...] > > I think you don't need this. (I mean, over-engineering.) I was going to abstract this by writing a function fuzzTest that took a callback, assertion, and list of valid values and tested random values not on the list, but I thought better of it. (j/k) I just hardcoded one run’s worth of random values, which should be plenty.
Created attachment 184415 [details] revised to address concerns (Attachment seems to not have been attached?)
Comment on attachment 184415 [details] revised to address concerns View in context: https://bugs.webkit.org/attachment.cgi?id=184415&action=review Almost looks OK. > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseType.html:38 > +req.responseType = "ZdBrlAwOzlYbJII"; > +shouldBeEmptyString("req.responseType"); > +req.responseType = "U0q1V8kGQ"; > +shouldBeEmptyString("req.responseType"); > +req.responseType = "2T"; > +shouldBeEmptyString("req.responseType"); > +req.responseType = "XJaC9ON1mTIPIYEUNv"; > +shouldBeEmptyString("req.responseType"); > +req.responseType = "X02eXXO4J44r6Y"; > +shouldBeEmptyString("req.responseType"); Rather than testing various kind of strings, you can test various types ({foo: 1}, 123, 123.4, function(){}, document, null, undefined). > Source/WebCore/bindings/scripts/IDLParser.pm:677 > + 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; I understand some change like this is needed. Instead of using ad-hoc regular expressions, would you write it by strictly following the BNF of the Web IDL grammer? (http://www.w3.org/TR/WebIDL/#prod-Enum) Maybe you can use parseString() or something. > Source/WebCore/bindings/scripts/IDLParser.pm:697 > + 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; Ditto. > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:41 > +#include "JSTestEnumType.h" For JSC, you can fix it in a follow-up patch. > Source/WebCore/bindings/scripts/test/TestObj.idl:33 > +#if defined(TESTING_JS) || defined(TESTING_V8) Remove this. Alternatively, add a logic to CodeGenerator{ObjC,GObject,CPP}.pm to skip 'enum'. For now it's OK to keep the macro. Please fix it in a follow-up patch. > Source/WebCore/bindings/scripts/test/TestObj.idl:34 > +enum TestEnumType { "", "foo", "bar", " foo, bar, or zork", "ãã²" }; Remove multi-byte characters. Here we are not intending to test various strings. Rather, readability is important. So the test case could be: enum TestEnumType { "Enum1", "Enum2", "Enum3" };
Created attachment 185136 [details] updated (In reply to comment #11) > > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseType.html:38 > > +req.responseType = "ZdBrlAwOzlYbJII"; > > +shouldBeEmptyString("req.responseType"); [...] > Rather than testing various kind of strings, you can test various types ({foo: 1}, 123, 123.4, function(){}, document, null, undefined). Got it -- done. > > Source/WebCore/bindings/scripts/IDLParser.pm:677 > > + 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; > > I understand some change like this is needed. Instead of using ad-hoc regular expressions, would you write it by strictly following the BNF of the Web IDL grammer? (http://www.w3.org/TR/WebIDL/#prod-Enum) Maybe you can use parseString() or something. OIC -- the problem was dumping a regex in the middle of the parsing code, so you can't follow the BNF grammar. Per offline discussion, I've put the unquoting code in a utility function (unquoteString), so parseEnumValueList and parseEnumValues are legible. StringToken itself is still quoted (a string literal has quotes), so this doesn't affect any other code. > > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:41 > > +#include "JSTestEnumType.h" > > For JSC, you can fix it in a follow-up patch. > > > Source/WebCore/bindings/scripts/test/TestObj.idl:33 > > +#if defined(TESTING_JS) || defined(TESTING_V8) > > Remove this. Alternatively, add a logic to CodeGenerator{ObjC,GObject,CPP}.pm to skip 'enum'. For now it's OK to keep the macro. Please fix it in a follow-up patch. Ok -- for first cut just have V8 (with macro), then implement JSC, then comment out legacy (ObjC etc.) and remove macro from TestObj.idl > > Source/WebCore/bindings/scripts/test/TestObj.idl:34 > > +enum TestEnumType { "", "foo", "bar", " foo, bar, or zork", "ãã²" }; > > Remove multi-byte characters. Here we are not intending to test various strings. Rather, readability is important. So the test case could be: > > enum TestEnumType { "Enum1", "Enum2", "Enum3" }; Got it -- this is "code generation", not "torture test". I've changed as indicated, though I've retained the empty "", as that seems an important corner case (esp. if default value).
Comment on attachment 185136 [details] updated 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.)
Created attachment 185173 [details] updated -- final? (In reply to comment #13) > Looks almost OK. If you want to have the patch be reviewed, please set r?. Marked as r? -- may have a tweak or two left (a comment or two). Removed XMLHttpRequest.idl and layout test for now (blocked by JSC etc. fixes). Updated ChangeLog. > > 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 ...; Oops! =P Fixed. > > 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.) Understood -- removed. (We die on syntax errors; we're not doing semantic checks here.) Q: I've added a comment to the code ( sub parseEnum ) that we're ignoring the attributes, since currently the variable is set and ignored (which raises an eyebrow). Options include: 0. Ignore with comment (patch) 1. Ignored without comment (current) 2. Just not shift it (delete the my $extendedAttributeList = shift; line) -- then it's dropped earlier 3. Not pass it in at the sub parseDefinition level return $self->parseEnum($extendedAttributeList); return $self->parseEnum(); Any preferences? 3 is perhaps clearest since by visibly *not* passing it we're saying it's not used (surrounding lines pass it) so no comment is needed, but I understand if we want to drop it somewhere else or have identical code to other parse(SomeDefinition). > > Source/WebCore/bindings/scripts/IDLParser.pm:687 > > + # value list must be non-empty > > Really? (I'm not sure. Please check the spec.) Yup -- "non-empty" follows from the grammar. (EnumValueList is non-empty, but the tail/cdr EnumValues can be empty.) The current parser code is correct, but since this is potentially confusing I added a note. (Fine to remove if not esp. useful.) The spec would be clearer with a note in the text; it currently reads: "The enumeration values are specified as a comma-separated list of string literals. The list of enumeration values must not include duplicates." It would be clearer written something like: "The enumeration values are specified as a comma-separated list of string literals. The list of enumeration values *must not be empty and* must not include duplicates." (Non-empty is syntax, no duplicates is semantics, right?) http://www.w3.org/TR/WebIDL/#idl-enums http://www.w3.org/TR/WebIDL/#idl-grammar [20] Enum → "enum" identifier "{" EnumValueList "}" ";" [21] EnumValueList → string EnumValues [22] EnumValues → "," string EnumValues | ε Note that we don't have: *[21] EnumValueList → string EnumValues | ε
Comment on attachment 185173 [details] updated -- final? View in context: https://bugs.webkit.org/attachment.cgi?id=185173&action=review Looks OK to me. - Please click the "Submit for EWS analysis" button (https://bugs.webkit.org/show_bug.cgi?id=106553) to check builds/tests in WebKit bots. If they look fine, I'm happy to r+ the patch. - After getting r+, please set cq?. Then I can commit the patch. > Source/WebCore/bindings/scripts/IDLParser.pm:654 > + my $extendedAttributeList = shift; # ignored: Extended attributes are not applicable to enumerations This looks fine.
(In reply to comment #15) > - Please click the "Submit for EWS analysis" button (https://bugs.webkit.org/show_bug.cgi?id=106553) to check builds/tests in WebKit bots. If they look fine, I'm happy to r+ the patch. Just to clarify: If you set r?, the button is automatically clicked (so you don't need to click it.)
(In reply to comment #16) (In reply to comment #15) Q: Should we break this up into a few patches (for ease of review etc.)? It would be easy to make it 2 patches: one for basic parser/code gen guts, one for V8. With a little glue I could make it 3 patches: 1. basic parser changes 2. enum support in parser 3. V8 code gen > > - Please click the "Submit for EWS analysis" button (https://bugs.webkit.org/show_bug.cgi?id=106553) to check builds/tests in WebKit bots. If they look fine, I'm happy to r+ the patch. > > Just to clarify: If you set r?, the button is automatically clicked (so you don't need to click it.) Ok -- I see it's been doing various analyses. Q: EWS is "Early Warning System", right? (Various tests.) I just fast-forwarded/rebased git to avoid conflicts and check that I'm playing nice with recent changes. Should be ok, but wanna check just in case.
(In reply to comment #17) > Q: Should we break this up into a few patches (for ease of review etc.)? > It would be easy to make it 2 patches: > one for basic parser/code gen guts, one for V8. Actually I'd like to go without splitting. If you split the patch further, you cannot test individual patches. The basic approach would be "split your patch into reviewable-size patches but don't split your patch into untestable-size patches". > Q: EWS is "Early Warning System", right? (Various tests.) Yes.
Comment on attachment 185173 [details] updated -- final? Attachment 185173 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16202116 New failing tests: fast/workers/worker-document-leak.html
Comment on attachment 185173 [details] updated -- final? > fast/workers/worker-document-leak.html This would be false positive.
Comment on attachment 185173 [details] updated -- final? View in context: https://bugs.webkit.org/attachment.cgi?id=185173&action=review > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1263 > + const String string = String(v); This is an odd line. Why not: String string = v; ?
(In reply to comment #18) > (In reply to comment #17) > > Q: Should we break this up into a few patches (for ease of review etc.)? > > It would be easy to make it 2 patches: > > one for basic parser/code gen guts, one for V8. > > Actually I'd like to go without splitting. If you split the patch further, you cannot test individual patches. The basic approach would be "split your patch into reviewable-size patches but don't split your patch into untestable-size patches". Got it -- changing guts doesn't change behavior until it's used, so the only test would be "doesn't break/same as before", but doesn't show that it does what it's supposed to -- it's just an invisible internal change. (Only when there's some visible change can you tell that it works.)
(In reply to comment #21) > (From update of attachment 185173 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185173&action=review > > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1263 > > + const String string = String(v); > > This is an odd line. Why not: > > String string = v; > > ? No reason -- I'm not familiar with V8/WebCore conventions or classes; what should I do (well, you have a suggestion -- could you elaborate)? A couple questions: 1. Should we avoid explicit conversions? (Makes sense, just checking) so: "String string = v" instead of "String string = String(v)" ? 2. Should we use const -- I'm just converting so I can compare, and not modifying "string". so: "const String string = v" instead of "String string = v" ? 3. How should I be comparing strings? v is a V8StringResource I believe, which doesn't have a comparison, so I'm casting it to String and comparing those, which works; is it The Right Way to do it? Oh, and pleasure to be working with you, Mr. Barth.
> No reason -- I'm not familiar with V8/WebCore conventions or classes; > what should I do (well, you have a suggestion -- could you elaborate)? > > A couple questions: > 1. Should we avoid explicit conversions? > (Makes sense, just checking) > so: "String string = v" instead of "String string = String(v)" ? Generally it's fine to use implicit conversions. We declare many constructors "explicit", but for the ones that aren't marked "explicit", it's fine to use the implicit conversions. > 2. Should we use const -- I'm just converting so I can compare, > and not modifying "string". > so: "const String string = v" instead of "String string = v" ? Generally, we don't use "const" with String. I don't think there's any harm in using const in this context, it's just not something we do typically. > 3. How should I be comparing strings? > v is a V8StringResource I believe, which doesn't have a comparison, > so I'm casting it to String and comparing those, which works; > is it The Right Way to do it? That's a good question. Haraken might have some insight, but conversion to a String makes sense in this case because we'll need the String version in the common case anyway (i.e., when the comparisons pass). > Oh, and pleasure to be working with you, Mr. Barth. And you as well Mr. Barth.
"String string = v" looks fine. > > 3. How should I be comparing strings? > > v is a V8StringResource I believe, which doesn't have a comparison, > > so I'm casting it to String and comparing those, which works; > > is it The Right Way to do it? > > That's a good question. Haraken might have some insight, but conversion to a String makes sense in this case because we'll need the String version in the common case anyway (i.e., when the comparisons pass). It's a right usage of V8StringResource. V8StringResource is a "meta" class of String and AtomicString. V8StringResource is expected to be used by being implicitly converted into String or AtomicString when you need to access a string.
(In reply to comment #25) > "String string = v" looks fine. Ok -- should I post an updated patch? > V8StringResource is a "meta" class of String and AtomicString. V8StringResource is expected to be used by being implicitly converted into String or AtomicString when you need to access a string. Ok, that clarifies things -- thanks!
(In reply to comment #26) > Ok -- should I post an updated patch? yes
(In reply to comment #24) Thanks for replies! (Conversion) > Generally it's fine to use implicit conversions. We declare many constructors "explicit", but for the ones that aren't marked "explicit", it's fine to use the implicit conversions. Got it: explicit conversion only if required. (const) > Generally, we don't use "const" with String. I don't think there's any harm in using const in this context, it's just not something we do typically. Ok -- do we in general not use const (in V8/WebCore/Chrome), or does it vary with type or context? (Is it just "not worth bother with String"?) I generally use const, both internally to functions and in interfaces (for usual clarity/correctness/efficiency reasons), but I understand that it can add noise, and maintaining const-correctness can be a hassle, so I was wondering what Chrome standards and best practices are -- do we use it in Chrome and if so, how widely, and anything to be aware of?
(In reply to comment #28) > Ok -- do we in general not use const (in V8/WebCore/Chrome), or does it vary with type or context? (Is it just "not worth bother with String"?) No strict rule (although there had been a couple of discussions in webkit-dev@). It looks like we normally use const for a newly added code but don't use const for Strings.
(In reply to comment #29) > (In reply to comment #28) > > Ok -- do we in general not use const (in V8/WebCore/Chrome), or does it vary with type or context? (Is it just "not worth bother with String"?) > > No strict rule (although there had been a couple of discussions in webkit-dev@). It looks like we normally use const for a newly added code but don't use const for Strings. Ok, so I should use const for new code, except for String. (And also not go around adding it to old code.)
Created attachment 185453 [details] Patch
Comment on attachment 185453 [details] Patch ok
Comment on attachment 185453 [details] Patch Clearing flags on attachment: 185453 Committed r141360: <http://trac.webkit.org/changeset/141360>
All reviewed patches have been landed. Closing bug.