Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1021713003:
[bindings] Let NativeValueTraits<T>::nativeValue be variadic function and merge various convers… (Closed)
|
Created:
5 years, 9 months ago by vivekg_samsung Modified:
5 years, 9 months ago CC:
aandrey+blink_chromium.org, apavlov+blink_chromium.org, arv+blink, blink-reviews, blink-reviews-bindings_chromium.org, caseq+blink_chromium.org, cmumford, devtools-reviews_chromium.org, dgrogan, eustas+blink_chromium.org, jsbell+idb_chromium.org, kozyatinskiy+blink_chromium.org, loislo+blink_chromium.org, lushnikov+blink_chromium.org, malch+blink_chromium.org, pfeldman+blink_chromium.org, sergeyv+blink_chromium.org, vivekg_samsung, yurys+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
Description[bindings] Let NativeValueTraits<T>::nativeValue be variadic function and merge various conversion methods
There exist multiple methods to convert from ScriptValue to IDB*. With these fragmented
methods, there might exist some bugs (even security implications) related to v8 API usage.
Having a single window to such conversions would help reduce these inconsistencies.
Make an universal converter function to be static in ScriptValue to assist any such conversions.
Using the NativeValueTraits<T>::nativeValue(), the conversion can be achieved.
R=haraken@chromium.org, yukishiino@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192250
Patch Set 1 #Patch Set 2 : Fixed the compiler error behind ASSERT flag #
Total comments: 5
Patch Set 3 : IDB related changes alone. #
Messages
Total messages: 24 (5 generated)
vivekg@chromium.org changed reviewers: + vivekg@chromium.org
The things being done in this CL: * Make NativeValueTraits<T>::nativeValue a variadic template method to allow additional arguments required along with ScriptValue * Make ScriptValue::to<T> static as class method is more suitable than being a instance method. Also this is marked as variadic as well. * Remove createIDBKeyFromScriptValueAndKeyPath() from V8BindingForModules.h and make the native traits handle both cases. * Remove v8ToJSONValue from V8Binding.h This is parking CL for discussion of the merits/demerits of the same. If agreed, we should split this into multiple logically grouped patches. WDYT? Thanks in advance for your review on this. :)
jsbell@chromium.org changed reviewers: + jsbell@chromium.org
idb changes lgtm Having the conversion function be variadic, with the extra arguments depending on the traits, is a little unusual. It may be less readable at the call sites without context, but in all these cases it's pretty clear (i.e. there's an explicit maxDepth or keyPath argument) https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... File Source/bindings/modules/v8/V8BindingForModules.h (right): https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... Source/bindings/modules/v8/V8BindingForModules.h:39: static IDBKey* nativeValue(v8::Local<v8::Value>, v8::Isolate*, ExceptionState&, const IDBKeyPath& = IDBKeyPath()); It's possible that creating a temporary IDBKeyPath here in the non-keypath case will show up as a performance regression. I'm fine with trying this as-is, but if necessary will an overload work with the template function/traits approach?
Thank you jsbell@ for the review, I appreciate your feedback. https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... File Source/bindings/modules/v8/V8BindingForModules.h (right): https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... Source/bindings/modules/v8/V8BindingForModules.h:39: static IDBKey* nativeValue(v8::Local<v8::Value>, v8::Isolate*, ExceptionState&, const IDBKeyPath& = IDBKeyPath()); On 2015/03/19 16:46:57, jsbell wrote: > It's possible that creating a temporary IDBKeyPath here in the non-keypath case > will show up as a performance regression. I'm fine with trying this as-is, but > if necessary will an overload work with the template function/traits approach? Sure, we can have the overloaded method for non IDBKeyPath case so as not to affect the performance.
https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... File Source/core/inspector/InjectedScript.cpp (right): https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... Source/core/inspector/InjectedScript.cpp:285: ScriptState::Scope scope(state); You sometimes enter a ScriptState::Scope but sometimes don't. Where does the difference come from? For example, I wonder why we need to enter a ScriptState::Scope here.
On 2015/03/19 23:26:13, haraken wrote: > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > File Source/core/inspector/InjectedScript.cpp (right): > > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > Source/core/inspector/InjectedScript.cpp:285: ScriptState::Scope scope(state); > > You sometimes enter a ScriptState::Scope but sometimes don't. Where does the > difference come from? > > For example, I wonder why we need to enter a ScriptState::Scope here. Yes this is where I had a question. The places where we call ScriptValue.toJSONValue(), as the method currently enters the scope [1], I added the explicit scope creation. Is it really required here? [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
On 2015/03/19 23:36:52, vivekg_ wrote: > On 2015/03/19 23:26:13, haraken wrote: > > > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > > File Source/core/inspector/InjectedScript.cpp (right): > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > > Source/core/inspector/InjectedScript.cpp:285: ScriptState::Scope scope(state); > > > > You sometimes enter a ScriptState::Scope but sometimes don't. Where does the > > difference come from? > > > > For example, I wonder why we need to enter a ScriptState::Scope here. > > Yes this is where I had a question. The places where we call > ScriptValue.toJSONValue(), as the method currently enters the scope [1], I added > the explicit scope creation. Is it really required here? > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Basically, we must be in a correct ScriptState always. We must be in a correct ScriptState in toJSONValue. The question is whether we're already in a correct ScriptState when calling toJSONValue. If yes, we don't need to re-enter the ScriptState. As far as I see this patch, it looks like we're already in a correct ScriptState when calling toJSONValue. So I guess we won't need to re-enter a ScriptState. The place where we have to enter a ScriptState is limited: when Blink calls binding code asynchronously and we need to resume the ScriptState recorded before.
bashi@chromium.org changed reviewers: + bashi@chromium.org
https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... File Source/bindings/modules/v8/V8BindingForModulesTest.cpp (right): https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... Source/bindings/modules/v8/V8BindingForModulesTest.cpp:45: NonThrowableExceptionState exceptionState; Can we pass exceptionState from the caller of this function? We want to minimize use of NonThrowableExceptionState. If it's difficult, please add a TODO comment. I understand some nativeValue() methods never throw an exception via exceptionState, but it could be changed in the future and if that happens we will swallow exceptions unexpectedly. https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... File Source/core/inspector/InjectedScript.cpp (right): https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... Source/core/inspector/InjectedScript.cpp:286: NonThrowableExceptionState exceptionState; It would be better not to use NonThrowableExceptionState here, too.
On 2015/03/19 23:51:14, haraken wrote: > On 2015/03/19 23:36:52, vivekg_ wrote: > > On 2015/03/19 23:26:13, haraken wrote: > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > > > File Source/core/inspector/InjectedScript.cpp (right): > > > > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > > > Source/core/inspector/InjectedScript.cpp:285: ScriptState::Scope > scope(state); > > > > > > You sometimes enter a ScriptState::Scope but sometimes don't. Where does the > > > difference come from? > > > > > > For example, I wonder why we need to enter a ScriptState::Scope here. > > > > Yes this is where I had a question. The places where we call > > ScriptValue.toJSONValue(), as the method currently enters the scope [1], I > added > > the explicit scope creation. Is it really required here? > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Basically, we must be in a correct ScriptState always. We must be in a correct > ScriptState in toJSONValue. The question is whether we're already in a correct > ScriptState when calling toJSONValue. If yes, we don't need to re-enter the > ScriptState. As far as I see this patch, it looks like we're already in a > correct ScriptState when calling toJSONValue. So I guess we won't need to > re-enter a ScriptState. > > The place where we have to enter a ScriptState is limited: when Blink calls > binding code asynchronously and we need to resume the ScriptState recorded > before. Ah understood. Nice explanation with async example. I will correct it. Thank you.
On 2015/03/19 23:53:42, bashi1 wrote: > https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... > File Source/bindings/modules/v8/V8BindingForModulesTest.cpp (right): > > https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... > Source/bindings/modules/v8/V8BindingForModulesTest.cpp:45: > NonThrowableExceptionState exceptionState; > Can we pass exceptionState from the caller of this function? We want to minimize > use of NonThrowableExceptionState. If it's difficult, please add a TODO comment. > > I understand some nativeValue() methods never throw an exception via > exceptionState, but it could be changed in the future and if that happens we > will swallow exceptions unexpectedly. > > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > File Source/core/inspector/InjectedScript.cpp (right): > > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > Source/core/inspector/InjectedScript.cpp:286: NonThrowableExceptionState > exceptionState; > It would be better not to use NonThrowableExceptionState here, too. How about making the signature change in NativeValueTraits<T>::nativeValue to accept ExceptionState as pointer and defaulting it to be nullptr instead of a reference. The same is applicable to ScriptState::to<>(). WDYT?
On 2015/03/20 00:18:07, vivekg_ wrote: > On 2015/03/19 23:53:42, bashi1 wrote: > > > https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... > > File Source/bindings/modules/v8/V8BindingForModulesTest.cpp (right): > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... > > Source/bindings/modules/v8/V8BindingForModulesTest.cpp:45: > > NonThrowableExceptionState exceptionState; > > Can we pass exceptionState from the caller of this function? We want to > minimize > > use of NonThrowableExceptionState. If it's difficult, please add a TODO > comment. > > > > I understand some nativeValue() methods never throw an exception via > > exceptionState, but it could be changed in the future and if that happens we > > will swallow exceptions unexpectedly. > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > > File Source/core/inspector/InjectedScript.cpp (right): > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > > Source/core/inspector/InjectedScript.cpp:286: NonThrowableExceptionState > > exceptionState; > > It would be better not to use NonThrowableExceptionState here, too. > > How about making the signature change in NativeValueTraits<T>::nativeValue to > accept ExceptionState as pointer and defaulting it to be nullptr instead of a > reference. The same is applicable to ScriptState::to<>(). WDYT? Oh sorry, it can't be a default arg due to variadic nature. On the other hand can we make the ExceptionState itself a variadic param so the conversions where its absolutely required can pass it as reference?
On 2015/03/20 00:18:07, vivekg_ wrote: > On 2015/03/19 23:53:42, bashi1 wrote: > > > https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... > > File Source/bindings/modules/v8/V8BindingForModulesTest.cpp (right): > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... > > Source/bindings/modules/v8/V8BindingForModulesTest.cpp:45: > > NonThrowableExceptionState exceptionState; > > Can we pass exceptionState from the caller of this function? We want to > minimize > > use of NonThrowableExceptionState. If it's difficult, please add a TODO > comment. > > > > I understand some nativeValue() methods never throw an exception via > > exceptionState, but it could be changed in the future and if that happens we > > will swallow exceptions unexpectedly. > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > > File Source/core/inspector/InjectedScript.cpp (right): > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > > Source/core/inspector/InjectedScript.cpp:286: NonThrowableExceptionState > > exceptionState; > > It would be better not to use NonThrowableExceptionState here, too. > > How about making the signature change in NativeValueTraits<T>::nativeValue to > accept ExceptionState as pointer and defaulting it to be nullptr instead of a > reference. The same is applicable to ScriptState::to<>(). WDYT? Hmm, it means that every nativeValue() need to have !exceptionState check, which I'm not very happy with. Is it possible to ExceptionState& be a variadic argument as well?
On 2015/03/20 00:22:37, bashi1 wrote: > On 2015/03/20 00:18:07, vivekg_ wrote: > > On 2015/03/19 23:53:42, bashi1 wrote: > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... > > > File Source/bindings/modules/v8/V8BindingForModulesTest.cpp (right): > > > > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... > > > Source/bindings/modules/v8/V8BindingForModulesTest.cpp:45: > > > NonThrowableExceptionState exceptionState; > > > Can we pass exceptionState from the caller of this function? We want to > > minimize > > > use of NonThrowableExceptionState. If it's difficult, please add a TODO > > comment. > > > > > > I understand some nativeValue() methods never throw an exception via > > > exceptionState, but it could be changed in the future and if that happens we > > > will swallow exceptions unexpectedly. > > > > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > > > File Source/core/inspector/InjectedScript.cpp (right): > > > > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > > > Source/core/inspector/InjectedScript.cpp:286: NonThrowableExceptionState > > > exceptionState; > > > It would be better not to use NonThrowableExceptionState here, too. > > > > How about making the signature change in NativeValueTraits<T>::nativeValue to > > accept ExceptionState as pointer and defaulting it to be nullptr instead of a > > reference. The same is applicable to ScriptState::to<>(). WDYT? > > Hmm, it means that every nativeValue() need to have !exceptionState check, which > I'm not very happy with. Is it possible to ExceptionState& be a variadic > argument as well? Yeah I realized the same and said the same in #13. I think it makes sense for it to be variadic.
On 2015/03/20 00:25:58, vivekg_ wrote: > On 2015/03/20 00:22:37, bashi1 wrote: > > On 2015/03/20 00:18:07, vivekg_ wrote: > > > On 2015/03/19 23:53:42, bashi1 wrote: > > > > > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... > > > > File Source/bindings/modules/v8/V8BindingForModulesTest.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... > > > > Source/bindings/modules/v8/V8BindingForModulesTest.cpp:45: > > > > NonThrowableExceptionState exceptionState; > > > > Can we pass exceptionState from the caller of this function? We want to > > > minimize > > > > use of NonThrowableExceptionState. If it's difficult, please add a TODO > > > comment. > > > > > > > > I understand some nativeValue() methods never throw an exception via > > > > exceptionState, but it could be changed in the future and if that happens > we > > > > will swallow exceptions unexpectedly. > > > > > > > > > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > > > > File Source/core/inspector/InjectedScript.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > > > > Source/core/inspector/InjectedScript.cpp:286: NonThrowableExceptionState > > > > exceptionState; > > > > It would be better not to use NonThrowableExceptionState here, too. > > > > > > How about making the signature change in NativeValueTraits<T>::nativeValue > to > > > accept ExceptionState as pointer and defaulting it to be nullptr instead of > a > > > reference. The same is applicable to ScriptState::to<>(). WDYT? > > > > Hmm, it means that every nativeValue() need to have !exceptionState check, > which > > I'm not very happy with. Is it possible to ExceptionState& be a variadic > > argument as well? > > Yeah I realized the same and said the same in #13. I think it makes sense for it > to be variadic. Yeah, I think it makes sense, but I'd like to hear other bindings owners opinion.
On 2015/03/20 at 00:37:24, bashi wrote: > On 2015/03/20 00:25:58, vivekg_ wrote: > > On 2015/03/20 00:22:37, bashi1 wrote: > > > On 2015/03/20 00:18:07, vivekg_ wrote: > > > > On 2015/03/19 23:53:42, bashi1 wrote: > > > > > > > > > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... > > > > > File Source/bindings/modules/v8/V8BindingForModulesTest.cpp (right): > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... > > > > > Source/bindings/modules/v8/V8BindingForModulesTest.cpp:45: > > > > > NonThrowableExceptionState exceptionState; > > > > > Can we pass exceptionState from the caller of this function? We want to > > > > minimize > > > > > use of NonThrowableExceptionState. If it's difficult, please add a TODO > > > > comment. > > > > > > > > > > I understand some nativeValue() methods never throw an exception via > > > > > exceptionState, but it could be changed in the future and if that happens > > we > > > > > will swallow exceptions unexpectedly. > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > > > > > File Source/core/inspector/InjectedScript.cpp (right): > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > > > > > Source/core/inspector/InjectedScript.cpp:286: NonThrowableExceptionState > > > > > exceptionState; > > > > > It would be better not to use NonThrowableExceptionState here, too. > > > > > > > > How about making the signature change in NativeValueTraits<T>::nativeValue > > to > > > > accept ExceptionState as pointer and defaulting it to be nullptr instead of > > a > > > > reference. The same is applicable to ScriptState::to<>(). WDYT? > > > > > > Hmm, it means that every nativeValue() need to have !exceptionState check, > > which > > > I'm not very happy with. Is it possible to ExceptionState& be a variadic > > > argument as well? > > > > Yeah I realized the same and said the same in #13. I think it makes sense for it > > to be variadic. > > Yeah, I think it makes sense, but I'd like to hear other bindings owners opinion. Sure, thanks. haraken@, yuki@: WDYT?
On 2015/03/20 04:51:33, vivekg_ wrote: > On 2015/03/20 at 00:37:24, bashi wrote: > > On 2015/03/20 00:25:58, vivekg_ wrote: > > > On 2015/03/20 00:22:37, bashi1 wrote: > > > > On 2015/03/20 00:18:07, vivekg_ wrote: > > > > > On 2015/03/19 23:53:42, bashi1 wrote: > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... > > > > > > File Source/bindings/modules/v8/V8BindingForModulesTest.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... > > > > > > Source/bindings/modules/v8/V8BindingForModulesTest.cpp:45: > > > > > > NonThrowableExceptionState exceptionState; > > > > > > Can we pass exceptionState from the caller of this function? We want > to > > > > > minimize > > > > > > use of NonThrowableExceptionState. If it's difficult, please add a > TODO > > > > > comment. > > > > > > > > > > > > I understand some nativeValue() methods never throw an exception via > > > > > > exceptionState, but it could be changed in the future and if that > happens > > > we > > > > > > will swallow exceptions unexpectedly. > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > > > > > > File Source/core/inspector/InjectedScript.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > > > > > > Source/core/inspector/InjectedScript.cpp:286: > NonThrowableExceptionState > > > > > > exceptionState; > > > > > > It would be better not to use NonThrowableExceptionState here, too. > > > > > > > > > > How about making the signature change in > NativeValueTraits<T>::nativeValue > > > to > > > > > accept ExceptionState as pointer and defaulting it to be nullptr instead > of > > > a > > > > > reference. The same is applicable to ScriptState::to<>(). WDYT? > > > > > > > > Hmm, it means that every nativeValue() need to have !exceptionState check, > > > which > > > > I'm not very happy with. Is it possible to ExceptionState& be a variadic > > > > argument as well? > > > > > > Yeah I realized the same and said the same in #13. I think it makes sense > for it > > > to be variadic. > > > > Yeah, I think it makes sense, but I'd like to hear other bindings owners > opinion. > > Sure, thanks. haraken@, yuki@: WDYT? If we need NonThrowableExceptionState in a lot of places, variadic parameters sound good. If the places are limited (less than 10 for example), I'd prefer ExceptionState&.
On 2015/03/20 at 06:10:06, haraken wrote: > On 2015/03/20 04:51:33, vivekg_ wrote: > > On 2015/03/20 at 00:37:24, bashi wrote: > > > On 2015/03/20 00:25:58, vivekg_ wrote: > > > > On 2015/03/20 00:22:37, bashi1 wrote: > > > > > On 2015/03/20 00:18:07, vivekg_ wrote: > > > > > > On 2015/03/19 23:53:42, bashi1 wrote: > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... > > > > > > > File Source/bindings/modules/v8/V8BindingForModulesTest.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/bindings/modules... > > > > > > > Source/bindings/modules/v8/V8BindingForModulesTest.cpp:45: > > > > > > > NonThrowableExceptionState exceptionState; > > > > > > > Can we pass exceptionState from the caller of this function? We want > > to > > > > > > minimize > > > > > > > use of NonThrowableExceptionState. If it's difficult, please add a > > TODO > > > > > > comment. > > > > > > > > > > > > > > I understand some nativeValue() methods never throw an exception via > > > > > > > exceptionState, but it could be changed in the future and if that > > happens > > > > we > > > > > > > will swallow exceptions unexpectedly. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > > > > > > > File Source/core/inspector/InjectedScript.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1021713003/diff/20001/Source/core/inspector/I... > > > > > > > Source/core/inspector/InjectedScript.cpp:286: > > NonThrowableExceptionState > > > > > > > exceptionState; > > > > > > > It would be better not to use NonThrowableExceptionState here, too. > > > > > > > > > > > > How about making the signature change in > > NativeValueTraits<T>::nativeValue > > > > to > > > > > > accept ExceptionState as pointer and defaulting it to be nullptr instead > > of > > > > a > > > > > > reference. The same is applicable to ScriptState::to<>(). WDYT? > > > > > > > > > > Hmm, it means that every nativeValue() need to have !exceptionState check, > > > > which > > > > > I'm not very happy with. Is it possible to ExceptionState& be a variadic > > > > > argument as well? > > > > > > > > Yeah I realized the same and said the same in #13. I think it makes sense > > for it > > > > to be variadic. > > > > > > Yeah, I think it makes sense, but I'd like to hear other bindings owners > > opinion. > > > > Sure, thanks. haraken@, yuki@: WDYT? > > If we need NonThrowableExceptionState in a lot of places, variadic parameters sound good. If the places are limited (less than 10 for example), I'd prefer ExceptionState&. I think its better to retain the ExceptionState as we have about 8 places in patchset #2. Now I am splitting this patch into two: one for IDB related changes and the other for JSONValue realted one. PTAL at patset #3.
LGTM
The CQ bit was checked by vivekg@chromium.org
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/1021713003/#ps40001 (title: "IDB related changes alone.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1021713003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=192250 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
