Bug 163284 – Move Web SQL database and WebSockets off legacy exceptions

• Component: Bindings
• Status: RESOLVED FIXED

[Darin Adler]

Move Web SQL database and WebSockets off legacy exceptions

[Darin Adler]

Created attachment 291266 [details]
Patch

[Chris Dumez]

Looks like this break EFL and GTK.

[Chris Dumez]

Comment on attachment 291266 [details]
Patch

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

r=me but please fix the GTK/EFL build before landing.

> Source/WebCore/Modules/webdatabase/SQLResultSet.cpp:48
> +    return WTFMove(insertId);

Why do we WTFMove() an integer?

can't we just: "return *m_insertId;" ?

> Source/WebCore/Modules/webdatabase/SQLResultSet.idl:33
> +    [GetterMayThrowException] readonly attribute long insertId; // FIXME: Will not work for insertId greater than 2^31.

I don't understand this comment. This is a long and a Web IDL long is in the range [−2147483648, 2147483647]*. So it will obviously not work for values greater than 2^31, right?
The spec says to use long, not long long, so it seems the fact that we're using an int in the implementation is correct. Am I missing something?

* https://www.w3.org/TR/WebIDL/#idl-long

> Source/WebCore/Modules/websockets/WebSocket.cpp:184
> +    return create(context, url, { 1, protocol });

Personally feel like having an explicit Vector { } would be more easily understandable.

> Source/WebCore/Modules/websockets/WebSocket.cpp:194
> +    return connect(url, { 1, protocol });

ditto.

[Darin Adler]

Comment on attachment 291266 [details]
Patch

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

>> Source/WebCore/Modules/webdatabase/SQLResultSet.cpp:48
>> +    return WTFMove(insertId);
> 
> Why do we WTFMove() an integer?
> 
> can't we just: "return *m_insertId;" ?

I don’t think that works. I believe I tried it already; the constructor takes an int&&, not an int, and the result of *m_insertId is an int&, not an int&&.

We can return to this and try to fix the ExceptionOr template so that more normal syntax works, but doing that probably requires specializing ExceptionOr for scalars or something along those lines.

>> Source/WebCore/Modules/webdatabase/SQLResultSet.idl:33
>> +    [GetterMayThrowException] readonly attribute long insertId; // FIXME: Will not work for insertId greater than 2^31.
> 
> I don't understand this comment. This is a long and a Web IDL long is in the range [−2147483648, 2147483647]*. So it will obviously not work for values greater than 2^31, right?
> The spec says to use long, not long long, so it seems the fact that we're using an int in the implementation is correct. Am I missing something?
> 
> * https://www.w3.org/TR/WebIDL/#idl-long

Yes, the specification and the IDL file agree with each other. Also not sure how important the specification is given that this was not a widely-adopted standard; in this case our legacy behavior may be more important.

But our implementation uses int64_t; I don’t know what prevents the implementation from generating numbers outside of the range of an IDL long. I can write a different comment and put it somewhere different, but I think the issue does exist.

>> Source/WebCore/Modules/websockets/WebSocket.cpp:184
>> +    return create(context, url, { 1, protocol });
> 
> Personally feel like having an explicit Vector { } would be more easily understandable.

OK. I will go with that.

[Darin Adler]

I’ll have to figure out what the failure on EFL and GTK is. Something about the openDatabase function.

[Chris Dumez]

(In reply to comment #5)
> I’ll have to figure out what the failure on EFL and GTK is. Something about
> the openDatabase function.

Looks like the generated bindings still try to pass an ExceptionCode& ?

DerivedSources/WebCore/JSDOMWindow.cpp: In function 'JSC::EncodedJSValue WebCore::jsDOMWindowInstanceFunctionOpenDatabase(JSC::ExecState*)':
DerivedSources/WebCore/JSDOMWindow.cpp:29274:355: error: no matching function for call to 'WebCore::DOMWindowWebDatabase::openDatabase(WebCore::DOMWindow&, std::remove_reference<WTF::String&>::type, std::remove_reference<WTF::String&>::type, std::remove_reference<WTF::String&>::type, std::remove_reference<unsigned int&>::type, std::remove_reference<WTF::RefPtr<WebCore::DatabaseCallback>&>::type, WebCore::ExceptionCode&)'

Either we generate bad bindings or the bot needs a clean build.

[Chris Dumez]

(In reply to comment #6)
> (In reply to comment #5)
> > I’ll have to figure out what the failure on EFL and GTK is. Something about
> > the openDatabase function.
> 
> Looks like the generated bindings still try to pass an ExceptionCode& ?
> 
> DerivedSources/WebCore/JSDOMWindow.cpp: In function 'JSC::EncodedJSValue
> WebCore::jsDOMWindowInstanceFunctionOpenDatabase(JSC::ExecState*)':
> DerivedSources/WebCore/JSDOMWindow.cpp:29274:355: error: no matching
> function for call to
> 'WebCore::DOMWindowWebDatabase::openDatabase(WebCore::DOMWindow&,
> std::remove_reference<WTF::String&>::type,
> std::remove_reference<WTF::String&>::type,
> std::remove_reference<WTF::String&>::type, std::remove_reference<unsigned
> int&>::type,
> std::remove_reference<WTF::RefPtr<WebCore::DatabaseCallback>&>::type,
> WebCore::ExceptionCode&)'
> 
> Either we generate bad bindings or the bot needs a clean build.

I think it is likely dependency tracking is wrong for those ports and JSDOMWindow.cpp was not properly re-generated even though you modified:
Source/WebCore/Modules/webdatabase/DOMWindowWebDatabase.idl

[Darin Adler]

I’ll work around the dependency problem when landing this by touching DOMWindow.idl. And I think I will also file a bug report about the CMake build system not handling this right.

[Darin Adler]

Comment on attachment 291266 [details]
Patch

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

>>> Source/WebCore/Modules/webdatabase/SQLResultSet.idl:33
>>> +    [GetterMayThrowException] readonly attribute long insertId; // FIXME: Will not work for insertId greater than 2^31.
>> 
>> I don't understand this comment. This is a long and a Web IDL long is in the range [−2147483648, 2147483647]*. So it will obviously not work for values greater than 2^31, right?
>> The spec says to use long, not long long, so it seems the fact that we're using an int in the implementation is correct. Am I missing something?
>> 
>> * https://www.w3.org/TR/WebIDL/#idl-long
> 
> Yes, the specification and the IDL file agree with each other. Also not sure how important the specification is given that this was not a widely-adopted standard; in this case our legacy behavior may be more important.
> 
> But our implementation uses int64_t; I don’t know what prevents the implementation from generating numbers outside of the range of an IDL long. I can write a different comment and put it somewhere different, but I think the issue does exist.

I just realized that our bindings will indeed work with int64_t even though we specify long here. The fact that we specify long here doesn’t matter; the integer conversion is done based on overloading and the actual return type, the type specified here in the IDL file just tells it that it’s numeric. The conversion won’t work for values of 64-bit integers that can’t be represented as JavaScript number because they can’t be represented as a double, but that would be a problem no matter what this IDL file said.

[Darin Adler]

Committed r207384: <http://trac.webkit.org/changeset/207384>