[bindings] Remove SQLTransaction's usage of custom binding.

[bindings] Remove SQLTransaction's usage of custom binding.

Making changes as per the spec, http://www.w3.org/TR/webdatabase/#sqltransaction to remove the custom binding usage.

R=jl@opera.com, haraken@chromium.org

BUG=345519
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191516

[vivekg, 2015-02-24 09:58:15]

vivekg@chromium.org changed reviewers:
+ vivekg@chromium.org

[vivekg, 2015-02-24 09:58:15]

PTAL as this is WIP patch. The generated code looks like this:
http://paste.ubuntu.com/10386410/

The question is: The 2nd arg is a nullable arg by the spec definition while the
generated code 

TONATIVE_VOID_EXCEPTIONSTATE_INTERNAL(arguments,
toImplArray<ScriptValue>(info[1], 2, info.GetIsolate(), exceptionState),
exceptionState);

doesn't support nullable sequence or array. 

How should we support this in a better way? Due to this there are 3 case
failures.

https://codereview.chromium.org/949193002/diff/1/Source/modules/webdatabase/S...
File Source/modules/webdatabase/SQLTransaction.idl (right):

https://codereview.chromium.org/949193002/diff/1/Source/modules/webdatabase/S...
Source/modules/webdatabase/SQLTransaction.idl:36: optional ObjectArray?
arguments,
How can we specify ObjectArray can be a null. Right now there is no way in which
this can be specified either:

optional ObjectArray? arguments = null 

or 

optional object[]? arguments = null

Which is the correct way to support this? Or is this a valid construct at all?

[haraken, 2015-02-24 10:13:08]

On 2015/02/24 09:58:15, vivekg_ wrote:
> PTAL as this is WIP patch. The generated code looks like this:
> http://paste.ubuntu.com/10386410/
> 
> The question is: The 2nd arg is a nullable arg by the spec definition while
the
> generated code 
> 
> TONATIVE_VOID_EXCEPTIONSTATE_INTERNAL(arguments,
> toImplArray<ScriptValue>(info[1], 2, info.GetIsolate(), exceptionState),
> exceptionState);
> 
> doesn't support nullable sequence or array. 
> 
> How should we support this in a better way? Due to this there are 3 case
> failures.
> 
>
https://codereview.chromium.org/949193002/diff/1/Source/modules/webdatabase/S...
> File Source/modules/webdatabase/SQLTransaction.idl (right):
> 
>
https://codereview.chromium.org/949193002/diff/1/Source/modules/webdatabase/S...
> Source/modules/webdatabase/SQLTransaction.idl:36: optional ObjectArray?
> arguments,
> How can we specify ObjectArray can be a null. Right now there is no way in
which
> this can be specified either:
> 
> optional ObjectArray? arguments = null 
> 
> or 
> 
> optional object[]? arguments = null
> 
> Which is the correct way to support this? Or is this a valid construct at all?

Jens?

[Jens Widell, 2015-02-24 10:21:04]

On 2015/02/24 10:13:08, haraken wrote:
> On 2015/02/24 09:58:15, vivekg_ wrote:
> > PTAL as this is WIP patch. The generated code looks like this:
> > http://paste.ubuntu.com/10386410/
> > 
> > The question is: The 2nd arg is a nullable arg by the spec definition while
> the
> > generated code 
> > 
> > TONATIVE_VOID_EXCEPTIONSTATE_INTERNAL(arguments,
> > toImplArray<ScriptValue>(info[1], 2, info.GetIsolate(), exceptionState),
> > exceptionState);
> > 
> > doesn't support nullable sequence or array. 
> > 
> > How should we support this in a better way? Due to this there are 3 case
> > failures.
> > 
> >
>
https://codereview.chromium.org/949193002/diff/1/Source/modules/webdatabase/S...
> > File Source/modules/webdatabase/SQLTransaction.idl (right):
> > 
> >
>
https://codereview.chromium.org/949193002/diff/1/Source/modules/webdatabase/S...
> > Source/modules/webdatabase/SQLTransaction.idl:36: optional ObjectArray?
> > arguments,
> > How can we specify ObjectArray can be a null. Right now there is no way in
> which
> > this can be specified either:
> > 
> > optional ObjectArray? arguments = null 
> > 
> > or 
> > 
> > optional object[]? arguments = null
> > 
> > Which is the correct way to support this? Or is this a valid construct at
all?
> 
> Jens?

It looks like the code generator doesn't support explicitly nullable method
arguments, or attribute setter for explicitly nullable attributes. It only
supports explicitly nullable types for method and attribute getter return
values.

