Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2265603002:
Blink-compatible serialization of Boolean, Number and String objects. (Closed)
|
Created:
4 years, 4 months ago by jbroman Modified:
4 years, 4 months ago Reviewers:
Jakob Kummerow CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@vs-date Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionBlink-compatible serialization of Boolean, Number and String objects.
BUG=chromium:148757
Committed: https://crrev.com/4dce969078270e14e89276128d0493a0c65c354a
Cr-Commit-Position: refs/heads/master@{#38830}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Correct GC safety error (NewNumber could invalidate pointer, depending on evaluation order) #
Total comments: 10
Patch Set 3 : UNREACHABLE #Patch Set 4 : remove HasSpecificClassOf #
Messages
Total messages: 24 (16 generated)
The CQ bit was checked by jbroman@chromium.org 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...
jbroman@chromium.org changed reviewers: + jkummerow@chromium.org
https://codereview.chromium.org/2265603002/diff/1/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2265603002/diff/1/src/value-serializer.cc#new... src/value-serializer.cc:151: if (!bytes) return nullptr; This sometimes triggered the debug std::vector checks (it complains when trying to compute the address of &buffer_[old_size] if that's at the end of the vector). Returning nullptr is fine, since the caller has promised to write at most zero bytes.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/11203) v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)
The CQ bit was checked by jbroman@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with comments. https://codereview.chromium.org/2265603002/diff/20001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2265603002/diff/20001/src/value-serializer.cc... src/value-serializer.cc:378: if (value->HasSpecificClassOf(isolate_->heap()->Boolean_string())) { Just: if(inner_value->IsBoolean()) In fact, you can drop this, and just hoist the nested conditions to the top level: if (inner_value->IsTrue(isolate_)) { ... } else if (inner_value->IsFalse(isolate_)) { ... } else if (inner_value->IsNumber()) { ... } else if (inner_value->IsString()) { ... } else { DCHECK(inner_value->IsSymbol()); return Nothing<bool>(); } https://codereview.chromium.org/2265603002/diff/20001/src/value-serializer.cc... src/value-serializer.cc:767: return MaybeHandle<JSValue>(); nit: please add an UNREACHABLE(); before returning. https://codereview.chromium.org/2265603002/diff/20001/test/unittests/value-se... File test/unittests/value-serializer-unittest.cc (right): https://codereview.chromium.org/2265603002/diff/20001/test/unittests/value-se... test/unittests/value-serializer-unittest.cc:1190: InvalidEncodeTest("Object.valueOf.apply(Symbol())"); Why not just "Symbol()"?
https://codereview.chromium.org/2265603002/diff/20001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2265603002/diff/20001/src/value-serializer.cc... src/value-serializer.cc:378: if (value->HasSpecificClassOf(isolate_->heap()->Boolean_string())) { On 2016/08/22 at 17:42:21, Jakob wrote: > Just: if(inner_value->IsBoolean()) > > In fact, you can drop this, and just hoist the nested conditions to the top level: > > if (inner_value->IsTrue(isolate_)) { > ... > } else if (inner_value->IsFalse(isolate_)) { > ... > } else if (inner_value->IsNumber()) { > ... > } else if (inner_value->IsString()) { > ... > } else { > DCHECK(inner_value->IsSymbol()); > return Nothing<bool>(); > } A previous version of this CL did just that, but this is how the API methods check (IsBooleanObject, etc), and I was concerned about other JS_VALUE_TYPE objects, like OpaqueReference, which might possibly wrap a primitive value for some other purpose. I'm happy to change this back if you're confident that it's correct. https://codereview.chromium.org/2265603002/diff/20001/src/value-serializer.cc... src/value-serializer.cc:378: if (value->HasSpecificClassOf(isolate_->heap()->Boolean_string())) { On 2016/08/22 at 17:42:21, Jakob wrote: > Just: if(inner_value->IsBoolean()) > > In fact, you can drop this, and just hoist the nested conditions to the top level: > > if (inner_value->IsTrue(isolate_)) { > ... > } else if (inner_value->IsFalse(isolate_)) { > ... > } else if (inner_value->IsNumber()) { > ... > } else if (inner_value->IsString()) { > ... > } else { > DCHECK(inner_value->IsSymbol()); > return Nothing<bool>(); > } A previous version of this CL did just that, but this is how the API methods check (IsBooleanObject, etc), and I was concerned about other JS_VALUE_TYPE objects, like OpaqueReference, which might possibly wrap a primitive value for some other purpose. I'm happy to change this back if you tell me that it's correct. https://codereview.chromium.org/2265603002/diff/20001/src/value-serializer.cc... src/value-serializer.cc:767: return MaybeHandle<JSValue>(); On 2016/08/22 at 17:42:21, Jakob wrote: > nit: please add an UNREACHABLE(); before returning. Done. https://codereview.chromium.org/2265603002/diff/20001/test/unittests/value-se... File test/unittests/value-serializer-unittest.cc (right): https://codereview.chromium.org/2265603002/diff/20001/test/unittests/value-se... test/unittests/value-serializer-unittest.cc:1190: InvalidEncodeTest("Object.valueOf.apply(Symbol())"); On 2016/08/22 at 17:42:22, Jakob wrote: > Why not just "Symbol()"? Symbol() isn't an object; it's a value of type 'symbol'. I wanted an object wrapper for a symbol (i.e., an object that is an instance of Symbol). Unlike Boolean, Number and String, the constructor cannot be directly used. Conceptually, this is "new Symbol(Symbol())".
The CQ bit was checked by jbroman@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2265603002/diff/20001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2265603002/diff/20001/src/value-serializer.cc... src/value-serializer.cc:378: if (value->HasSpecificClassOf(isolate_->heap()->Boolean_string())) { On 2016/08/22 19:02:32, jbroman wrote: > On 2016/08/22 at 17:42:21, Jakob wrote: > > Just: if(inner_value->IsBoolean()) > > > > In fact, you can drop this, and just hoist the nested conditions to the top > level: > > > > if (inner_value->IsTrue(isolate_)) { > > ... > > } else if (inner_value->IsFalse(isolate_)) { > > ... > > } else if (inner_value->IsNumber()) { > > ... > > } else if (inner_value->IsString()) { > > ... > > } else { > > DCHECK(inner_value->IsSymbol()); > > return Nothing<bool>(); > > } > > A previous version of this CL did just that, but this is how the API methods > check (IsBooleanObject, etc), and I was concerned about other JS_VALUE_TYPE > objects, like OpaqueReference, which might possibly wrap a primitive value for > some other purpose. > > I'm happy to change this back if you're confident that it's correct. I'm reasonably confident, yes :-) There shouldn't be any other JSValues floating around, and there are other places relying on that, e.g. https://chromium.googlesource.com/v8/v8/+/master/src/objects.cc#15925 . https://codereview.chromium.org/2265603002/diff/20001/test/unittests/value-se... File test/unittests/value-serializer-unittest.cc (right): https://codereview.chromium.org/2265603002/diff/20001/test/unittests/value-se... test/unittests/value-serializer-unittest.cc:1190: InvalidEncodeTest("Object.valueOf.apply(Symbol())"); On 2016/08/22 19:02:33, jbroman wrote: > On 2016/08/22 at 17:42:22, Jakob wrote: > > Why not just "Symbol()"? > > Symbol() isn't an object; it's a value of type 'symbol'. I wanted an object > wrapper for a symbol (i.e., an object that is an instance of Symbol). Unlike > Boolean, Number and String, the constructor cannot be directly used. > Conceptually, this is "new Symbol(Symbol())". Acknowledged.
The CQ bit was checked by jbroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkummerow@chromium.org Link to the patchset: https://codereview.chromium.org/2265603002/#ps60001 (title: "remove HasSpecificClassOf")
https://codereview.chromium.org/2265603002/diff/20001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2265603002/diff/20001/src/value-serializer.cc... src/value-serializer.cc:378: if (value->HasSpecificClassOf(isolate_->heap()->Boolean_string())) { On 2016/08/23 at 09:43:14, Jakob wrote: > On 2016/08/22 19:02:32, jbroman wrote: > > On 2016/08/22 at 17:42:21, Jakob wrote: > > > Just: if(inner_value->IsBoolean()) > > > > > > In fact, you can drop this, and just hoist the nested conditions to the top > > level: > > > > > > if (inner_value->IsTrue(isolate_)) { > > > ... > > > } else if (inner_value->IsFalse(isolate_)) { > > > ... > > > } else if (inner_value->IsNumber()) { > > > ... > > > } else if (inner_value->IsString()) { > > > ... > > > } else { > > > DCHECK(inner_value->IsSymbol()); > > > return Nothing<bool>(); > > > } > > > > A previous version of this CL did just that, but this is how the API methods > > check (IsBooleanObject, etc), and I was concerned about other JS_VALUE_TYPE > > objects, like OpaqueReference, which might possibly wrap a primitive value for > > some other purpose. > > > > I'm happy to change this back if you're confident that it's correct. > > I'm reasonably confident, yes :-) > There shouldn't be any other JSValues floating around, and there are other places relying on that, e.g. https://chromium.googlesource.com/v8/v8/+/master/src/objects.cc#15925 . Ah, fantastic. Done. :D
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 #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Blink-compatible serialization of Boolean, Number and String objects. BUG=chromium:148757 ========== to ========== Blink-compatible serialization of Boolean, Number and String objects. BUG=chromium:148757 Committed: https://crrev.com/4dce969078270e14e89276128d0493a0c65c354a Cr-Commit-Position: refs/heads/master@{#38830} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4dce969078270e14e89276128d0493a0c65c354a Cr-Commit-Position: refs/heads/master@{#38830} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
