Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1031783003:
Add V8ObjectBuilder helper and use in modules/crypto/ (Closed)
|
Created:
5 years, 9 months ago by Jens Widell Modified:
5 years, 9 months ago CC:
arv+blink, blink-reviews, blink-reviews-bindings_chromium.org, vivekg_samsung Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionAdd V8ObjectBuilder helper and use in modules/crypto/
V8ObjectBuilder is used to create simple V8 objects, and replaces the
corresponding functionality in Dictionary (Dictionary::createEmpty() and
the various Dictionary::set() functions), making Dictionary a read-only
accessor of an object's properties.
BUG=469650
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192599
Patch Set 1 #
Total comments: 13
Patch Set 2 : use ScriptState #
Total comments: 1
Patch Set 3 : include Handle.h #Patch Set 4 : add missing include #Patch Set 5 : make DictionaryBuilder STACK_ALLOCATED too #Patch Set 6 : drop STACK_ALLOCATED #
Total comments: 7
Patch Set 7 : add STACK_ALLOCATED again :-) #Patch Set 8 : add error handling #Patch Set 9 : add final #Patch Set 10 : add generic add() #
Total comments: 1
Patch Set 11 : use "const String&" #
Messages
Total messages: 29 (4 generated)
jl@opera.com changed reviewers: + bashi@chromium.org, haraken@chromium.org
PTAL I've modified V8ObjectBuilder a bit here, mostly to address the comments in https://codereview.chromium.org/1029093003/.
https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... File Source/bindings/core/v8/V8ObjectBuilder.cpp (right): https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... Source/bindings/core/v8/V8ObjectBuilder.cpp:20: return ScriptValue(ScriptState::current(m_isolate), m_object); Using ScriptState::current is not nice, since the ScriptState::current depends on what ScriptState the caller side is using. (For security reasons, it is very important that we always use a correct ScriptState.) I think we can pass in ScriptState to the V8ObjectBuilder, cache the ScriptState on V8ObjectBuilder::m_scriptState, and use it here. https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... Source/bindings/core/v8/V8ObjectBuilder.cpp:61: m_object->ForceSet(m_isolate->GetCurrentContext(), v8String(m_isolate, name), value); Why do we use ForceSet, not Set? https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... Source/bindings/core/v8/V8ObjectBuilder.cpp:61: m_object->ForceSet(m_isolate->GetCurrentContext(), v8String(m_isolate, name), value); Also we should use m_scriptState->context(). https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... File Source/bindings/core/v8/V8ObjectBuilder.h (right): https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... Source/bindings/core/v8/V8ObjectBuilder.h:15: class V8ObjectBuilder { Can we add STACK_ALLOCATED? https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... Source/bindings/core/v8/V8ObjectBuilder.h:17: V8ObjectBuilder(v8::Isolate*); Add explicit. https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... Source/bindings/core/v8/V8ObjectBuilder.h:29: V8ObjectBuilder& addString(String name, String value); Nit: I'd call all of these functions "add".
https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... File Source/bindings/core/v8/V8ObjectBuilder.cpp (right): https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... Source/bindings/core/v8/V8ObjectBuilder.cpp:61: m_object->ForceSet(m_isolate->GetCurrentContext(), v8String(m_isolate, name), value); On 2015/03/24 14:33:32, haraken wrote: > > Why do we use ForceSet, not Set? Web IDL uses [[DefineOwnProperty]] to describe ECMAScript serializers, for which I think ForceSet() is a better match than Set(). Please correct me if I'm wrong. Generally, [[DefineOwnProperty]] is better (than [[Put]]) for all cases where you simply want to build an object, so hopefully this will work for other use-cases besides serializers as well.
On 2015/03/24 14:38:40, Jens Widell wrote: > https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... > File Source/bindings/core/v8/V8ObjectBuilder.cpp (right): > > https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... > Source/bindings/core/v8/V8ObjectBuilder.cpp:61: > m_object->ForceSet(m_isolate->GetCurrentContext(), v8String(m_isolate, name), > value); > On 2015/03/24 14:33:32, haraken wrote: > > > > Why do we use ForceSet, not Set? > > Web IDL uses [[DefineOwnProperty]] to describe ECMAScript serializers, for which > I think ForceSet() is a better match than Set(). Please correct me if I'm wrong. > > Generally, [[DefineOwnProperty]] is better (than [[Put]]) for all cases where > you simply want to build an object, so hopefully this will work for other > use-cases besides serializers as well. ForceSet is for bypassing interceptors (see https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&q=...) and I think that's a different concept than [[DefineOwnProperty]].
https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... File Source/bindings/core/v8/V8ObjectBuilder.h (right): https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... Source/bindings/core/v8/V8ObjectBuilder.h:29: V8ObjectBuilder& addString(String name, String value); On 2015/03/24 14:33:33, haraken wrote: > > Nit: I'd call all of these functions "add". Including addNull()? Also, I think this might produce an overloading issue for a use like builder.add("foo", 10); since that can convert both to bool and double. I could of course add int and unsigned overloads to solve that. But then, a caller meaning to set a boolean property using a calculated value will need to make sure the value expression's type is really bool, and not something that would convert fine to a bool in a boolean context, but is actually, say, an integer. So, in summary, I would probably rather not rely on C++ overload resolution to get the correct JS value type.
On 2015/03/24 14:44:44, haraken wrote: > On 2015/03/24 14:38:40, Jens Widell wrote: > > > https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... > > File Source/bindings/core/v8/V8ObjectBuilder.cpp (right): > > > > > https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... > > Source/bindings/core/v8/V8ObjectBuilder.cpp:61: > > m_object->ForceSet(m_isolate->GetCurrentContext(), v8String(m_isolate, name), > > value); > > On 2015/03/24 14:33:32, haraken wrote: > > > > > > Why do we use ForceSet, not Set? > > > > Web IDL uses [[DefineOwnProperty]] to describe ECMAScript serializers, for > which > > I think ForceSet() is a better match than Set(). Please correct me if I'm > wrong. > > > > Generally, [[DefineOwnProperty]] is better (than [[Put]]) for all cases where > > you simply want to build an object, so hopefully this will work for other > > use-cases besides serializers as well. > > ForceSet is for bypassing interceptors (see > https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&q=...) > and I think that's a different concept than [[DefineOwnProperty]]. It's for "bypassing interceptors and overriding accessors or read-only properties", according to the comment accompanying ForceSet(). The bit about interceptors ought to be irrelevant in this case, but accessors and read-only properties (on Object.prototype, specifically) should not affect the outcome, and they do if I use Set() instead of ForceSet().
On 2015/03/24 14:51:46, Jens Widell wrote: > https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... > File Source/bindings/core/v8/V8ObjectBuilder.h (right): > > https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... > Source/bindings/core/v8/V8ObjectBuilder.h:29: V8ObjectBuilder& addString(String > name, String value); > On 2015/03/24 14:33:33, haraken wrote: > > > > Nit: I'd call all of these functions "add". > > Including addNull()? > > Also, I think this might produce an overloading issue for a use like > > builder.add("foo", 10); > > since that can convert both to bool and double. I could of course add int and > unsigned overloads to solve that. > > But then, a caller meaning to set a boolean property using a calculated value > will need to make sure the value expression's type is really bool, and not > something that would convert fine to a bool in a boolean context, but is > actually, say, an integer. > > So, in summary, I would probably rather not rely on C++ overload resolution to > get the correct JS value type. Sounds reasonable :)
On 2015/03/24 15:03:10, Jens Widell wrote: > On 2015/03/24 14:44:44, haraken wrote: > > On 2015/03/24 14:38:40, Jens Widell wrote: > > > > > > https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... > > > File Source/bindings/core/v8/V8ObjectBuilder.cpp (right): > > > > > > > > > https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... > > > Source/bindings/core/v8/V8ObjectBuilder.cpp:61: > > > m_object->ForceSet(m_isolate->GetCurrentContext(), v8String(m_isolate, > name), > > > value); > > > On 2015/03/24 14:33:32, haraken wrote: > > > > > > > > Why do we use ForceSet, not Set? > > > > > > Web IDL uses [[DefineOwnProperty]] to describe ECMAScript serializers, for > > which > > > I think ForceSet() is a better match than Set(). Please correct me if I'm > > wrong. > > > > > > Generally, [[DefineOwnProperty]] is better (than [[Put]]) for all cases > where > > > you simply want to build an object, so hopefully this will work for other > > > use-cases besides serializers as well. > > > > ForceSet is for bypassing interceptors (see > > > https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&q=...) > > and I think that's a different concept than [[DefineOwnProperty]]. > > It's for "bypassing interceptors and overriding accessors or read-only > properties", according to the comment accompanying ForceSet(). > > The bit about interceptors ought to be irrelevant in this case, but accessors > and read-only properties (on Object.prototype, specifically) should not affect > the outcome, and they do if I use Set() instead of ForceSet(). Ah, you're right. Since V8ObjectBuilder::add() wants to set a given property unconditionally, ForceSet() sounds more reasonable than Set(). That said, I'm not fully convinced that [[DefineOwnProperty]] is equivalent to ForceSet(). Would you elaborate on it?
https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... File Source/bindings/core/v8/V8ObjectBuilder.cpp (right): https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... Source/bindings/core/v8/V8ObjectBuilder.cpp:20: return ScriptValue(ScriptState::current(m_isolate), m_object); On 2015/03/24 14:33:32, haraken wrote: > > Using ScriptState::current is not nice, since the ScriptState::current depends > on what ScriptState the caller side is using. (For security reasons, it is very > important that we always use a correct ScriptState.) > > I think we can pass in ScriptState to the V8ObjectBuilder, cache the ScriptState > on V8ObjectBuilder::m_scriptState, and use it here. Done. The constructor now takes (and stores) a ScriptState instead of an isolate, and uses that. https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... Source/bindings/core/v8/V8ObjectBuilder.cpp:61: m_object->ForceSet(m_isolate->GetCurrentContext(), v8String(m_isolate, name), value); On 2015/03/24 14:33:33, haraken wrote: > > Also we should use m_scriptState->context(). Done. https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... File Source/bindings/core/v8/V8ObjectBuilder.h (right): https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... Source/bindings/core/v8/V8ObjectBuilder.h:15: class V8ObjectBuilder { On 2015/03/24 14:33:33, haraken wrote: > > Can we add STACK_ALLOCATED? Done. https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... Source/bindings/core/v8/V8ObjectBuilder.h:17: V8ObjectBuilder(v8::Isolate*); On 2015/03/24 14:33:33, haraken wrote: > > Add explicit. Done.
LGTM (but I'm curious about the relationship between [[DefineOwnProperty]] and ForceSet().) https://codereview.chromium.org/1031783003/diff/20001/Source/bindings/core/v8... File Source/bindings/core/v8/V8ObjectBuilder.h (right): https://codereview.chromium.org/1031783003/diff/20001/Source/bindings/core/v8... Source/bindings/core/v8/V8ObjectBuilder.h:8: #include "platform/heap/Heap.h" Heap.h => Handle.h (We normally include Handle.h.)
On 2015/03/24 15:13:52, haraken wrote: > On 2015/03/24 15:03:10, Jens Widell wrote: > > On 2015/03/24 14:44:44, haraken wrote: > > > On 2015/03/24 14:38:40, Jens Widell wrote: > > > > > > > > > > https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... > > > > File Source/bindings/core/v8/V8ObjectBuilder.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... > > > > Source/bindings/core/v8/V8ObjectBuilder.cpp:61: > > > > m_object->ForceSet(m_isolate->GetCurrentContext(), v8String(m_isolate, > > name), > > > > value); > > > > On 2015/03/24 14:33:32, haraken wrote: > > > > > > > > > > Why do we use ForceSet, not Set? > > > > > > > > Web IDL uses [[DefineOwnProperty]] to describe ECMAScript serializers, for > > > which > > > > I think ForceSet() is a better match than Set(). Please correct me if I'm > > > wrong. > > > > > > > > Generally, [[DefineOwnProperty]] is better (than [[Put]]) for all cases > > where > > > > you simply want to build an object, so hopefully this will work for other > > > > use-cases besides serializers as well. > > > > > > ForceSet is for bypassing interceptors (see > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&q=...) > > > and I think that's a different concept than [[DefineOwnProperty]]. > > > > It's for "bypassing interceptors and overriding accessors or read-only > > properties", according to the comment accompanying ForceSet(). > > > > The bit about interceptors ought to be irrelevant in this case, but accessors > > and read-only properties (on Object.prototype, specifically) should not affect > > the outcome, and they do if I use Set() instead of ForceSet(). > > Ah, you're right. Since V8ObjectBuilder::add() wants to set a given property > unconditionally, ForceSet() sounds more reasonable than Set(). > > That said, I'm not fully convinced that [[DefineOwnProperty]] is equivalent to > ForceSet(). Would you elaborate on it? I'm not convinced it is, or is meant to be, equivalent either. It just seems to be more so than Set(). It's clear to me that Set() gives the wrong behavior under some set of circumstances. I'm not aware of ForceSet() leading to the wrong behavior under some set of circumstances, and it certainly gives the correct behavior when Set() gives the wrong behavior.
On 2015/03/24 15:33:48, Jens Widell wrote: > On 2015/03/24 15:13:52, haraken wrote: > > On 2015/03/24 15:03:10, Jens Widell wrote: > > > On 2015/03/24 14:44:44, haraken wrote: > > > > On 2015/03/24 14:38:40, Jens Widell wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... > > > > > File Source/bindings/core/v8/V8ObjectBuilder.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... > > > > > Source/bindings/core/v8/V8ObjectBuilder.cpp:61: > > > > > m_object->ForceSet(m_isolate->GetCurrentContext(), v8String(m_isolate, > > > name), > > > > > value); > > > > > On 2015/03/24 14:33:32, haraken wrote: > > > > > > > > > > > > Why do we use ForceSet, not Set? > > > > > > > > > > Web IDL uses [[DefineOwnProperty]] to describe ECMAScript serializers, > for > > > > which > > > > > I think ForceSet() is a better match than Set(). Please correct me if > I'm > > > > wrong. > > > > > > > > > > Generally, [[DefineOwnProperty]] is better (than [[Put]]) for all cases > > > where > > > > > you simply want to build an object, so hopefully this will work for > other > > > > > use-cases besides serializers as well. > > > > > > > > ForceSet is for bypassing interceptors (see > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&q=...) > > > > and I think that's a different concept than [[DefineOwnProperty]]. > > > > > > It's for "bypassing interceptors and overriding accessors or read-only > > > properties", according to the comment accompanying ForceSet(). > > > > > > The bit about interceptors ought to be irrelevant in this case, but > accessors > > > and read-only properties (on Object.prototype, specifically) should not > affect > > > the outcome, and they do if I use Set() instead of ForceSet(). > > > > Ah, you're right. Since V8ObjectBuilder::add() wants to set a given property > > unconditionally, ForceSet() sounds more reasonable than Set(). > > > > That said, I'm not fully convinced that [[DefineOwnProperty]] is equivalent to > > ForceSet(). Would you elaborate on it? > > I'm not convinced it is, or is meant to be, equivalent either. It just seems to > be more so than Set(). > > It's clear to me that Set() gives the wrong behavior under some set of > circumstances. I'm not aware of ForceSet() leading to the wrong behavior under > some set of circumstances, and it certainly gives the correct behavior when > Set() gives the wrong behavior. Yeah, I'm on the same page with you.
https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... File Source/bindings/core/v8/V8ObjectBuilder.h (right): https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... Source/bindings/core/v8/V8ObjectBuilder.h:15: class V8ObjectBuilder { On 2015/03/24 15:16:29, Jens Widell wrote: > On 2015/03/24 14:33:33, haraken wrote: > > > > Can we add STACK_ALLOCATED? > > Done. Actually this was a bit problematic. (I didn't notice since I don't build with clang locally.) V8ObjectBuilder is used as a member in DictionaryBuilder (CryptoKey.cpp) which then needs STACK_ALLOCATED() too (I guess). But DictionaryBuilder inherits WebCryptoKeyAlgorithmDictionary (public/platform/WebCryptoAlgorithmParams.h) which I'm assuming we don't/can't add STACK_ALLOCATED() to. Okay to drop STACK_ALLOCATED()?
On 2015/03/24 16:22:37, Jens Widell wrote: > https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... > File Source/bindings/core/v8/V8ObjectBuilder.h (right): > > https://codereview.chromium.org/1031783003/diff/1/Source/bindings/core/v8/V8O... > Source/bindings/core/v8/V8ObjectBuilder.h:15: class V8ObjectBuilder { > On 2015/03/24 15:16:29, Jens Widell wrote: > > On 2015/03/24 14:33:33, haraken wrote: > > > > > > Can we add STACK_ALLOCATED? > > > > Done. > > Actually this was a bit problematic. (I didn't notice since I don't build with > clang locally.) > > V8ObjectBuilder is used as a member in DictionaryBuilder (CryptoKey.cpp) which > then needs STACK_ALLOCATED() too (I guess). But DictionaryBuilder inherits > WebCryptoKeyAlgorithmDictionary (public/platform/WebCryptoAlgorithmParams.h) > which I'm assuming we don't/can't add STACK_ALLOCATED() to. > > Okay to drop STACK_ALLOCATED()? Ah, this is the issue bashi-san faced before... V8ObjectBuilder must be guaranteed to be used on stack, but we cannot use STACK_ALLOCATED because it is held by an object (e.g., Dictionary) which might be put in a collection. bashi-san, any thought?
LGTM On 2015/03/24 16:42:53, haraken wrote: > Ah, this is the issue bashi-san faced before... V8ObjectBuilder must be > guaranteed to be used on stack, but we cannot use STACK_ALLOCATED because it is > held by an object (e.g., Dictionary) which might be put in a collection. > > bashi-san, any thought? How about passing a stack allocated V8ObjectBuilder to DictionaryBuilder as a reference? Since we don't provide a way to reset the internal v8 object in V8ObjectBuilder, it would be good to make V8ObjectBuilder STACK_ALLOCATED(). But this will add one extra line in CryptoKey::algorithm() so I think either is fine.
https://codereview.chromium.org/1031783003/diff/100001/Source/bindings/core/v... File Source/bindings/core/v8/V8ObjectBuilder.cpp (right): https://codereview.chromium.org/1031783003/diff/100001/Source/bindings/core/v... Source/bindings/core/v8/V8ObjectBuilder.cpp:61: m_object->ForceSet(m_scriptState->context(), v8String(m_scriptState->isolate(), name), value); Just a note: In the future, we might want to provide a way to ensure that all addXXX() succeed. void V8ObjectBuilder::add(...) { m_errorOccurred = m_object->ForceSet(...).IsEmpty() || m_errorOccurred; } bool V8ObjectBuilder::errorOccurred() { return m_errorOccurred; } And return an empty value on failure when v8Value() called. https://codereview.chromium.org/1031783003/diff/100001/Source/bindings/core/v... File Source/bindings/core/v8/V8ObjectBuilder.h (right): https://codereview.chromium.org/1031783003/diff/100001/Source/bindings/core/v... Source/bindings/core/v8/V8ObjectBuilder.h:16: class V8ObjectBuilder { nit: Add final https://codereview.chromium.org/1031783003/diff/100001/Source/modules/crypto/... File Source/modules/crypto/CryptoKey.cpp (right): https://codereview.chromium.org/1031783003/diff/100001/Source/modules/crypto/... Source/modules/crypto/CryptoKey.cpp:129: m_builder.add(propertyName, ScriptValue::from(m_builder.scriptState(), DOMUint8Array::create(vector.data(), vector.size()))); IMHO, having addUint8Array() in V8ObjectBuilder would make sense. WDYT?
On 2015/03/25 01:09:14, bashi1 wrote: > How about passing a stack allocated V8ObjectBuilder to DictionaryBuilder as a > reference? Since we don't provide a way to reset the internal v8 object in > V8ObjectBuilder, it would be good to make V8ObjectBuilder STACK_ALLOCATED(). But > this will add one extra line in CryptoKey::algorithm() so I think either is > fine. Yes, that works. I've done that now. Turned out quite reasonable in CryptoKey.cpp as well, so I'm happy with it. https://codereview.chromium.org/1031783003/diff/100001/Source/bindings/core/v... File Source/bindings/core/v8/V8ObjectBuilder.cpp (right): https://codereview.chromium.org/1031783003/diff/100001/Source/bindings/core/v... Source/bindings/core/v8/V8ObjectBuilder.cpp:61: m_object->ForceSet(m_scriptState->context(), v8String(m_scriptState->isolate(), name), value); On 2015/03/25 01:09:25, bashi1 wrote: > Just a note: In the future, we might want to provide a way to ensure that all > addXXX() succeed. > > void V8ObjectBuilder::add(...) { > m_errorOccurred = m_object->ForceSet(...).IsEmpty() || m_errorOccurred; > } > > bool V8ObjectBuilder::errorOccurred() { return m_errorOccurred; } > > And return an empty value on failure when v8Value() called. I implemented this with a slight variation: letting empty m_object mean "has failed" (in which case no further ForceSet()s are performed) and clearing m_object upon failure. Failure to allocate m_object in the first place is thus automatically detected as an error too, and an empty value is returned in the end if anything has failed. https://codereview.chromium.org/1031783003/diff/100001/Source/bindings/core/v... File Source/bindings/core/v8/V8ObjectBuilder.h (right): https://codereview.chromium.org/1031783003/diff/100001/Source/bindings/core/v... Source/bindings/core/v8/V8ObjectBuilder.h:16: class V8ObjectBuilder { On 2015/03/25 01:09:25, bashi1 wrote: > nit: Add final Done. https://codereview.chromium.org/1031783003/diff/100001/Source/modules/crypto/... File Source/modules/crypto/CryptoKey.cpp (right): https://codereview.chromium.org/1031783003/diff/100001/Source/modules/crypto/... Source/modules/crypto/CryptoKey.cpp:129: m_builder.add(propertyName, ScriptValue::from(m_builder.scriptState(), DOMUint8Array::create(vector.data(), vector.size()))); On 2015/03/25 01:09:25, bashi1 wrote: > IMHO, having addUint8Array() in V8ObjectBuilder would make sense. WDYT? I'm not sure. Supporting generic values (e.g. ScriptValue) covers this (and many other potential use-cases), and seems less arbitrary than explicitly supporting the one "complex" type we currently need. We could add template <typename T> V8ObjectBuilder add(String name, const T& value) { add(name, toV8(value, ...); } of course.
https://codereview.chromium.org/1031783003/diff/100001/Source/modules/crypto/... File Source/modules/crypto/CryptoKey.cpp (right): https://codereview.chromium.org/1031783003/diff/100001/Source/modules/crypto/... Source/modules/crypto/CryptoKey.cpp:129: m_builder.add(propertyName, ScriptValue::from(m_builder.scriptState(), DOMUint8Array::create(vector.data(), vector.size()))); On 2015/03/25 11:16:04, Jens Widell wrote: > On 2015/03/25 01:09:25, bashi1 wrote: > > IMHO, having addUint8Array() in V8ObjectBuilder would make sense. WDYT? > > I'm not sure. Supporting generic values (e.g. ScriptValue) covers this (and many > other potential use-cases), and seems less arbitrary than explicitly supporting > the one "complex" type we currently need. > > We could add > > template <typename T> > V8ObjectBuilder add(String name, const T& value) > { > add(name, toV8(value, ...); > } > > of course. Generic add() added now, and the explicit ScriptValue variant dropped (since the generic version handles ScriptValue).
LGTM. Thanks!
vivekg@chromium.org changed reviewers: + vivekg@chromium.org
https://codereview.chromium.org/1031783003/diff/180001/Source/bindings/core/v... File Source/bindings/core/v8/V8ObjectBuilder.h (right): https://codereview.chromium.org/1031783003/diff/180001/Source/bindings/core/v... Source/bindings/core/v8/V8ObjectBuilder.h:25: V8ObjectBuilder& add(String name, const V8ObjectBuilder&); nit: Can we use `const String&` here and subsequent places?
On 2015/03/26 03:46:50, vivekg_ wrote: > https://codereview.chromium.org/1031783003/diff/180001/Source/bindings/core/v... > File Source/bindings/core/v8/V8ObjectBuilder.h (right): > > https://codereview.chromium.org/1031783003/diff/180001/Source/bindings/core/v... > Source/bindings/core/v8/V8ObjectBuilder.h:25: V8ObjectBuilder& add(String name, > const V8ObjectBuilder&); > nit: Can we use `const String&` here and subsequent places? I don't know the reason, but Blink normally doesn't use const String&.
On 2015/03/26 at 03:57:24, haraken wrote: > On 2015/03/26 03:46:50, vivekg_ wrote: > > https://codereview.chromium.org/1031783003/diff/180001/Source/bindings/core/v... > > File Source/bindings/core/v8/V8ObjectBuilder.h (right): > > > > https://codereview.chromium.org/1031783003/diff/180001/Source/bindings/core/v... > > Source/bindings/core/v8/V8ObjectBuilder.h:25: V8ObjectBuilder& add(String name, > > const V8ObjectBuilder&); > > nit: Can we use `const String&` here and subsequent places? > > I don't know the reason, but Blink normally doesn't use const String&. Though I don't have strong opinion about using const version, I think its mixed approach used across blink code base. As per [1] there is high usage of const String& usage as well :) [1] https://code.google.com/p/chromium/codesearch#search/&q=%5C(.*const%5C%20Stri...
On 2015/03/26 04:01:49, vivekg_ wrote: > On 2015/03/26 at 03:57:24, haraken wrote: > > On 2015/03/26 03:46:50, vivekg_ wrote: > > > > https://codereview.chromium.org/1031783003/diff/180001/Source/bindings/core/v... > > > File Source/bindings/core/v8/V8ObjectBuilder.h (right): > > > > > > > https://codereview.chromium.org/1031783003/diff/180001/Source/bindings/core/v... > > > Source/bindings/core/v8/V8ObjectBuilder.h:25: V8ObjectBuilder& add(String > name, > > > const V8ObjectBuilder&); > > > nit: Can we use `const String&` here and subsequent places? > > > > I don't know the reason, but Blink normally doesn't use const String&. > > Though I don't have strong opinion about using const version, I think its mixed > approach used across blink code base. > As per [1] there is high usage of const String& usage as well :) > > [1] > https://code.google.com/p/chromium/codesearch#search/&q=%5C(.*const%5C%20Stri... String objects are effectively pointers, so passing them by reference is more work than passing them by value. OTOH, there's reference counting involved, so passing them by reference is also less work than passing them by value. In total, I doubt it really matters much either way. But it seems 'const String&' is actually more common across the code base, so I switched to using that.
The CQ bit was checked by jl@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, bashi@chromium.org Link to the patchset: https://codereview.chromium.org/1031783003/#ps200001 (title: "use "const String&"")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1031783003/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://src.chromium.org/viewvc/blink?view=rev&revision=192599 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
