Bug 157465 – [Bindings] Simplify [RequiresExistingAtomicString] IDL extended attribute handling

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

Simplify [RequiresExistingAtomicString] IDL extended attribute handling in the bindings generator.

[Chris Dumez]

Created attachment 278370 [details]
Patch

[Darin Adler]

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?

[Chris Dumez]

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;

[Darin Adler]

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.

[Chris Dumez]

Comment on attachment 278370 [details]
Patch

Clearing flags on attachment: 278370

Committed r200562: <http://trac.webkit.org/changeset/200562>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.