Simplify [RequiresExistingAtomicString] IDL extended attribute handling in the bindings generator.
Created attachment 278370 [details] Patch
Comment on attachment 278370 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278370&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4382 > + return ("AtomicString($value.toString(state)->toExistingAtomicString(state))", 1) if $signature->extendedAttributes->{"RequiresExistingAtomicString"}; The parentheses here after "return" are not normal perl coding style. I like it better without. It’s irritating that we have to construct AtomicString rather than just passing the result of toExistingAtomicString. Does it compile without the explicit AtomicString?
Comment on attachment 278370 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278370&action=review >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4382 >> + return ("AtomicString($value.toString(state)->toExistingAtomicString(state))", 1) if $signature->extendedAttributes->{"RequiresExistingAtomicString"}; > > The parentheses here after "return" are not normal perl coding style. I like it better without. > > It’s irritating that we have to construct AtomicString rather than just passing the result of toExistingAtomicString. Does it compile without the explicit AtomicString? 1. We use these parentheses in all return statements in this function. Do you want be to drop them all? I am not Perl expert but I personally like the parentheses here as it makes it clear we're returning a tuple. 2. Yes, I have tried without the AtomicString() but it did not build. The compiler complains about it being ambiguous because there are 2 overloads in TreeScope.h: Element* getElementById(const AtomicString&) const; WEBCORE_EXPORT Element* getElementById(const String&) const;
Comment on attachment 278370 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278370&action=review >>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4382 >>> + return ("AtomicString($value.toString(state)->toExistingAtomicString(state))", 1) if $signature->extendedAttributes->{"RequiresExistingAtomicString"}; >> >> The parentheses here after "return" are not normal perl coding style. I like it better without. >> >> It’s irritating that we have to construct AtomicString rather than just passing the result of toExistingAtomicString. Does it compile without the explicit AtomicString? > > 1. We use these parentheses in all return statements in this function. Do you want be to drop them all? > I am not Perl expert but I personally like the parentheses here as it makes it clear we're returning a tuple. > > 2. Yes, I have tried without the AtomicString() but it did not build. The compiler complains about it being ambiguous because there are 2 overloads in TreeScope.h: > Element* getElementById(const AtomicString&) const; > WEBCORE_EXPORT Element* getElementById(const String&) const; Oops, need the parentheses. Forgot this was a tuple! I really don’t get why toExistingAtomicString returns RefPtr<AtomicStringImpl> instead of AtomicString, but this code is all right with me.
Comment on attachment 278370 [details] Patch Clearing flags on attachment: 278370 Committed r200562: <http://trac.webkit.org/changeset/200562>
All reviewed patches have been landed. Closing bug.