WebSQL: Stop using ToImplArray().

WebSQL: Stop using ToImplArray().

As we work towards removing ToImplArray() from V8Binding.h, stop using it in
SQLTransaction::executeSql() by just iterating through the array and
converting the data via ScriptValue::To().

BUG=685754
R=michaeln@chromium.org,bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org

[Raphael Kubo da Costa (rakuco), 2017-04-18 10:36:57]

PTAL. This was spun off https://codereview.chromium.org/2810843002/

[Raphael Kubo da Costa (rakuco), 2017-04-18 10:37:03]

The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 10:37:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2017-04-18 10:48:32]

LGTM

https://codereview.chromium.org/2825033003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webdatabase/SQLTransaction.cpp (right):

https://codereview.chromium.org/2825033003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webdatabase/SQLTransaction.cpp:324:
script_state->GetIsolate(), script_value, exception_state));

I'm fine with this but is there any way to use
NativeValueTrait<SQLValue>::toNativeValue? I think we're aiming at replacing all
JS => C++ conversions with toNativeValue.

[Yuki, 2017-04-18 11:02:23]

Let me take a closer look later.

https://codereview.chromium.org/2825033003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webdatabase/SQLTransaction.cpp (right):

https://codereview.chromium.org/2825033003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webdatabase/SQLTransaction.cpp:324:
script_state->GetIsolate(), script_value, exception_state));
On 2017/04/18 10:48:32, haraken wrote:
> 
> I'm fine with this but is there any way to use
> NativeValueTrait<SQLValue>::toNativeValue? I think we're aiming at replacing
all
> JS => C++ conversions with toNativeValue.

The problem is that toNativeValue is designed to convert from a v8::Value to a
C++ object.  This case, the input is not a v8::Value.

[Raphael Kubo da Costa (rakuco), 2017-04-18 11:05:04]

On 2017/04/18 11:02:23, Yuki wrote:
> Let me take a closer look later.
> 
>
https://codereview.chromium.org/2825033003/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/webdatabase/SQLTransaction.cpp (right):
> 
>
https://codereview.chromium.org/2825033003/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/webdatabase/SQLTransaction.cpp:324:
> script_state->GetIsolate(), script_value, exception_state));
> On 2017/04/18 10:48:32, haraken wrote:
> > 
> > I'm fine with this but is there any way to use
> > NativeValueTrait<SQLValue>::toNativeValue? I think we're aiming at replacing
> all
> > JS => C++ conversions with toNativeValue.
> 
> The problem is that toNativeValue is designed to convert from a v8::Value to a
> C++ object.  This case, the input is not a v8::Value.

Exactly. The relevant IDL here specifies a sequence<any>, which becomes a
Vector<ScriptValue> that is passed to this method, which we then want to
transform into a Vector<SQLValue>. In the normal case, we'd be passed a
v8::Local<v8::Value> that needs to be converted into a Vector<SQLValue>. We can
either make our IDL stop being compliant with the spec or make NativeValueTraits
work with ScriptValue more seamlessly.

[haraken, 2017-04-18 11:06:40]

On 2017/04/18 11:05:04, Raphael Kubo da Costa (rakuco) wrote:
> On 2017/04/18 11:02:23, Yuki wrote:
> > Let me take a closer look later.
> > 
> >
>
https://codereview.chromium.org/2825033003/diff/1/third_party/WebKit/Source/m...
> > File third_party/WebKit/Source/modules/webdatabase/SQLTransaction.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/2825033003/diff/1/third_party/WebKit/Source/m...
> > third_party/WebKit/Source/modules/webdatabase/SQLTransaction.cpp:324:
> > script_state->GetIsolate(), script_value, exception_state));
> > On 2017/04/18 10:48:32, haraken wrote:
> > > 
> > > I'm fine with this but is there any way to use
> > > NativeValueTrait<SQLValue>::toNativeValue? I think we're aiming at
replacing
> > all
> > > JS => C++ conversions with toNativeValue.
> > 
> > The problem is that toNativeValue is designed to convert from a v8::Value to
a
> > C++ object.  This case, the input is not a v8::Value.
> 
> Exactly. The relevant IDL here specifies a sequence<any>, which becomes a
> Vector<ScriptValue> that is passed to this method, which we then want to
> transform into a Vector<SQLValue>. In the normal case, we'd be passed a
> v8::Local<v8::Value> that needs to be converted into a Vector<SQLValue>. We
can
> either make our IDL stop being compliant with the spec or make
NativeValueTraits
> work with ScriptValue more seamlessly.