Quick terminology:

"Implicit nullable": types where Blink's C++ representation has a native null
value, e.g. where the C++ representation is a (smart) pointer or WTF::String
(see WTF::String::isNull()).

"Explicit nullable": other types, e.g. int and double, which we then represent
on the C++ side as Nullable<int> or Nullable<double>.

Since Vector<> does not have a native null value, it falls into the second
category, and so the C++ function argument resulting from the second argument to
executeSql() ought to be Nullable<Vector<ScriptValue>>. But the code generator
lacks the logic to make it so, it seems.

[vivekg, 2015-02-24 10:24:17]

On 2015/02/24 at 10:21:04, jl wrote:

> It looks like the code generator doesn't support explicitly nullable method
arguments, or attribute setter for explicitly nullable attributes. It only
supports explicitly nullable types for method and attribute getter return
values.
> 
> Quick terminology:
> 
> "Implicit nullable": types where Blink's C++ representation has a native null
value, e.g. where the C++ representation is a (smart) pointer or WTF::String
(see WTF::String::isNull()).
> 
> "Explicit nullable": other types, e.g. int and double, which we then represent
on the C++ side as Nullable<int> or Nullable<double>.
> 
> Since Vector<> does not have a native null value, it falls into the second
category, and so the C++ function argument resulting from the second argument to
executeSql() ought to be Nullable<Vector<ScriptValue>>. But the code generator
lacks the logic to make it so, it seems.

Oh that's really good information, thanks Jens. Should I give it a try to fix
the code generator to handle this?

[Jens Widell, 2015-02-24 10:29:28]

On 2015/02/24 10:24:17, vivekg_ wrote:
> On 2015/02/24 at 10:21:04, jl wrote:
> 
> > It looks like the code generator doesn't support explicitly nullable method
> arguments, or attribute setter for explicitly nullable attributes. It only
> supports explicitly nullable types for method and attribute getter return
> values.
> > 
> > Quick terminology:
> > 
> > "Implicit nullable": types where Blink's C++ representation has a native
null
> value, e.g. where the C++ representation is a (smart) pointer or WTF::String
> (see WTF::String::isNull()).
> > 
> > "Explicit nullable": other types, e.g. int and double, which we then
represent
> on the C++ side as Nullable<int> or Nullable<double>.
> > 
> > Since Vector<> does not have a native null value, it falls into the second
> category, and so the C++ function argument resulting from the second argument
to
> executeSql() ought to be Nullable<Vector<ScriptValue>>. But the code generator
> lacks the logic to make it so, it seems.
> 
> Oh that's really good information, thanks Jens. Should I give it a try to fix
> the code generator to handle this?

Please do. I think argument_context() (v8_methods.py) just needs to set
'is_explicitly_nullable' in the returned context, and then it's a quick fix in
templates/methods.cpp to condition the conversion of the argument value with "if
(!info[N].isNull())". (Nullable<> objects are null by default and are declared
separately, so we just need to avoid setting it to some other value.)

[jsbell, 2015-02-24 19:13:16]

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2015-02-24 19:13:16]

Please leave a comment in the IDL that the spec has just "optional ObjectArray
arguments" but that spec prose defines behavior when null is passed.

(Just in case Someone™ decides to try and align blink's IDL with the antique IDL
in the spec.)

[vivekg, 2015-03-03 08:15:20]

Jens, jsbell@: Can you please take a look?

[jsbell, 2015-03-03 18:33:14]

lgtm with a suggestion...

https://codereview.chromium.org/949193002/diff/80001/Source/modules/webdataba...
File Source/modules/webdatabase/SQLTransaction.cpp (right):

https://codereview.chromium.org/949193002/diff/80001/Source/modules/webdataba...
Source/modules/webdatabase/SQLTransaction.cpp:294: v8::Local<v8::Value> arg =
args[i].v8Value();
Can we factor this out into toSQLValue(const ScriptValue&) ?

Also, per discussion in https://codereview.chromium.org/972153002/ I'd be in
favor of putting that in a helper library in bindings/v8/modules.

[haraken, 2015-03-04 02:00:07]

https://codereview.chromium.org/949193002/diff/80001/Source/modules/webdataba...
File Source/modules/webdatabase/SQLTransaction.cpp (right):

