Bug 166441 – [WebIDL] Remove custom bindings for WebSQL code

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

[WebIDL] Remove custom bindings for WebSQL code

[Sam Weinig]

Created attachment 297690 [details]
Patch

[Darin Adler]

Comment on attachment 297690 [details]
Patch

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

I probably shouldn’t review before EWS runs, but I did.

> Source/WebCore/Modules/webdatabase/SQLResultSetRowList.cpp:59
> +        auto name = m_columns[i];
> +        auto value = m_result[valuesIndex + i];
> +        
> +        result.set(WTFMove(name), WTFMove(value));

Why use set here instead of add? The only differences are that if multiple entries have the same name, with set, last wins, and with add, first wins. And also add makes *slightly* more efficient code.

Not sure this is easier to read with the local variables. I think this would be good:

    result.add(m_columns[i], m_result[valuesIndex + i]);

> Source/WebCore/Modules/webdatabase/SQLResultSetRowList.h:34
> +#include <wtf/RefCounted.h>

Not needed, because ExceptionOr includes this because Exception includes a String.

> Source/WebCore/Modules/webdatabase/SQLResultSetRowList.h:36
> +#include <wtf/text/WTFString.h>

Ditto.

> Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:87
> +    auto statement = std::make_unique<SQLStatement>(m_database, sqlStatement, arguments ? WTFMove(arguments.value()) : Vector<SQLValue> { }, WTFMove(callback), WTFMove(callbackError), permissions);

Can we write "WTFMove(arguments.value_or(Vector<SQLValue> { }))". Or does that not work to move? Maybe could even leave out the class name if doing it that way?

> Source/WebCore/inspector/InspectorDatabaseAgent.cpp:88
> +            RefPtr<InspectorValue> inspectorValue = WTF::switchOn(value,

Can we use auto here instead of writing out the type?

> Source/WebCore/platform/sql/SQLiteStatement.cpp:305
> -            return SQLValue();
> +            return SQLValue { nullptr };

Can we leave out the type name here and just use { nullptr }? If so, I think I’d like it slightly better, not sure if you agree.

> Source/WebCore/platform/sql/SQLiteStatement.cpp:307
> +        return SQLValue { nullptr };

Ditto.

> Source/WebCore/platform/sql/SQLiteStatement.cpp:315
> +            return SQLValue { sqlite3_value_double(value) };

Ditto.

> Source/WebCore/platform/sql/SQLiteStatement.cpp:319
> +            return SQLValue { StringImpl::create8BitIfPossible(string) };

Ditto.

> Source/WebCore/platform/sql/SQLiteStatement.cpp:322
> +            return SQLValue { nullptr };

Ditto.

> Source/WebCore/platform/sql/SQLiteStatement.cpp:328
> +    return SQLValue { nullptr };

Ditto.

[Sam Weinig]

Committed r210134: <http://trac.webkit.org/changeset/210134>