Bug 157459 – Change EventSource constructor to take an IDL dictionary instead of a WebCore::Dictionary

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

[Darin Adler]

Change EventSource constructor to take an IDL dictionary instead of a WebCore::Dictionary

[Darin Adler]

Created attachment 278355 [details]
Patch

[Chris Dumez]

Comment on attachment 278355 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278355&action=review

> Source/WebCore/page/EventSource.cpp:60
> +inline EventSource::EventSource(ScriptExecutionContext& context, const URL& url, const Optional<Init>& eventSourceInit)

This is probably my bug but I don't think we should use Optional<> for dictionaries. Web IDL says:
"""
If the type of an argument is a dictionary type or a union type that has a dictionary type as one of its flattened member types, and that dictionary type and its ancestors have no required members, and the argument is either the final argument or is followed only by optional arguments, then the argument must be specified as optional. Such arguments are always considered to have a default value of an empty dictionary, unless otherwise specified.
"""

[Darin Adler]

Sounds like that does cover this case. So we should say optional in the IDL but the code should not generate an Optional<>. Should be really quick to fix.

[Darin Adler]

Comment on attachment 278355 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278355&action=review

> Source/WebCore/bindings/js/JSDOMConvert.h:54
> +template<> inline bool convert<bool>(JSC::ExecState& state, JSC::JSValue value)
> +{
> +    return value.toBoolean(&state);
> +}

I will rebase because this function was added to JSDOMConvert.h by another of my patches last night.

>> Source/WebCore/page/EventSource.cpp:60
>> +inline EventSource::EventSource(ScriptExecutionContext& context, const URL& url, const Optional<Init>& eventSourceInit)
> 
> This is probably my bug but I don't think we should use Optional<> for dictionaries. Web IDL says:
> """
> If the type of an argument is a dictionary type or a union type that has a dictionary type as one of its flattened member types, and that dictionary type and its ancestors have no required members, and the argument is either the final argument or is followed only by optional arguments, then the argument must be specified as optional. Such arguments are always considered to have a default value of an empty dictionary, unless otherwise specified.
> """

I will change this to not use Optional after landing a patch I am working on that fixes the handling of undefined and null and obviates the need for Optional.

[Darin Adler]

Created attachment 278363 [details]
Patch

[Darin Adler]

Patch is doing well on EWS. Ready to get reviewed and land!

[Chris Dumez]

Comment on attachment 278363 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278363&action=review

r=me

> Source/WebCore/ChangeLog:67
> +        * page/EventSource.idl: U

U ?

> Source/WTF/wtf/text/WTFString.h:59
> +WTF_EXPORT_STRING_API uint64_t charactersToUInt64(const UChar*, size_t, bool* ok = 0); // ignores trailing garbage

= nullptr

> Source/WebCore/page/EventSource.cpp:289
> +                position++;

++position

> Source/WebCore/page/EventSource.cpp:295
> +        for (unsigned i = position; !lineLength && i < size; i++) {

++i

> Source/WebCore/page/EventSource.cpp:401
> +    auto data = SerializedScriptValue::create(StringView { m_data.data(), size });

Do we need to explicitly say StringView? Wouldn't "{ m_data.data(), size }" work?

[Darin Adler]

Committed r200558: <http://trac.webkit.org/changeset/200558>