https://codereview.chromium.org/949193002/diff/80001/Source/modules/webdataba...
Source/modules/webdatabase/SQLTransaction.cpp:294: v8::Local<v8::Value> arg =
args[i].v8Value();
On 2015/03/03 18:33:14, jsbell wrote:
> Can we factor this out into toSQLValue(const ScriptValue&) ?
> 
> Also, per discussion in https://codereview.chromium.org/972153002/ I'd be in
> favor of putting that in a helper library in bindings/v8/modules.

Sounds nice to put the function into IDBBindingUtilities.cpp.

[bashi, 2015-03-04 03:33:01]

bashi@chromium.org changed reviewers:
+ bashi@chromium.org

[bashi, 2015-03-04 03:33:02]

https://codereview.chromium.org/949193002/diff/80001/Source/modules/webdataba...
File Source/modules/webdatabase/SQLTransaction.cpp (right):

https://codereview.chromium.org/949193002/diff/80001/Source/modules/webdataba...
Source/modules/webdatabase/SQLTransaction.cpp:294: v8::Local<v8::Value> arg =
args[i].v8Value();
On 2015/03/04 02:00:07, haraken wrote:
> On 2015/03/03 18:33:14, jsbell wrote:
> > Can we factor this out into toSQLValue(const ScriptValue&) ?
> > 
> > Also, per discussion in https://codereview.chromium.org/972153002/ I'd be in
> > favor of putting that in a helper library in bindings/v8/modules.
> 
> Sounds nice to put the function into IDBBindingUtilities.cpp.

Is is possible to use toImplArray()? (You need to write
NativeValueTraits<SQLValue>)

https://codereview.chromium.org/949193002/diff/80001/Source/modules/webdataba...
Source/modules/webdatabase/SQLTransaction.cpp:298:
sqlArguments.append(SQLValue(arg->NumberValue()));
FYI, NumberValue() is going to be deprecated.
NumberValue(v8::Local<v8::Context>) is the new API, and I'm working on adding
macros which do empty checks.

[vivekg, 2015-03-05 13:18:10]

Done all the changes, PTAL. Thanks.

[Jens Widell, 2015-03-05 13:54:38]

https://codereview.chromium.org/949193002/diff/100001/Source/bindings/modules...
File Source/bindings/modules/v8/V8BindingForModules.cpp (right):

https://codereview.chromium.org/949193002/diff/100001/Source/bindings/modules...
Source/bindings/modules/v8/V8BindingForModules.cpp:13: Vector<SQLValue>
toSQLValueArray(const Nullable<Vector<ScriptValue>>& values)
I think toSQLValueArray(const Vector<ScriptValue>>&) makes more sense, leaving
the Nullable<>/isNull() stuff in the caller. The caller can trivially construct
an empty Vector<SQLValue> for the null case, after all.

Actually, it might make even more sense to have toSQLValue(const ScriptValue&),
and do the looping in the caller.

https://codereview.chromium.org/949193002/diff/100001/Source/bindings/modules...
Source/bindings/modules/v8/V8BindingForModules.cpp:27:
TOSTRING_DEFAULT(V8StringResource<>, stringValue, arg, Vector<SQLValue>());
Exception handling is wrong here. This function should take an "ExceptionState&
exceptionState" argument, and then the code here should be

  V8StringResource<> stringValue(arg);
  if (!stringValue.prepare(exceptionState))
    return Vector<SQLValue>();

[bashi, 2015-03-06 00:52:59]

https://codereview.chromium.org/949193002/diff/100001/Source/bindings/modules...
File Source/bindings/modules/v8/V8BindingForModules.cpp (right):

https://codereview.chromium.org/949193002/diff/100001/Source/bindings/modules...
Source/bindings/modules/v8/V8BindingForModules.cpp:13: Vector<SQLValue>
toSQLValueArray(const Nullable<Vector<ScriptValue>>& values)
On 2015/03/05 13:54:37, Jens Widell wrote:
> I think toSQLValueArray(const Vector<ScriptValue>>&) makes more sense, leaving
> the Nullable<>/isNull() stuff in the caller. The caller can trivially
construct
> an empty Vector<SQLValue> for the null case, after all.
> 
> Actually, it might make even more sense to have toSQLValue(const
ScriptValue&),
> and do the looping in the caller.

We might want to have more generic one like following (omitting error handling):

template <typename T>
Vector<T> toImplArray(v8::Isolate* isolate, Vector<ScriptValue> v,
ExceptionState es) {
  Vector<T> ret;
  for (unsigned i = 0; i < v.size(); ++i) {
    ret.uncheckedAppend(NativeValueTraits<T>::nativeValue(v[i].v8Value(),
isolate, es));
  }
  return ret;
}