Would it be hard to make NativeValueTraits work with ScriptValues?

Either way this CL LGTM.

[Raphael Kubo da Costa (rakuco), 2017-04-18 11:30:22]

On 2017/04/18 11:06:40, haraken wrote:
> Would it be hard to make NativeValueTraits work with ScriptValues?

I need to think of a clean solution here. Not only would we need to be able to
convert ScriptValues to other types but for this specific case we need to be
able to go through a Vector of ScriptValues, and I wanted to avoid the special
cases we currently have in V8Binding.h (there's a ToImplArray() overload
specifically for Vector<ScriptValue> at the moment).

[commit-bot: I haz the power, 2017-04-18 12:24:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-18 12:24:09]

Dry run: This issue passed the CQ dry run.

[Yuki, 2017-04-18 13:57:24]

Hmm, now I think it would be better to keep ToImplArray.  Why is it better to
remove?

TL;DR: What about just renaming ToImplArray to a better name, and continue using
it?

I see that SQLTransaction::executeSql is the only client of ToImplArray, but
potentially there could be more clients in future.  Using ToImplArray would be
safer / more robust than implementing it on each client side.  I'm afraid that
clients wouldn't handle error cases appropriately.

My current understanding is that:

* NativeValueTraits::NativeValue is responsible to convert a v8::Value to a
value of an arbitrary implementation type T.

* ScriptValue::To is just an alias of NativeValueTraits::NativeValue.  I don't
understand why we need this function now.  Not sure, but we could want to remove
this.

* ScriptValue is designed to hold a raw v8::Value.  So, I think it makes sense
to treat a ScriptValue as if it's a v8::Value, i.e. Having a following overload.
  template <typename T>
  T NativeValue(isolate, scriptValue, exceptionState) {
    return NativeValue(isolate, scriptValue.V8Value(), exceptionState)
  }

* SQLTransaction::executeSql needs a kind of "map" higher-order function.
  Vector<ScriptValue> input;
  Vector<SQLValue> output;
  output = map(NativeValueTraits::NativeValue, input);
ToImplArray is a kind of non-general "map" function.

* Probably, it's not a bad idea to provide this kind of "map" function.

Thus, I'm now thinking it would be good to rename ToImplArray to "a map
higher-order function from Vector<v8:Value or ScriptValue> to Vector<T>", and
continue using it.

What do you think?

https://codereview.chromium.org/2825033003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webdatabase/SQLTransaction.cpp (right):

https://codereview.chromium.org/2825033003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webdatabase/SQLTransaction.cpp:324:
script_state->GetIsolate(), script_value, exception_state));
We need to handle a possible exception here.

The original code seems wrong, too, because the original code also ignored a
possible exception.

The code in this CL could be worse because it is completely ignoring a possible
exception.  The original code at least used an empty vector in case of error.

[michaeln, 2017-04-19 01:03:45]

So long as its functionally equivalent, lgtm, but i think you need to test
HadException() while looping to do that.

Since I don't know squat about the bindings, take this with a grain of salt.
Given how non-obvious and error prone these conversions are, having it codified
in a helper seems like a good thing to me.

[Raphael Kubo da Costa (rakuco), 2017-04-19 15:26:05]

On 2017/04/18 13:57:24, Yuki wrote:
> TL;DR: What about just renaming ToImplArray to a better name, and continue
using
> it?
[...]
> What do you think?

I agree :-)

Thanks for spending some time on this. I've filed
https://bugs.chromium.org/p/chromium/issues/detail?id=713200 to track everything
that should be done, and I'll drop this CL for now.