Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2228283002:
Make a deserializing function (Closed)
DescriptionMake a deserializing function
In the previous code, a conditional operator was used and {cpp_value}
was evaluated twice in the success case. This CL adds a separate function
in order to avoid evaluating it twice.
BUG=607215
Committed: https://crrev.com/400793e57acc5f8b315a27caf44edc1a362d7afc
Cr-Commit-Position: refs/heads/master@{#410995}
Patch Set 1 #
Total comments: 7
Patch Set 2 : addressed comments #
Messages
Total messages: 23 (7 generated)
lkawai@google.com changed reviewers: + bashi@chromium.org, haraken@chromium.org, peria@chromium.org, yukishiino@chromium.org
PTAL
LGTM. https://codereview.chromium.org/2228283002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h (right): https://codereview.chromium.org/2228283002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h:14: #include "bindings/core/v8/SerializedScriptValue.h" I'm not sure if it's possible or not, but could you try to use a forward declaration for SerializedScriptValue? i.e. Can you move this #include into .cpp file?
LGTM I'm not sure but is this the only place to evaluate methods twice in a operation? If you find more, please update them, too (in other CLs).
https://codereview.chromium.org/2228283002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h (right): https://codereview.chromium.org/2228283002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h:22: CORE_EXPORT v8::Local<v8::Value> v8Deserialize(v8::Isolate*, PassRefPtr<SerializedScriptValue>); I'm getting confused about what functions you're planning to move to GeneratedCodeHelper. value->deserialize() doesn't need to be limited to generated code (it's used by hand-written bindings as well). Also if you really want to create a helper function for this, I'd prefer creating toV8(PassRefPtr<SerializedScriptValue>) and put it in ToV8.h.
https://codereview.chromium.org/2228283002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h (right): https://codereview.chromium.org/2228283002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h:22: CORE_EXPORT v8::Local<v8::Value> v8Deserialize(v8::Isolate*, PassRefPtr<SerializedScriptValue>); On 2016/08/10 02:19:36, haraken wrote: > > I'm getting confused about what functions you're planning to move to > GeneratedCodeHelper. value->deserialize() doesn't need to be limited to > generated code (it's used by hand-written bindings as well). For hand-written code, value->deserialize() is preferred, and no need to use this helper function. This helper function is only needed for code generator to evaluate the value only once. > Also if you really want to create a helper function for this, I'd prefer > creating toV8(PassRefPtr<SerializedScriptValue>) and put it in ToV8.h. I'm on the fence with that idea. Serialization / deserialization are a pair concept. I think "deserialize" in the name is clearer. For example, v8SetReturnValue(v8Deserialize(serializedValue)); would make more sense than v8SetReturnValue(toV8(serializedValue)); The latter case is still understandable, but could be a bit more confusing.
https://codereview.chromium.org/2228283002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h (right): https://codereview.chromium.org/2228283002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h:22: CORE_EXPORT v8::Local<v8::Value> v8Deserialize(v8::Isolate*, PassRefPtr<SerializedScriptValue>); On 2016/08/10 02:19:36, haraken wrote: > > I'm getting confused about what functions you're planning to move to > GeneratedCodeHelper. value->deserialize() doesn't need to be limited to > generated code (it's used by hand-written bindings as well). The objective of CL is to avoid running needless deserialize(), not to wrap value->deserialize(). > Also if you really want to create a helper function for this, I'd prefer > creating toV8(PassRefPtr<SerializedScriptValue>) and put it in ToV8.h. So this suggestion makes sense to me.
https://codereview.chromium.org/2228283002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h (right): https://codereview.chromium.org/2228283002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h:22: CORE_EXPORT v8::Local<v8::Value> v8Deserialize(v8::Isolate*, PassRefPtr<SerializedScriptValue>); Note that toV8() is ideally supposed to satisfy the following things. - toV8(x) == toV8(x) : returns the same V8 object for the same |x| - does not make any side effect on the value |x| - just be a simple type conversion I know some of the actual implementation of toV8() violates some of them. But we never wanted to do so. I guess, deserialization itself is a bit out of scope of toV8().
On 2016/08/10 02:30:16, Yuki wrote: > https://codereview.chromium.org/2228283002/diff/1/third_party/WebKit/Source/b... > File third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h (right): > > https://codereview.chromium.org/2228283002/diff/1/third_party/WebKit/Source/b... > third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h:22: CORE_EXPORT > v8::Local<v8::Value> v8Deserialize(v8::Isolate*, > PassRefPtr<SerializedScriptValue>); > On 2016/08/10 02:19:36, haraken wrote: > > > > I'm getting confused about what functions you're planning to move to > > GeneratedCodeHelper. value->deserialize() doesn't need to be limited to > > generated code (it's used by hand-written bindings as well). > > For hand-written code, value->deserialize() is preferred, and no need to use > this helper function. > > This helper function is only needed for code generator to evaluate the value > only once. > > > > Also if you really want to create a helper function for this, I'd prefer > > creating toV8(PassRefPtr<SerializedScriptValue>) and put it in ToV8.h. > > I'm on the fence with that idea. Serialization / deserialization are a pair > concept. I think "deserialize" in the name is clearer. > > For example, > v8SetReturnValue(v8Deserialize(serializedValue)); > would make more sense than > v8SetReturnValue(toV8(serializedValue)); > > The latter case is still understandable, but could be a bit more confusing. Sounds reasonable, LGTM. (However, I still feel that this is a kind of refactoring whose benefit is not quite clear.)
On 2016/08/10 02:38:26, haraken wrote: > On 2016/08/10 02:30:16, Yuki wrote: > > > https://codereview.chromium.org/2228283002/diff/1/third_party/WebKit/Source/b... > > File third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h (right): > > > > > https://codereview.chromium.org/2228283002/diff/1/third_party/WebKit/Source/b... > > third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h:22: > CORE_EXPORT > > v8::Local<v8::Value> v8Deserialize(v8::Isolate*, > > PassRefPtr<SerializedScriptValue>); > > On 2016/08/10 02:19:36, haraken wrote: > > > > > > I'm getting confused about what functions you're planning to move to > > > GeneratedCodeHelper. value->deserialize() doesn't need to be limited to > > > generated code (it's used by hand-written bindings as well). > > > > For hand-written code, value->deserialize() is preferred, and no need to use > > this helper function. > > > > This helper function is only needed for code generator to evaluate the value > > only once. > > > > > > > Also if you really want to create a helper function for this, I'd prefer > > > creating toV8(PassRefPtr<SerializedScriptValue>) and put it in ToV8.h. > > > > I'm on the fence with that idea. Serialization / deserialization are a pair > > concept. I think "deserialize" in the name is clearer. > > > > For example, > > v8SetReturnValue(v8Deserialize(serializedValue)); > > would make more sense than > > v8SetReturnValue(toV8(serializedValue)); > > > > The latter case is still understandable, but could be a bit more confusing. > > Sounds reasonable, LGTM. > > (However, I still feel that this is a kind of refactoring whose benefit is not > quite clear.) FWIW, we have v8StringOrNull() in V8Binding.h.
On 2016/08/10 02:40:50, haraken wrote: > FWIW, we have v8StringOrNull() in V8Binding.h. And it's used only in bindings/core/v8/ScriptCustomElementDefinition.cpp ... https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... It's better to remove it.
PTAL
The CQ bit was checked by lkawai@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2228283002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h (right): https://codereview.chromium.org/2228283002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h:14: #include "bindings/core/v8/SerializedScriptValue.h" On 2016/08/10 02:16:34, Yuki wrote: > I'm not sure if it's possible or not, but could you try to use a forward > declaration for SerializedScriptValue? i.e. Can you move this #include into > .cpp file? Done. https://codereview.chromium.org/2228283002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h:22: CORE_EXPORT v8::Local<v8::Value> v8Deserialize(v8::Isolate*, PassRefPtr<SerializedScriptValue>); On 2016/08/10 02:19:36, haraken wrote: > > I'm getting confused about what functions you're planning to move to > GeneratedCodeHelper. value->deserialize() doesn't need to be limited to > generated code (it's used by hand-written bindings as well). > > Also if you really want to create a helper function for this, I'd prefer > creating toV8(PassRefPtr<SerializedScriptValue>) and put it in ToV8.h. Acknowledged.
On 2016/08/10 02:46:17, Yuki wrote: > On 2016/08/10 02:40:50, haraken wrote: > > FWIW, we have v8StringOrNull() in V8Binding.h. > > And it's used only in bindings/core/v8/ScriptCustomElementDefinition.cpp ... > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... > > It's better to remove it. We discussed the issue in https://codereview.chromium.org/2060753002/ and decided to introduce v8StringOrNull. So we should rather use v8StringOrNull in v8_types.py as well.
The CQ bit was unchecked by lkawai@google.com
The CQ bit was checked by lkawai@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, peria@chromium.org, yukishiino@chromium.org Link to the patchset: https://codereview.chromium.org/2228283002/#ps20001 (title: "addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from
==========
Make a deserializing function
In the previous code, a conditional operator was used and {cpp_value}
was evaluated twice in the success case. This CL adds a separate function
in order to avoid evaluating it twice.
BUG=607215
==========
to
==========
Make a deserializing function
In the previous code, a conditional operator was used and {cpp_value}
was evaluated twice in the success case. This CL adds a separate function
in order to avoid evaluating it twice.
BUG=607215
Committed: https://crrev.com/400793e57acc5f8b315a27caf44edc1a362d7afc
Cr-Commit-Position: refs/heads/master@{#410995}
==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/400793e57acc5f8b315a27caf44edc1a362d7afc Cr-Commit-Position: refs/heads/master@{#410995} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