template<>
struct NativeValueTraits<SQLValue> {
  static SQLValue nativeValue(const v8::Local<v8::Value>& value, v8::Isolate*
isolate, ExceptionState& es) {
    if (value.IsEmpty() || value->IsNull())
      return SQLValue();
    if (value->IsNumber())
      return value->NumberValue(); // This is going to be deprecated BTW
    V8StringResource<> stringValue(value);
    if (!stringValue.prepare(es))
      return SQLValue();
    return stringValue;
  }
};

It toImplArray() should be in V8Binding.h

[vivekg, 2015-03-06 14:15:13]

PTAL, thanks.

https://codereview.chromium.org/949193002/diff/120001/Source/modules/webdatab...
File Source/modules/webdatabase/InspectorDatabaseAgent.cpp (right):

https://codereview.chromium.org/949193002/diff/120001/Source/modules/webdatab...
Source/modules/webdatabase/InspectorDatabaseAgent.cpp:165:
transaction->executeSql(ScriptState::current(v8::Isolate::GetCurrent()),
m_sqlStatement, Vector<ScriptValue>(), callback, errorCallback,
IGNORE_EXCEPTION);
Is there a way to avoid using the v8::Isolate here? I am not quite sure about it
hence parked it here. WDYT?

[vivekg, 2015-03-06 14:19:44]

https://codereview.chromium.org/949193002/diff/120001/Source/modules/webdatab...
File Source/modules/webdatabase/InspectorDatabaseAgent.cpp (right):

https://codereview.chromium.org/949193002/diff/120001/Source/modules/webdatab...
Source/modules/webdatabase/InspectorDatabaseAgent.cpp:165:
transaction->executeSql(ScriptState::current(v8::Isolate::GetCurrent()),
m_sqlStatement, Vector<ScriptValue>(), callback, errorCallback,
IGNORE_EXCEPTION);
On 2015/03/06 at 14:15:13, vivekg_ wrote:
> Is there a way to avoid using the v8::Isolate here? I am not quite sure about
it hence parked it here. WDYT?

Also I could see its usage at these many places here [1]. So is it ok to have it
here as is?

[1]
https://code.google.com/p/chromium/codesearch#search/&q=v8::Isolate::GetCurre...

[Jens Widell, 2015-03-06 14:26:03]

https://codereview.chromium.org/949193002/diff/120001/Source/modules/webdatab...
File Source/modules/webdatabase/InspectorDatabaseAgent.cpp (right):

https://codereview.chromium.org/949193002/diff/120001/Source/modules/webdatab...
Source/modules/webdatabase/InspectorDatabaseAgent.cpp:165:
transaction->executeSql(ScriptState::current(v8::Isolate::GetCurrent()),
m_sqlStatement, Vector<ScriptValue>(), callback, errorCallback,
IGNORE_EXCEPTION);
On 2015/03/06 14:19:44, vivekg_ wrote:
> On 2015/03/06 at 14:15:13, vivekg_ wrote:
> > Is there a way to avoid using the v8::Isolate here? I am not quite sure
about
> it hence parked it here. WDYT?
> 
> Also I could see its usage at these many places here [1]. So is it ok to have
it
> here as is?
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#search/&q=v8::Isolate::GetCurre...

One way to avoid it would be to keep the old executeSQL() function (with
unchanged signature), and use that from here and from the new executeSql()
method.

[vivekg, 2015-03-06 14:30:38]

On 2015/03/06 at 14:26:03, jl wrote:

> One way to avoid it would be to keep the old executeSQL() function (with
unchanged signature), and use that from here and from the new executeSql()
method.

Sure that's definitely doable although the onus would just shift onto
SQLTransaction.cpp for using v8::Isolate::GetCurrent.
Also wouldn't it make the code little complex by having multiple signatures of
executeSql. (This CL is already adding one) Just my thoughts. WDYT?

[vivekg, 2015-03-06 14:32:24]

On 2015/03/06 at 14:30:38, vivekg_ wrote:
> On 2015/03/06 at 14:26:03, jl wrote:
> 
> > One way to avoid it would be to keep the old executeSQL() function (with
unchanged signature), and use that from here and from the new executeSql()
method.
> 
> Sure that's definitely doable although the onus would just shift onto
SQLTransaction.cpp for using v8::Isolate::GetCurrent.
> Also wouldn't it make the code little complex by having multiple signatures of
executeSql. (This CL is already adding one) Just my thoughts. WDYT?

