Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
972153002:
[bindings] Remove custom binding usage in SQLResultSetRowList. (Closed)
|
Created:
5 years, 9 months ago by vivekg_samsung Modified:
4 years, 7 months ago CC:
arv+blink, blink-reviews, blink-reviews-bindings_chromium.org, vivekg_samsung, Yuki Base URL:
https://chromium.googlesource.com/chromium/blink.git@test Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
Description[bindings] Remove custom binding usage in SQLResultSetRowList.
As per [1], removing the usage of custom binding required for SQLResultSetRowList.
R=jl@opera.com, haraken@chromium.org, jsbell@chromium.org
BUG=345519
[1] http://www.w3.org/TR/webdatabase/#sqlresultsetrowlist
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191359
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addition of new file ToV8.h for modules #
Total comments: 13
Patch Set 3 : Review comments addressed! #
Total comments: 3
Patch Set 4 : CallWith=ScriptState #
Total comments: 12
Patch Set 5 : Patch for landing #Messages
Total messages: 44 (7 generated)
vivekg@chromium.org changed reviewers: + vivekg@chromium.org
PTAL, thanks.
vivekg@chromium.org changed reviewers: + jsbell@chromium.org
Unfortunately I might be negative to this change :-/ https://codereview.chromium.org/972153002/diff/1/Source/modules/webdatabase/S... File Source/modules/webdatabase/SQLResultSetRowList.cpp (right): https://codereview.chromium.org/972153002/diff/1/Source/modules/webdatabase/S... Source/modules/webdatabase/SQLResultSetRowList.cpp:46: ScriptValue SQLResultSetRowList::item(unsigned index, ExceptionState&) This function contains a bunch of V8 APIs. In terms of DEPS, it is OK to use V8 APIs in core/ and modules/, but in terms of code health, we want to limit usages of V8 APIs into bindings/ because it's hard for ordinary developers to use V8 APIs correctly.
The pattern we use for Indexed DB is to have Source/bindings/modules/v8/IDBBindingUtilities.h/cpp that encapsulates the module-specific bindings goop like Blink<->V8 type conversions, letting the code in modules simply pass in e.g. a WTF structure and get back a ScriptValue. That eliminates the need for fragile custom bindings, but keeps the potentially tricky v8::Value conversion code under the eye of bindings owners.
On 2015/03/03 17:29:24, jsbell wrote: > The pattern we use for Indexed DB is to have > Source/bindings/modules/v8/IDBBindingUtilities.h/cpp that encapsulates the > module-specific bindings goop like Blink<->V8 type conversions, letting the code > in modules simply pass in e.g. a WTF structure and get back a ScriptValue. > > That eliminates the need for fragile custom bindings, but keeps the potentially > tricky v8::Value conversion code under the eye of bindings owners. Sounds nice to put the fuction in IDBBindingUtilities!
On 2015/03/04 at 01:51:21, haraken wrote: > Sounds nice to put the fuction in IDBBindingUtilities! Sure. On a side note, should we rename the file, "bindings/modules/v8/IDBBindingUtilities.h" to just "bindings/modules/v8/BindingUtilities.h" as now we are accessing this file even from the webdatabase module? WDYT?
haraken@chromium.org changed reviewers: + bashi@chromium.org
> > Sounds nice to put the fuction in IDBBindingUtilities! > > Sure. On a side note, should we rename the file, > "bindings/modules/v8/IDBBindingUtilities.h" to just > "bindings/modules/v8/BindingUtilities.h" as now we are accessing this file even > from the webdatabase module? WDYT? Ah, understand the situation. I'm not super happy about introducing BindingUtilities, because it will end up with a random collection of utility functions (we previously had V8Utilities.h, which I removed a long time ago). That said, I agree that we don't want to put webdatabase functions into IDBBindingUtilities. bashi-san: Do you have any suggestion? Can we use toImplArray here somehow as well? https://codereview.chromium.org/972153002/diff/1/Source/modules/webdatabase/S... File Source/modules/webdatabase/SQLResultSetRowList.cpp (right): https://codereview.chromium.org/972153002/diff/1/Source/modules/webdatabase/S... Source/modules/webdatabase/SQLResultSetRowList.cpp:69: item->ForceSet(v8String(isolate, m_columns[i]), value, static_cast<v8::PropertyAttribute>(v8::DontDelete | v8::ReadOnly)); I'm wondering why this is using ForceSet. I think this can be a normal Set.
On 2015/03/04 04:06:35, haraken wrote: > > > Sounds nice to put the fuction in IDBBindingUtilities! > > > > Sure. On a side note, should we rename the file, > > "bindings/modules/v8/IDBBindingUtilities.h" to just > > "bindings/modules/v8/BindingUtilities.h" as now we are accessing this file > even > > from the webdatabase module? WDYT? > > Ah, understand the situation. > > I'm not super happy about introducing BindingUtilities, because it will end up > with a random collection of utility functions (we previously had V8Utilities.h, > which I removed a long time ago). That said, I agree that we don't want to put > webdatabase functions into IDBBindingUtilities. BindingUtilities sounds too general to me, too. > > bashi-san: Do you have any suggestion? Can we use toImplArray here somehow as > well? I didn't take a closer look, but it seems that SQLResultSetRowList::item() needs a specific C++ -> V8 conversion which we can't generalize. I don't think functions in V8Binding.h (like toImplArray) make us happy here. One option might be to add a way to create specialized toV8() for a specific type (like DictionaryHelperTraits), but I doubt that it's a good idea given that the current Dictionary class is messed up. yukishiino@ might have a better idea. > > https://codereview.chromium.org/972153002/diff/1/Source/modules/webdatabase/S... > File Source/modules/webdatabase/SQLResultSetRowList.cpp (right): > > https://codereview.chromium.org/972153002/diff/1/Source/modules/webdatabase/S... > Source/modules/webdatabase/SQLResultSetRowList.cpp:69: > item->ForceSet(v8String(isolate, m_columns[i]), value, > static_cast<v8::PropertyAttribute>(v8::DontDelete | v8::ReadOnly)); > > I'm wondering why this is using ForceSet. I think this can be a normal Set.
On 2015/03/04 04:37:49, bashi1 wrote: > On 2015/03/04 04:06:35, haraken wrote: > > I'm not super happy about introducing BindingUtilities, because it will end up > > with a random collection of utility functions (we previously had > V8Utilities.h, > > which I removed a long time ago). That said, I agree that we don't want to put > > webdatabase functions into IDBBindingUtilities. > BindingUtilities sounds too general to me, too. > > > > bashi-san: Do you have any suggestion? Can we use toImplArray here somehow as > > well? > I didn't take a closer look, but it seems that SQLResultSetRowList::item() needs > a specific C++ -> V8 conversion which we can't generalize. I don't think > functions in V8Binding.h (like toImplArray) make us happy here. One option might > be to add a way to create specialized toV8() for a specific type (like > DictionaryHelperTraits), but I doubt that it's a good idea given that the > current Dictionary class is messed up. > > yukishiino@ might have a better idea. My idea is as follows. a) Create a new header named bindings/modules/v8/ToV8.h , which has a function v8::Handle<v8::Value> toV8(const SQLValue& value, creationContext, isolate) following the same manner of toV8 functions in bindings/core/v8/ToV8.h . b) Add a new function in bindings/core/v8/ToV8.h to support a list of key-value pair. template<typename Key, typename Value> v8::Handle<v8::Value> toV8(const Vector<std::pair<Key, Value>>& value, creationContext, isolate) which does almost the same thing as toV8(const Vector&, ...) version, but it stores key-values in v8::Object instead of v8::Array. We may prefer template<typename T> toV8(const Vector<std::pair<String, T>>& value, ...) because keys must be type of string. Now we have two new tools and they should be enough. You create a Vector<pair<String, SQLValue>> and fill it with the result, then, simply call toV8(results). This trick works in general. You can add more modules-specific types in bindings/modules/v8/ToV8.h.
Thanks everyone for the suggestions. Based on the discussion, I have made a patch, PTAL. https://codereview.chromium.org/972153002/diff/20001/Source/bindings/modules/... File Source/bindings/modules/v8/ToV8.h (right): https://codereview.chromium.org/972153002/diff/20001/Source/bindings/modules/... Source/bindings/modules/v8/ToV8.h:5: #ifndef BINDINGS_MODULES_V8_ToV8_h Presubmit throws an error here: Source/bindings/modules/v8/ToV8.h:5: #ifndef header guard has wrong style, please use: ToV8_h [build/header_guard] [5] But this would be a problem considering the existence of Source/bindings/core/v8/ToV8.h using the same header guard. We should probably use a different file name instead of ToV8.h. WDYT?
https://codereview.chromium.org/972153002/diff/20001/Source/modules/webdataba... File Source/modules/webdatabase/SQLResultSetRowList.cpp (right): https://codereview.chromium.org/972153002/diff/20001/Source/modules/webdataba... Source/modules/webdatabase/SQLResultSetRowList.cpp:34: #include "bindings/core/v8/V8Binding.h" The V8Binding.h inclusion is not required. I wanted to know if I am heading in the right direction so I will remove it in the next iteration.
https://codereview.chromium.org/972153002/diff/20001/Source/bindings/core/v8/... File Source/bindings/core/v8/ToV8.h (right): https://codereview.chromium.org/972153002/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/ToV8.h:218: object->Set(v8String(isolate, value[i].first), value[i].second); By passing value[i].second directly to v8::Object::Set(), you are in fact depending on T=v8::Handle<v8::Value>. It would make more sense if the second argument to object->Set() was toV8(value[i].second, creationContext, isolate) You could then call this with a Vector<std::pair<String, SQLValue>> argument instead.
https://codereview.chromium.org/972153002/diff/20001/Source/bindings/core/v8/... File Source/bindings/core/v8/ToV8.h (right): https://codereview.chromium.org/972153002/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/ToV8.h:218: object->Set(v8String(isolate, value[i].first), value[i].second); On 2015/03/04 at 13:53:52, Jens Widell wrote: > By passing value[i].second directly to v8::Object::Set(), you are in fact depending on T=v8::Handle<v8::Value>. > > It would make more sense if the second argument to object->Set() was > > toV8(value[i].second, creationContext, isolate) > > You could then call this with a Vector<std::pair<String, SQLValue>> argument instead. Thanks Jens, I did the same thing initially. Though that would create a dependency on modules/v8/ToV8.h which needs to be included in the core which I thought would be a layering violation. No?
https://codereview.chromium.org/972153002/diff/20001/Source/bindings/core/v8/... File Source/bindings/core/v8/ToV8.h (right): https://codereview.chromium.org/972153002/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/ToV8.h:218: object->Set(v8String(isolate, value[i].first), value[i].second); On 2015/03/04 13:57:19, vivekg_ wrote: > On 2015/03/04 at 13:53:52, Jens Widell wrote: > > By passing value[i].second directly to v8::Object::Set(), you are in fact > depending on T=v8::Handle<v8::Value>. > > > > It would make more sense if the second argument to object->Set() was > > > > toV8(value[i].second, creationContext, isolate) > > > > You could then call this with a Vector<std::pair<String, SQLValue>> argument > instead. > > Thanks Jens, I did the same thing initially. > Though that would create a dependency on modules/v8/ToV8.h which needs to be > included in the core which I thought would be a layering violation. No? It would certainly be wrong to include modules/v8/ToV8.h from core/v8/ToV8.h, but luckily there would be no need for that. You would only need to have included modules/v8/ToV8.h at the point where you call this toV8() function with such an argument. (Which you already do.)
yukishiino@chromium.org changed reviewers: + yukishiino@chromium.org
https://codereview.chromium.org/972153002/diff/20001/Source/bindings/core/v8/... File Source/bindings/core/v8/ToV8.h (right): https://codereview.chromium.org/972153002/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/ToV8.h:214: inline v8::Handle<v8::Value> toV8(const Vector<std::pair<String, T>>& value, v8::Handle<v8::Object> creationContext, v8::Isolate* isolate) Since both of toV8(Vector<T>) and toV8(HeapVector<T>) returns v8::Array, and both of them use toV8SequenceInternal, I think these two are closer to each other than your new toV8(Vector<pair>). So I'd prefer put this new function at the end of the file (after HeapVector version). https://codereview.chromium.org/972153002/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/ToV8.h:218: object->Set(v8String(isolate, value[i].first), value[i].second); Please use toV8(value[i].second, creationContext, isolate) for the value so we can pass any v8-convertible types. (Otherwise template<typename T> is meaningless.) See toV8SequenceInternal() in this file for reference. https://codereview.chromium.org/972153002/diff/20001/Source/bindings/modules/... File Source/bindings/modules/v8/ToV8.h (right): https://codereview.chromium.org/972153002/diff/20001/Source/bindings/modules/... Source/bindings/modules/v8/ToV8.h:5: #ifndef BINDINGS_MODULES_V8_ToV8_h On 2015/03/04 13:34:04, vivekg_ wrote: > Presubmit throws an error here: > > Source/bindings/modules/v8/ToV8.h:5: #ifndef header guard has wrong style, > please use: ToV8_h [build/header_guard] [5] > > But this would be a problem considering the existence of > Source/bindings/core/v8/ToV8.h using the same header guard. > We should probably use a different file name instead of ToV8.h. WDYT? I personally think the style guide is wrong. As we're going to merge the Chromium repository and Blink repository, we'll have many instances of this issue. So the style guide (and lint) should be fixed. But this is just my personal opinion. What do other people think? I'm okay to change the file name for the time being. (Looks inconsistent, though.) https://codereview.chromium.org/972153002/diff/20001/Source/bindings/modules/... Source/bindings/modules/v8/ToV8.h:15: inline v8::Handle<v8::Value> toV8(const SQLValue& sqlValue, v8::Isolate* isolate) The signature of toV8() functions must be v8::Handle<v8::Value> toV8(T value, v8::Handle<v8::Object> creationContext, v8::Isolate* isolate) https://codereview.chromium.org/972153002/diff/20001/Source/modules/webdataba... File Source/modules/webdatabase/SQLResultSetRowList.cpp (right): https://codereview.chromium.org/972153002/diff/20001/Source/modules/webdataba... Source/modules/webdatabase/SQLResultSetRowList.cpp:50: ScriptValue SQLResultSetRowList::item(unsigned index, ExceptionState& exceptionState) This function should return Vector<pair<String, SQLValue>>. Do not call any toV8 functions here. Just create Vector<pair<String, SQLValue>> results; here and fill it, and then return it. The rest of part should be taken care by the binding code generator. The current implementation of the binding code generator generates the following code: v8SetReturnValue(info, impl->item(index).v8Value()); This is wrong. This must be: v8SetReturnValue(info, toV8(impl->item(index))); Then, impl->item(index) returns Vector<pair<String, SQLValue>> and toV8(Vector<pair<String, SQLValue>>, ...) is called. Within this toV8, your another toV8(SQLValue, ...) is called.
https://codereview.chromium.org/972153002/diff/20001/Source/modules/webdataba... File Source/modules/webdatabase/SQLResultSetRowList.cpp (right): https://codereview.chromium.org/972153002/diff/20001/Source/modules/webdataba... Source/modules/webdatabase/SQLResultSetRowList.cpp:50: ScriptValue SQLResultSetRowList::item(unsigned index, ExceptionState& exceptionState) On 2015/03/04 14:11:04, Yuki wrote: > This function should return Vector<pair<String, SQLValue>>. > > Do not call any toV8 functions here. Just create > Vector<pair<String, SQLValue>> results; > here and fill it, and then return it. > > The rest of part should be taken care by the binding code generator. > > The current implementation of the binding code generator generates the following > code: > v8SetReturnValue(info, impl->item(index).v8Value()); > This is wrong. This must be: > v8SetReturnValue(info, toV8(impl->item(index))); > > Then, impl->item(index) returns Vector<pair<String, SQLValue>> and > toV8(Vector<pair<String, SQLValue>>, ...) is called. Within this toV8, your > another toV8(SQLValue, ...) is called. I disagree. The code generator determines what the C++ implementation should return based on the return type in the IDL. This function returns "any", so the code generator will determine that it should return ScriptValue, and so this function must return something that's compatible with being stored in a local variable of type ScriptValue. Relying on toV8() overloading would be convenient in this type of case, but it would also mean that there would be no typing between what the C++ implementation returns and the declared return type in the IDL. BTW, the generated code here, because of [RaisesException], is actually ScriptValue result = impl->item(index, exceptionState); if (exceptionState.hadException()) { exceptionState.throwIfNeeded(); return; } v8SetReturnValue(info, result.v8Value());
https://codereview.chromium.org/972153002/diff/20001/Source/modules/webdataba... File Source/modules/webdatabase/SQLResultSetRowList.cpp (right): https://codereview.chromium.org/972153002/diff/20001/Source/modules/webdataba... Source/modules/webdatabase/SQLResultSetRowList.cpp:50: ScriptValue SQLResultSetRowList::item(unsigned index, ExceptionState& exceptionState) On 2015/03/04 14:18:26, Jens Widell wrote: > On 2015/03/04 14:11:04, Yuki wrote: > > This function should return Vector<pair<String, SQLValue>>. > > > > Do not call any toV8 functions here. Just create > > Vector<pair<String, SQLValue>> results; > > here and fill it, and then return it. > > > > The rest of part should be taken care by the binding code generator. > > > > The current implementation of the binding code generator generates the > following > > code: > > v8SetReturnValue(info, impl->item(index).v8Value()); > > This is wrong. This must be: > > v8SetReturnValue(info, toV8(impl->item(index))); > > > > Then, impl->item(index) returns Vector<pair<String, SQLValue>> and > > toV8(Vector<pair<String, SQLValue>>, ...) is called. Within this toV8, your > > another toV8(SQLValue, ...) is called. > > I disagree. The code generator determines what the C++ implementation should > return based on the return type in the IDL. This function returns "any", so the > code generator will determine that it should return ScriptValue, and so this > function must return something that's compatible with being stored in a local > variable of type ScriptValue. > > Relying on toV8() overloading would be convenient in this type of case, but it > would also mean that there would be no typing between what the C++ > implementation returns and the declared return type in the IDL. > > BTW, the generated code here, because of [RaisesException], is actually > > ScriptValue result = impl->item(index, exceptionState); > if (exceptionState.hadException()) { > exceptionState.throwIfNeeded(); > return; > } > v8SetReturnValue(info, result.v8Value()); That said, we should still perhaps not use toV8() here. We could have a utility function implement in bindings/ with the signature ScriptValue f(const Vector<pair<String, SQLValue>>&) that this code uses to constructor the return value.
https://codereview.chromium.org/972153002/diff/20001/Source/modules/webdataba... File Source/modules/webdatabase/SQLResultSetRowList.cpp (right): https://codereview.chromium.org/972153002/diff/20001/Source/modules/webdataba... Source/modules/webdatabase/SQLResultSetRowList.cpp:50: ScriptValue SQLResultSetRowList::item(unsigned index, ExceptionState& exceptionState) On 2015/03/04 14:23:41, Jens Widell wrote: > On 2015/03/04 14:18:26, Jens Widell wrote: > > On 2015/03/04 14:11:04, Yuki wrote: > > > This function should return Vector<pair<String, SQLValue>>. > > > > > > Do not call any toV8 functions here. Just create > > > Vector<pair<String, SQLValue>> results; > > > here and fill it, and then return it. > > > > > > The rest of part should be taken care by the binding code generator. > > > > > > The current implementation of the binding code generator generates the > > following > > > code: > > > v8SetReturnValue(info, impl->item(index).v8Value()); > > > This is wrong. This must be: > > > v8SetReturnValue(info, toV8(impl->item(index))); > > > > > > Then, impl->item(index) returns Vector<pair<String, SQLValue>> and > > > toV8(Vector<pair<String, SQLValue>>, ...) is called. Within this toV8, your > > > another toV8(SQLValue, ...) is called. > > > > I disagree. The code generator determines what the C++ implementation should > > return based on the return type in the IDL. This function returns "any", so > the > > code generator will determine that it should return ScriptValue, and so this > > function must return something that's compatible with being stored in a local > > variable of type ScriptValue. > > > > Relying on toV8() overloading would be convenient in this type of case, but it > > would also mean that there would be no typing between what the C++ > > implementation returns and the declared return type in the IDL. > > > > BTW, the generated code here, because of [RaisesException], is actually > > > > ScriptValue result = impl->item(index, exceptionState); > > if (exceptionState.hadException()) { > > exceptionState.throwIfNeeded(); > > return; > > } > > v8SetReturnValue(info, result.v8Value()); > > That said, we should still perhaps not use toV8() here. We could have a utility > function implement in bindings/ with the signature > > ScriptValue f(const Vector<pair<String, SQLValue>>&) > > that this code uses to constructor the return value. Agreed. Otherwise, we face to another issue how to get creationContext and isolate. v8::Isolate::GetCurrent() is not recommended today, IIUC. I'm not quite sure if a) ScriptValue was designed to simply hold v8::Value, or b) it was designed to represent "any" type in IDL. In the former case, we may need a new class to represent "any" type.
Thank you, done the changes accordingly. PTAL.
https://codereview.chromium.org/972153002/diff/40001/Source/bindings/modules/... File Source/bindings/modules/v8/ScriptValueUtilities.h (right): https://codereview.chromium.org/972153002/diff/40001/Source/bindings/modules/... Source/bindings/modules/v8/ScriptValueUtilities.h:18: ScriptState* scriptState = ScriptState::current(isolate); It would be better if this function just took a ScriptState* argument. If you add [CallWith=ScriptState] in the IDL, SQLResultSetRowList::item() will be called with a ScriptState* as the first argument.
lgtm on my side. Please address Jens' comment. https://codereview.chromium.org/972153002/diff/40001/Source/bindings/core/v8/... File Source/bindings/core/v8/ToV8Test.cpp (right): https://codereview.chromium.org/972153002/diff/40001/Source/bindings/core/v8/... Source/bindings/core/v8/ToV8Test.cpp:232: Vector<std::pair<String, v8::Handle<v8::Value>>> dictionary; Optional: You can test with Vector<pair<String, int>>. https://codereview.chromium.org/972153002/diff/40001/Source/modules/webdataba... File Source/modules/webdatabase/SQLResultSetRowList.cpp (right): https://codereview.chromium.org/972153002/diff/40001/Source/modules/webdataba... Source/modules/webdatabase/SQLResultSetRowList.cpp:64: } nit: // namespace blink
On 2015/03/04 at 15:53:28, jl wrote: > https://codereview.chromium.org/972153002/diff/40001/Source/bindings/modules/... > File Source/bindings/modules/v8/ScriptValueUtilities.h (right): > > https://codereview.chromium.org/972153002/diff/40001/Source/bindings/modules/... > Source/bindings/modules/v8/ScriptValueUtilities.h:18: ScriptState* scriptState = ScriptState::current(isolate); > It would be better if this function just took a ScriptState* argument. > > If you add [CallWith=ScriptState] in the IDL, SQLResultSetRowList::item() will be called with a ScriptState* as the first argument. Sure, for this we need to have this support CL. https://codereview.chromium.org/968573004
Thanks yukishiino@, done all the changes. PTAL.
LGTM
On 2015/03/05 at 07:12:00, jl wrote: > LGTM Thank you jl@, now awaiting approval for CL https://codereview.chromium.org/968573004 before landing this.
lgtm
https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... File Source/bindings/modules/v8/ScriptValueUtilities.h (right): https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... Source/bindings/modules/v8/ScriptValueUtilities.h:15: inline ScriptValue toScriptValue(ScriptState* scriptState, const Vector<std::pair<String, SQLValue>>& value) Sorry, I'm probably missing what you've discussed, but can we put this function to ScriptValue.h? I'm not super happy about increasing XXXUtilities.h. https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... Source/bindings/modules/v8/ScriptValueUtilities.h:17: RELEASE_ASSERT(scriptState); Remove this. We normally don't check this and we don't need to check this here because if scriptState is 0, the below scriptState-> will just crash. https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... File Source/bindings/modules/v8/ToV8Utilities.h (right): https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... Source/bindings/modules/v8/ToV8Utilities.h:13: inline v8::Handle<v8::Value> toV8(const SQLValue& sqlValue, v8::Handle<v8::Object> creationContext, v8::Isolate* isolate) I think we should - rename this file to ToV8ForModules.h - rename ToV8.h to ToV8ForCore.h (in a follow up) - move toV8 functions in IDBBindingUtilities.h to ToV8ForModules.h (in a follow up) https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... File Source/bindings/modules/v8/custom/V8SQLResultSetRowListCustom.cpp (left): https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... Source/bindings/modules/v8/custom/V8SQLResultSetRowListCustom.cpp:60: if (index >= rowList->length()) { Just to confirm: Is this condition covered in the auto-generated code? https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... Source/bindings/modules/v8/custom/V8SQLResultSetRowListCustom.cpp:87: item->ForceSet(v8String(info.GetIsolate(), rowList->columnNames()[i]), value, static_cast<v8::PropertyAttribute>(v8::DontDelete | v8::ReadOnly)); This CL is changing behavior since you're changing ForceSet to Set, but I believe it doesn't matter (ForceSet is discouraged).
Thanks haraken@, please find my comments inline. https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... File Source/bindings/modules/v8/ScriptValueUtilities.h (right): https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... Source/bindings/modules/v8/ScriptValueUtilities.h:15: inline ScriptValue toScriptValue(ScriptState* scriptState, const Vector<std::pair<String, SQLValue>>& value) On 2015/03/05 at 09:03:13, haraken wrote: > Sorry, I'm probably missing what you've discussed, but can we put this function to ScriptValue.h? I'm not super happy about increasing XXXUtilities.h. The reason we have put this under the binding/modules is due to the usage of SQLValue which is module specific. And there will be one more addition here in this file with https://codereview.chromium.org/949193002 e.g. Vector<SQLValue> toSQLValueArray(const Nullable<Vector<ScriptValue>>& arguments) https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... Source/bindings/modules/v8/ScriptValueUtilities.h:17: RELEASE_ASSERT(scriptState); On 2015/03/05 at 09:03:13, haraken wrote: > Remove this. We normally don't check this and we don't need to check this here because if scriptState is 0, the below scriptState-> will just crash. Sure. https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... File Source/bindings/modules/v8/ToV8Utilities.h (right): https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... Source/bindings/modules/v8/ToV8Utilities.h:13: inline v8::Handle<v8::Value> toV8(const SQLValue& sqlValue, v8::Handle<v8::Object> creationContext, v8::Isolate* isolate) On 2015/03/05 at 09:03:13, haraken wrote: > I think we should > > - rename this file to ToV8ForModules.h Sure. > - rename ToV8.h to ToV8ForCore.h (in a follow up) > - move toV8 functions in IDBBindingUtilities.h to ToV8ForModules.h (in a follow up) Yeah, make sense to do as a followup. Will do both. https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... File Source/bindings/modules/v8/custom/V8SQLResultSetRowListCustom.cpp (left): https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... Source/bindings/modules/v8/custom/V8SQLResultSetRowListCustom.cpp:60: if (index >= rowList->length()) { On 2015/03/05 at 09:03:13, haraken wrote: > Just to confirm: Is this condition covered in the auto-generated code? This is not covered by the generated code. There must be an explicit check as done in SQLResultSetRowList::item() in SQLResultSetRowList.cpp https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... Source/bindings/modules/v8/custom/V8SQLResultSetRowListCustom.cpp:87: item->ForceSet(v8String(info.GetIsolate(), rowList->columnNames()[i]), value, static_cast<v8::PropertyAttribute>(v8::DontDelete | v8::ReadOnly)); On 2015/03/05 at 09:03:13, haraken wrote: > This CL is changing behavior since you're changing ForceSet to Set, but I believe it doesn't matter (ForceSet is discouraged). Yeah, you are right. Its doesn't matter here.
https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... File Source/bindings/modules/v8/ScriptValueUtilities.h (right): https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... Source/bindings/modules/v8/ScriptValueUtilities.h:15: inline ScriptValue toScriptValue(ScriptState* scriptState, const Vector<std::pair<String, SQLValue>>& value) On 2015/03/05 09:13:53, vivekg_ wrote: > On 2015/03/05 at 09:03:13, haraken wrote: > > Sorry, I'm probably missing what you've discussed, but can we put this > function to ScriptValue.h? I'm not super happy about increasing XXXUtilities.h. > > The reason we have put this under the binding/modules is due to the usage of > SQLValue which is module specific. > And there will be one more addition here in this file with > https://codereview.chromium.org/949193002 > > e.g. Vector<SQLValue> toSQLValueArray(const Nullable<Vector<ScriptValue>>& > arguments) ah, understood. Can we put this function to SQLValue.h? There is no problem to use this function in modules/ because this function is not using any V8 API. Both ScriptValue and ScriptState are well-defined abstractions and it's OK to use them in core/ and modules/ (we're already doing this in many places). Sorry about an ambiguous rule. The rule is just that we want to avoid using V8 APIs (i.e., writing "v8::") in core/ and modules/.
https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... File Source/bindings/modules/v8/ScriptValueUtilities.h (right): https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... Source/bindings/modules/v8/ScriptValueUtilities.h:15: inline ScriptValue toScriptValue(ScriptState* scriptState, const Vector<std::pair<String, SQLValue>>& value) On 2015/03/05 09:13:53, vivekg_ wrote: > On 2015/03/05 at 09:03:13, haraken wrote: > > Sorry, I'm probably missing what you've discussed, but can we put this > function to ScriptValue.h? I'm not super happy about increasing XXXUtilities.h. > > The reason we have put this under the binding/modules is due to the usage of > SQLValue which is module specific. > And there will be one more addition here in this file with > https://codereview.chromium.org/949193002 > > e.g. Vector<SQLValue> toSQLValueArray(const Nullable<Vector<ScriptValue>>& > arguments) We can have the following template function: template<typename T> ScriptValue toScriptValue(ScriptState* scriptState, T value) { return ScriptValue(scriptState, toV8(value, scriptState->context()->Global(), scriptState->isolate())); } This doesn't directly depend on a specific type in modules/. Also we don't need to add a new type support in addition to Vector<pair<String, SQLValue>> in future.
On 2015/03/05 09:25:21, Yuki wrote: > https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... > File Source/bindings/modules/v8/ScriptValueUtilities.h (right): > > https://codereview.chromium.org/972153002/diff/60001/Source/bindings/modules/... > Source/bindings/modules/v8/ScriptValueUtilities.h:15: inline ScriptValue > toScriptValue(ScriptState* scriptState, const Vector<std::pair<String, > SQLValue>>& value) > On 2015/03/05 09:13:53, vivekg_ wrote: > > On 2015/03/05 at 09:03:13, haraken wrote: > > > Sorry, I'm probably missing what you've discussed, but can we put this > > function to ScriptValue.h? I'm not super happy about increasing > XXXUtilities.h. > > > > The reason we have put this under the binding/modules is due to the usage of > > SQLValue which is module specific. > > And there will be one more addition here in this file with > > https://codereview.chromium.org/949193002 > > > > e.g. Vector<SQLValue> toSQLValueArray(const Nullable<Vector<ScriptValue>>& > > arguments) > > We can have the following template function: > > template<typename T> > ScriptValue toScriptValue(ScriptState* scriptState, T value) > { > return ScriptValue(scriptState, toV8(value, scriptState->context()->Global(), > scriptState->isolate())); > } > > This doesn't directly depend on a specific type in modules/. > Also we don't need to add a new type support in addition to Vector<pair<String, > SQLValue>> in future. I like the idea :)
On 2015/03/05 at 09:30:36, haraken wrote:
> I like the idea :)
+1.
One question remains with the removal of SQLTransaction.idl's custom binding
removal, I have added a function in ScriptValueUtilities.h as:
+Vector<SQLValue> toSQLValueArray(const Nullable<Vector<ScriptValue>>&
arguments)
+{
+ if (arguments.isNull())
+ return Vector<SQLValue>();
+
+ Vector<SQLValue> sqlArguments;
+ const Vector<ScriptValue>& args = arguments.get();
+ for (unsigned i = 0; i < args.size(); ++i) {
+ const v8::Local<v8::Value>& arg = args[i].v8Value();
+ if (arg.IsEmpty() || arg->IsNull()) {
+ sqlArguments.append(SQLValue());
+ } else if (arg->IsNumber()) {
+ sqlArguments.append(SQLValue(arg->NumberValue()));
+ } else {
+ TOSTRING_DEFAULT(V8StringResource<>, stringValue, arg,
Vector<SQLValue>());
+ sqlArguments.append(SQLValue(stringValue));
+ }
+ }
+ return sqlArguments;
+}
+
So any better ideas about handling this as well? I am bit doubtful about using
any template functions here as this is too specific to SQLValue. WDYT?
On 2015/03/05 09:36:51, vivekg_ wrote:
> On 2015/03/05 at 09:30:36, haraken wrote:
>
> > I like the idea :)
>
> +1.
>
> One question remains with the removal of SQLTransaction.idl's custom binding
> removal, I have added a function in ScriptValueUtilities.h as:
>
> +Vector<SQLValue> toSQLValueArray(const Nullable<Vector<ScriptValue>>&
> arguments)
> +{
> + if (arguments.isNull())
> + return Vector<SQLValue>();
> +
> + Vector<SQLValue> sqlArguments;
> + const Vector<ScriptValue>& args = arguments.get();
> + for (unsigned i = 0; i < args.size(); ++i) {
> + const v8::Local<v8::Value>& arg = args[i].v8Value();
> + if (arg.IsEmpty() || arg->IsNull()) {
> + sqlArguments.append(SQLValue());
> + } else if (arg->IsNumber()) {
> + sqlArguments.append(SQLValue(arg->NumberValue()));
> + } else {
> + TOSTRING_DEFAULT(V8StringResource<>, stringValue, arg,
> Vector<SQLValue>());
> + sqlArguments.append(SQLValue(stringValue));
> + }
> + }
> + return sqlArguments;
> +}
> +
>
> So any better ideas about handling this as well? I am bit doubtful about using
> any template functions here as this is too specific to SQLValue. WDYT?
Sorry for the back and forth, but I now begin to think that we'll need something
like V8BindingForModules. We can put those things and IDBBindingUtilities into
V8BindingForModules.
So my proposal is:
- core/v8/ToV8.h (or ToV8ForCore.h)
- modules/v8/ToV8ForModules.h
- core/v8/V8Binding.h (or V8BindingForCore.h)
- modules/v8/V8BindingForModules.h
On 2015/03/05 at 09:42:40, haraken wrote: > Sorry for the back and forth, but I now begin to think that we'll need something like V8BindingForModules. We can put those things and IDBBindingUtilities into V8BindingForModules. > > So my proposal is: > > - core/v8/ToV8.h (or ToV8ForCore.h) > - modules/v8/ToV8ForModules.h > - core/v8/V8Binding.h (or V8BindingForCore.h) > - modules/v8/V8BindingForModules.h +1 Sounds a very nice idea. I will do the changes accordingly. Thanks for all the inputs, appreciate that.
Done, PTAL. Thanks.
LGTM
The CQ bit was checked by vivekg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yukishiino@chromium.org, jl@opera.com Link to the patchset: https://codereview.chromium.org/972153002/#ps80001 (title: "Patch for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/972153002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=191359
Message was sent while issue was closed.
Description was changed from ========== [bindings] Remove custom binding usage in SQLResultSetRowList. As per [1], removing the usage of custom binding required for SQLResultSetRowList. R=jl@opera.com, haraken@chromium.org, jsbell@chromium.org BUG=345519 [1] http://www.w3.org/TR/webdatabase/#sqlresultsetrowlist Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191359 ========== to ========== [bindings] Remove custom binding usage in SQLResultSetRowList. As per [1], removing the usage of custom binding required for SQLResultSetRowList. R=jl@opera.com, haraken@chromium.org, jsbell@chromium.org BUG=345519 [1] http://www.w3.org/TR/webdatabase/#sqlresultsetrowlist Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191359 ========== |
