Add parsing support for extended attributes on IDL constants
Created attachment 101766 [details] Patch
Comment on attachment 101766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101766&action=review Looks good. r+, but please consider my suggestions. > Source/WebCore/bindings/scripts/IDLParser.pm:332 > + my $constExtendedAttributes = (defined($1) ? $1 : " "); chop($constExtendedAttributes); I'm not a fan of having two bits of logic on a single line like this. I'd rather see the 'chop' on its own line. > Source/WebCore/bindings/scripts/IDLStructure.pm:97 > +our $constantSelector = 'const\s*(' . $extendedAttributeSyntax . ' )?' . $supportedTypes . '\s*(' . $idlType . '*)\s*=\s*(' . $constValue . ')'; Does adding this new elements to the IDL structure require lots of the existing IDL to be changed? I think the answer is no (based on the EWS), but I just wanted to double-check.
Comment on attachment 101766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101766&action=review >> Source/WebCore/bindings/scripts/IDLParser.pm:332 >> + my $constExtendedAttributes = (defined($1) ? $1 : " "); chop($constExtendedAttributes); > > I'm not a fan of having two bits of logic on a single line like this. I'd rather see the 'chop' on its own line. It's only the same code as in ... http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/IDLParser.pm?rev=91770#L216 http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/IDLParser.pm?rev=91770#L245 http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/IDLParser.pm?rev=91770#L279 http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/IDLParser.pm?rev=91770#L312 >> Source/WebCore/bindings/scripts/IDLStructure.pm:97 >> +our $constantSelector = 'const\s*(' . $extendedAttributeSyntax . ' )?' . $supportedTypes . '\s*(' . $idlType . '*)\s*=\s*(' . $constValue . ')'; > > Does adding this new elements to the IDL structure require lots of the existing IDL to be changed? I think the answer is no (based on the EWS), but I just wanted to double-check. as the ? in the regexp already says: it's optional :-)
Comment on attachment 101766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101766&action=review Re-approved. r+ >>> Source/WebCore/bindings/scripts/IDLParser.pm:332 >>> + my $constExtendedAttributes = (defined($1) ? $1 : " "); chop($constExtendedAttributes); >> >> I'm not a fan of having two bits of logic on a single line like this. I'd rather see the 'chop' on its own line. > > It's only the same code as in ... > http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/IDLParser.pm?rev=91770#L216 > http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/IDLParser.pm?rev=91770#L245 > http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/IDLParser.pm?rev=91770#L279 > http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/IDLParser.pm?rev=91770#L312 Okay -- existing precedent trumps personal opinion :-) >>> Source/WebCore/bindings/scripts/IDLStructure.pm:97 >>> +our $constantSelector = 'const\s*(' . $extendedAttributeSyntax . ' )?' . $supportedTypes . '\s*(' . $idlType . '*)\s*=\s*(' . $constValue . ')'; >> >> Does adding this new elements to the IDL structure require lots of the existing IDL to be changed? I think the answer is no (based on the EWS), but I just wanted to double-check. > > as the ? in the regexp already says: it's optional :-) Great!
Comment on attachment 101766 [details] Patch Clearing flags on attachment: 101766 Committed r94010: <http://trac.webkit.org/changeset/94010>
All reviewed patches have been landed. Closing bug.