65046 2011-07-22 14:20:14 -0700 Add parsing support for extended attributes on IDL constants 2011-08-29 13:36:39 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 paroga paroga webkit.review.bot oldest_to_newest 441160 0 paroga 2011-07-22 14:20:14 -0700 Add parsing support for extended attributes on IDL constants 441175 1 101766 paroga 2011-07-22 14:29:11 -0700 Created attachment 101766 Patch 458337 2 101766 bfulgham 2011-08-29 12:00:42 -0700 Comment on attachment 101766 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. 458378 3 101766 paroga 2011-08-29 12:42:07 -0700 Comment on attachment 101766 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 :-) 458382 4 101766 bfulgham 2011-08-29 12:47:58 -0700 Comment on attachment 101766 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! 458421 5 101766 webkit.review.bot 2011-08-29 13:36:35 -0700 Comment on attachment 101766 Patch Clearing flags on attachment: 101766 Committed r94010: <http://trac.webkit.org/changeset/94010> 458422 6 webkit.review.bot 2011-08-29 13:36:39 -0700 All reviewed patches have been landed. Closing bug. 101766 2011-07-22 14:29:11 -0700 2011-08-29 13:36:35 -0700 Patch bug-65046.patch text/plain 3812 paroga ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA3ZTU1MDk0Li5kNmIxZTdlIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjAg QEAKKzIwMTEtMDctMjIgIFBhdHJpY2sgR2Fuc3RlcmVyICA8cGFyb2dhQHdlYmtpdC5vcmc+CisK KyAgICAgICAgQWRkIHBhcnNpbmcgc3VwcG9ydCBmb3IgZXh0ZW5kZWQgYXR0cmlidXRlcyBvbiBJ REwgY29uc3RhbnRzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD02NTA0NgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIFdpdGggc3VwcG9ydCBmb3IgZXh0ZW5kZWQgYXR0cmlidXRlcyBvbiBjb25zdGFudHMgd2Ug Y2FuCisgICAgICAgIHJlcGxhY2Ugc29tZSBwcmVwcm9jZXNzb3IgbGluZXMgZnJvbSB0aGUgSURM IGZpbGVzIHdpdGgKKyAgICAgICAgdGhlIGNvcnJlc3BvbmRpbmcgW0NvbmRpdGlvbmFsPVhYWF0g bGF0ZXIuCisgICAgICAgIFVzaW5nIHRoZSBwcmVwcm9jZXNzb3IgY2F1c2VzIG11Y2ggcGFpbiBv biBuYXRpdmUgd2luZG93cy4KKworICAgICAgICBDb2RlR2VuZXJhdG9yIHN1cHBvcnQgd2lsbCBi ZSBhZGRlZCBpbiBzZXBhcmF0ZSBwYXRjaGVzLgorCisgICAgICAgICogYmluZGluZ3Mvc2NyaXB0 cy9JRExQYXJzZXIucG06CisgICAgICAgICogYmluZGluZ3Mvc2NyaXB0cy9JRExTdHJ1Y3R1cmUu cG06CisKIDIwMTEtMDctMjEgIFBhdmVsIEZlbGRtYW4gIDxwZmVsZG1hbkBnb29nbGUuY29tPgog CiAgICAgICAgIFdlYiBJbnNwZWN0b3I6IFtwcm90b2NvbF0gZXh0ZW5kIHNldEJyZWFrcG9pbnRC eVVybCByZXF1ZXN0IHRvIGFjY2VwdCBpbmNvbXBsZXRlIFVSTCAocHJlZGljYXRlcykKZGlmZiAt LWdpdCBhL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL3NjcmlwdHMvSURMUGFyc2VyLnBtIGIvU291 cmNlL1dlYkNvcmUvYmluZGluZ3Mvc2NyaXB0cy9JRExQYXJzZXIucG0KaW5kZXggMDUzNzRmNy4u MGNiOGNhNyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvYmluZGluZ3Mvc2NyaXB0cy9JRExQ YXJzZXIucG0KKysrIGIvU291cmNlL1dlYkNvcmUvYmluZGluZ3Mvc2NyaXB0cy9JRExQYXJzZXIu cG0KQEAgLTMyOSwxNCArMzI5LDE2IEBAIHN1YiBQYXJzZUludGVyZmFjZQogICAgICAgICAgICAg ICAgIHB1c2goQCRhcnJheVJlZiwgJG5ld0RhdGFOb2RlKTsKICAgICAgICAgICAgIH0gZWxzaWYg KCRsaW5lID1+IC9eXHMqY29uc3QvKSB7CiAgICAgICAgICAgICAgICAgJGxpbmUgPX4gLyRJRExT dHJ1Y3R1cmU6OmNvbnN0YW50U2VsZWN0b3IvOwotICAgICAgICAgICAgICAgIG15ICRjb25zdFR5 cGUgPSAoZGVmaW5lZCgkMSkgPyAkMSA6IGRpZSgiUGFyc2luZyBlcnJvciFcblNvdXJjZTpcbiRs aW5lXG4pIikpOwotICAgICAgICAgICAgICAgIG15ICRjb25zdE5hbWUgPSAoZGVmaW5lZCgkMikg PyAkMiA6IGRpZSgiUGFyc2luZyBlcnJvciFcblNvdXJjZTpcbiRsaW5lXG4pIikpOwotICAgICAg ICAgICAgICAgIG15ICRjb25zdFZhbHVlID0gKGRlZmluZWQoJDMpID8gJDMgOiBkaWUoIlBhcnNp bmcgZXJyb3IhXG5Tb3VyY2U6XG4kbGluZVxuKSIpKTsKKyAgICAgICAgICAgICAgICBteSAkY29u c3RFeHRlbmRlZEF0dHJpYnV0ZXMgPSAoZGVmaW5lZCgkMSkgPyAkMSA6ICIgIik7IGNob3AoJGNv bnN0RXh0ZW5kZWRBdHRyaWJ1dGVzKTsKKyAgICAgICAgICAgICAgICBteSAkY29uc3RUeXBlID0g KGRlZmluZWQoJDIpID8gJDIgOiBkaWUoIlBhcnNpbmcgZXJyb3IhXG5Tb3VyY2U6XG4kbGluZVxu KSIpKTsKKyAgICAgICAgICAgICAgICBteSAkY29uc3ROYW1lID0gKGRlZmluZWQoJDMpID8gJDMg OiBkaWUoIlBhcnNpbmcgZXJyb3IhXG5Tb3VyY2U6XG4kbGluZVxuKSIpKTsKKyAgICAgICAgICAg ICAgICBteSAkY29uc3RWYWx1ZSA9IChkZWZpbmVkKCQ0KSA/ICQ0IDogZGllKCJQYXJzaW5nIGVy cm9yIVxuU291cmNlOlxuJGxpbmVcbikiKSk7CiAKICAgICAgICAgICAgICAgICBteSAkbmV3RGF0 YU5vZGUgPSBuZXcgZG9tQ29uc3RhbnQoKTsKICAgICAgICAgICAgICAgICAkbmV3RGF0YU5vZGUt Pm5hbWUoJGNvbnN0TmFtZSk7CiAgICAgICAgICAgICAgICAgJG5ld0RhdGFOb2RlLT50eXBlKCRj b25zdFR5cGUpOwogICAgICAgICAgICAgICAgICRuZXdEYXRhTm9kZS0+dmFsdWUoJGNvbnN0VmFs dWUpOworICAgICAgICAgICAgICAgICRuZXdEYXRhTm9kZS0+ZXh0ZW5kZWRBdHRyaWJ1dGVzKHBh cnNlRXh0ZW5kZWRBdHRyaWJ1dGVzKCRjb25zdEV4dGVuZGVkQXR0cmlidXRlcykpOwogCiAgICAg ICAgICAgICAgICAgbXkgJGFycmF5UmVmID0gJGRhdGFOb2RlLT5jb25zdGFudHM7CiAgICAgICAg ICAgICAgICAgcHVzaChAJGFycmF5UmVmLCAkbmV3RGF0YU5vZGUpOwpkaWZmIC0tZ2l0IGEvU291 cmNlL1dlYkNvcmUvYmluZGluZ3Mvc2NyaXB0cy9JRExTdHJ1Y3R1cmUucG0gYi9Tb3VyY2UvV2Vi Q29yZS9iaW5kaW5ncy9zY3JpcHRzL0lETFN0cnVjdHVyZS5wbQppbmRleCBmOWRkNGFiLi5mM2Iz ZTJhIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9zY3JpcHRzL0lETFN0cnVj dHVyZS5wbQorKysgYi9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9zY3JpcHRzL0lETFN0cnVjdHVy ZS5wbQpAQCAtNzAsNiArNzAsNyBAQCBzdHJ1Y3QoIGRvbUNvbnN0YW50ID0+IHsKICAgICBuYW1l ID0+ICckJywgICAgICAjIERPTSBDb25zdGFudCBpZGVudGlmaWVyCiAgICAgdHlwZSA9PiAnJCcs ICAgICAgIyBUeXBlIG9mIGRhdGEKICAgICB2YWx1ZSA9PiAnJCcsICAgICAgIyBDb25zdGFudCB2 YWx1ZQorICAgIGV4dGVuZGVkQXR0cmlidXRlcyA9PiAnJCcsICMgRXh0ZW5kZWQgYXR0cmlidXRl cwogfSk7CiAKICMgSGVscGVycwpAQCAtOTMsNyArOTQsNyBAQCBvdXIgJGV4dGVuZGVkQXR0cmli dXRlU3ludGF4ID0gJ1xbW15dXSpcXSc7ICMgVXNlZCBmb3IgZXh0ZW5kZWQgYXR0cmlidXRlcwog IyBSZWd1bGFyIGV4cHJlc3Npb24gYmFzZWQgSURMICdzeW50YWN0aWNhbCB0b2tlbml6ZXInIHVz ZWQgaW4gdGhlIElETFBhcnNlcgogb3VyICRtb2R1bGVTZWxlY3RvciA9ICdtb2R1bGVccyooJyAu ICRpZGxJZCAuICcqKVxzKnsnOwogb3VyICRtb2R1bGVOU1NlbGVjdG9yID0gJ21vZHVsZVxzKign IC4gJGlkbElkIC4gJyopXHMqXFtuc1xzKignIC4gJGlkbElkTnMgLiAnKilccyooJyAuICRpZGxJ ZE5zIC4gJyopXF1ccyo7JzsKLW91ciAkY29uc3RhbnRTZWxlY3RvciA9ICdjb25zdFxzKicgLiAk c3VwcG9ydGVkVHlwZXMgLiAnXHMqKCcgLiAkaWRsVHlwZSAuICcqKVxzKj1ccyooJyAuICRjb25z dFZhbHVlIC4gJyknOworb3VyICRjb25zdGFudFNlbGVjdG9yID0gJ2NvbnN0XHMqKCcgLiAkZXh0 ZW5kZWRBdHRyaWJ1dGVTeW50YXggLiAnICk/JyAuICRzdXBwb3J0ZWRUeXBlcyAuICdccyooJyAu ICRpZGxUeXBlIC4gJyopXHMqPVxzKignIC4gJGNvbnN0VmFsdWUgLiAnKSc7CiBvdXIgJHJhaXNl c1NlbGVjdG9yID0gJ3JhaXNlc1xzKlwoKCcgLiAkaWRsSWROc0xpc3QgLiAnKilccypcKSc7CiBv dXIgJGdldHRlclJhaXNlc1NlbGVjdG9yID0gJ1xiZ2V0dGVyXHMrcmFpc2VzXHMqXCgoJyAuICRp ZGxJZE5zTGlzdCAuICcqKVxzKlwpJzsKIG91ciAkc2V0dGVyUmFpc2VzU2VsZWN0b3IgPSAnXGJz ZXR0ZXJccytyYWlzZXNccypcKCgnIC4gJGlkbElkTnNMaXN0IC4gJyopXHMqXCknOwo=