And just for the record, we are not even using the passed isolate anywhere in
V8BindingForModules.h in NativeValueTraits<SQLValue>.

[Jens Widell, 2015-03-06 14:35:53]

On 2015/03/06 14:30:38, vivekg_ wrote:
> On 2015/03/06 at 14:26:03, jl wrote:
> 
> > One way to avoid it would be to keep the old executeSQL() function (with
> unchanged signature), and use that from here and from the new executeSql()
> method.
> 
> Sure that's definitely doable although the onus would just shift onto
> SQLTransaction.cpp for using v8::Isolate::GetCurrent.

The "new" executeSql() already gets a ScriptState* argument, and the "old"
executeSQL() doesn't need a ScriptState, since all the ScriptState is used for
is the conversion from Vector<ScriptValue> to Vector<SQLValue> which is in the
new executeSql().

> Also wouldn't it make the code little complex by having multiple signatures of
> executeSql. (This CL is already adding one) Just my thoughts. WDYT?

There's nothing particularly unusual I think about having one set of functions
called by generated bindings code, with a (sometimes somewhat cumbersome) API
dictated by that, and other functions that perform the inner tasks, with a more
to-the-point API.

But sure, it does weigh down the API somewhat.

[vivekg, 2015-03-06 15:08:36]

On 2015/03/06 at 14:35:53, jl wrote:
> On 2015/03/06 14:30:38, vivekg_ wrote:
> > On 2015/03/06 at 14:26:03, jl wrote:
> > 
> > > One way to avoid it would be to keep the old executeSQL() function (with
> > unchanged signature), and use that from here and from the new executeSql()
> > method.
> > 
> > Sure that's definitely doable although the onus would just shift onto
> > SQLTransaction.cpp for using v8::Isolate::GetCurrent.
> 
> The "new" executeSql() already gets a ScriptState* argument, and the "old"
executeSQL() doesn't need a ScriptState, since all the ScriptState is used for
is the conversion from Vector<ScriptValue> to Vector<SQLValue> which is in the
new executeSql().
> 

Done. PTAL :)

[bashi, 2015-03-06 15:53:15]

https://codereview.chromium.org/949193002/diff/140001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/949193002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:739:
result.uncheckedAppend(NativeValueTraits<T>::nativeValue(value[i].v8Value(),
isolate, exceptionState));
You need to check whether the exceptionState had an exception like L727.

for (unsigned i = 0; i < value.size(); ++i) {
    result.uncheckedAppend(NativeValueTraits<T>::nativeValue(value[i].v8Value(),
isolate, exceptionState));
    if (exceptionState.hadException())
        return Vector<T>();
}

Similarly, you might need a v8::TryCatch, but I'm not sure it's actually needed.

[vivekg, 2015-03-06 17:32:04]

On 2015/03/06 at 15:53:15, bashi wrote:
>
https://codereview.chromium.org/949193002/diff/140001/Source/bindings/core/v8...
> File Source/bindings/core/v8/V8Binding.h (right):
> 
>
https://codereview.chromium.org/949193002/diff/140001/Source/bindings/core/v8...
> Source/bindings/core/v8/V8Binding.h:739:
result.uncheckedAppend(NativeValueTraits<T>::nativeValue(value[i].v8Value(),
isolate, exceptionState));
> You need to check whether the exceptionState had an exception like L727.
> 
> for (unsigned i = 0; i < value.size(); ++i) {
>    
result.uncheckedAppend(NativeValueTraits<T>::nativeValue(value[i].v8Value(),
isolate, exceptionState));
>     if (exceptionState.hadException())
>         return Vector<T>();
> }
> 

Oops missed that. Added it now. (Friday fever may be hehe :) )

> Similarly, you might need a v8::TryCatch, but I'm not sure it's actually
needed.

Yes its not needed here.

PTAL, thanks.

[vivekg, 2015-03-08 09:50:46]

jl@, bashi@: Is this good to go?

[bashi, 2015-03-09 01:38:48]

LGTM, but please wait for Jens' approval.

[haraken, 2015-03-09 03:50:37]

LGTM

[vivekg, 2015-03-09 03:56:43]

The CQ bit was checked by vivekg@chromium.org

[vivekg, 2015-03-09 03:56:43]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/949193002/#ps160001
(title: "Fixed exception")

[commit-bot: I haz the power, 2015-03-09 03:57:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/949193002/160001

[commit-bot: I haz the power, 2015-03-09 05:30:32]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191516

[Jens Widell, 2015-03-09 06:06:28]

LGTM