Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2845463002:
gin: Create a DataObjectBuilder class to help create simple data objects. (Closed)
Descriptiongin: Create a DataObjectBuilder class to help create simple data objects.
It's easy to get this wrong and use Set instead, which can invoke author script
in unexpected places. This is a convenient helper that does the right thing,
similar to blink::V8ObjectBuilder.
A unit test and two "real-world" uses are included.
BUG=
Review-Url: https://codereview.chromium.org/2845463002
Cr-Commit-Position: refs/heads/master@{#467711}
Committed: https://chromium.googlesource.com/chromium/src/+/5967a866a003e060b471aa2686de87d5adb9bde4
Patch Set 1 #Patch Set 2 : skia benchmarking use #Patch Set 3 : data_object_builder.cc #
Total comments: 27
Patch Set 4 : devlin #
Total comments: 2
Patch Set 5 : devlin, jochen #
Messages
Total messages: 25 (17 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...
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 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.
jbroman@chromium.org changed reviewers: + jochen@chromium.org, rdevlin.cronin@chromium.org
Nice! A number of small nits/suggestions, but nothing major. Also, you know this code much better than I, so feel free to override them. :) lgtm https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder.cc File gin/data_object_builder.cc (right): https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.cc:11: context_(isolate->GetCurrentContext()), Would it be better to pass in the context and get the isolate from that, rather than the other way around? That way we have an explicit context for the SetDataProperty() calls. https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder.h File gin/data_object_builder.h (right): https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.h:17: // Constructs a JavaScript object with a series of data properties. Optional: Maybe worth describing that the new property will be configurable, writable, and enumerable? (Maybe not - that might be the expectation) https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.h:19: // This class avoids the pitfall of using Set, which may invoke setters on the nit: maybe specify v8::Object::Set()? https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.h:35: DataObjectBuilder& Set(base::StringPiece key, T&& value) { Note: this won't work for std::vectors or potentially objects relying on ToV8Traits(). I think that's probably fine, since the alternative is just passing in an already converted v8::Value, but it may be worth a comment? https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.h:36: v8::Local<v8::String> v8_key = StringToSymbol(isolate_, key); DCHECK(!object_.IsEmpty()) (enforcing single-use)? https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.h:40: return *this; over the course of <n> template specializations, this may or may not become nontrivial code size since it's always defined inline (and for each typename). We could limit it a bit by something like: template <typename T> DataObjectBuilder& Set(base::StringPiece key, T&& value) { return SetInternal(key, ConvertToV8(isolate_, std::forward<T>(value)); } DataObjectBuilder& SetInternal(base::StringPiece key, v8::Local<v8::Value> value); // .cc DataObjectBuilder& DataObjectBuilder::SetInternal(base::StringPiece key, v8::Local<v8::Value> value) { v8::Local<v8::String> v8_key = StringToSymbol(isolate_, key); CHECK(object_->CreateDataProperty(context_, v8_key, value).ToChecked()); return *this; } You probably have a better impression than I if this would ever matter, so I leave it up to you. https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.h:43: v8::Local<v8::Object> Finish() { is it worth defining this inline? https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.h:44: v8::Local<v8::Object> result = object_; here, too, maybe DCHECK(!object_.IsEmpty())? https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.h:52: v8::Local<v8::Object> object_; DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... File gin/data_object_builder_unittest.cc (right): https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder_unittest.cc:37: bool writable; nit: initialize (here and below) (I know that since we ASSERT below, we guard against any uninitialized reads, but it's just easier to not have uninitialized values. :)) https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder_unittest.cc:82: // after something may have modified the object in unexpected ways. Oh, interesting. I suggested this be a DCHECK failure. I'm fine with either.
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder.cc File gin/data_object_builder.cc (right): https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.cc:11: context_(isolate->GetCurrentContext()), On 2017/04/26 at 15:50:31, Devlin wrote: > Would it be better to pass in the context and get the isolate from that, rather than the other way around? That way we have an explicit context for the SetDataProperty() calls. It's a bit of a lie because v8::Object::New already uses the current context. To make that properly true we'd need a v8::Context::Scope, but the caller is in a good position to ensure that one is in place (and probably already has). I don't feel strongly, so I could budge on this if you feel strongly. https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder.h File gin/data_object_builder.h (right): https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.h:17: // Constructs a JavaScript object with a series of data properties. On 2017/04/26 at 15:50:31, Devlin wrote: > Optional: Maybe worth describing that the new property will be configurable, writable, and enumerable? (Maybe not - that might be the expectation) Done. https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.h:19: // This class avoids the pitfall of using Set, which may invoke setters on the On 2017/04/26 at 15:50:31, Devlin wrote: > nit: maybe specify v8::Object::Set()? Done. https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.h:35: DataObjectBuilder& Set(base::StringPiece key, T&& value) { On 2017/04/26 at 15:50:31, Devlin wrote: > Note: this won't work for std::vectors or potentially objects relying on ToV8Traits(). I think that's probably fine, since the alternative is just passing in an already converted v8::Value, but it may be worth a comment? Yeah, I waffled on this a little. It is easy enough for callers to do the conversion, and only then would they have to handle the failure. If I make this support it, I have to figure out what to do if the conversion fails (which may or may not throw an exception). Probably something like "subsequent calls are no-ops, and Finish returns an empty MaybeLocal", but it seemed easier to punt. I've added a comment, but we could do something more radical if you'd prefer. (jochen may well have an opinion, too.) https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.h:36: v8::Local<v8::String> v8_key = StringToSymbol(isolate_, key); On 2017/04/26 at 15:50:31, Devlin wrote: > DCHECK(!object_.IsEmpty()) (enforcing single-use)? Could do. It will crash anyways when it's dereferenced below, so I didn't think a DCHECK added much value. https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.h:40: return *this; On 2017/04/26 at 15:50:31, Devlin wrote: > over the course of <n> template specializations, this may or may not become nontrivial code size since it's always defined inline (and for each typename). We could limit it a bit by something like: > > template <typename T> > DataObjectBuilder& Set(base::StringPiece key, T&& value) { > return SetInternal(key, ConvertToV8(isolate_, std::forward<T>(value)); > } > > DataObjectBuilder& SetInternal(base::StringPiece key, v8::Local<v8::Value> value); > > // .cc > > DataObjectBuilder& DataObjectBuilder::SetInternal(base::StringPiece key, v8::Local<v8::Value> value) { > v8::Local<v8::String> v8_key = StringToSymbol(isolate_, key); > CHECK(object_->CreateDataProperty(context_, v8_key, value).ToChecked()); > return *this; > } > > You probably have a better impression than I if this would ever matter, so I leave it up to you. As you note the ConvertToV8 call still is inline, so we're replacing a two function calls and a CHECK branch with one function call. I figured that probably wasn't enough of a difference. (If I did out-of-line it, I'd probably make the internal one return void, so that the caller can trivially see that it returns *this.) https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.h:43: v8::Local<v8::Object> Finish() { On 2017/04/26 at 15:50:31, Devlin wrote: > is it worth defining this inline? Not needed. My Blink-side habits tend to favour inlining things that are simpler than the function call. This code amounts to: - move this+offset to a register - move zero to this+offset which is super simple and might as well be inline, but I also doubt this will be used at any call site where it really matters. https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.h:44: v8::Local<v8::Object> result = object_; On 2017/04/26 at 15:50:31, Devlin wrote: > here, too, maybe DCHECK(!object_.IsEmpty())? I could. It takes away my ability to test it (because it will crash, death tests are :(, and I don't know how to otherwise test it), but a DCHECK may be sufficient. WDYT? https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.h:52: v8::Local<v8::Object> object_; On 2017/04/26 at 15:50:32, Devlin wrote: > DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... File gin/data_object_builder_unittest.cc (right): https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder_unittest.cc:37: bool writable; On 2017/04/26 at 15:50:32, Devlin wrote: > nit: initialize (here and below) (I know that since we ASSERT below, we guard against any uninitialized reads, but it's just easier to not have uninitialized values. :)) Done. I kinda like this use as it strongly implies that the next line must initialize it, but I'm totally okay with changing. https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder_unittest.cc:82: // after something may have modified the object in unexpected ways. On 2017/04/26 at 15:50:32, Devlin wrote: > Oh, interesting. I suggested this be a DCHECK failure. I'm fine with either. (shrug) Replied above, let me know what you think.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder.cc File gin/data_object_builder.cc (right): https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.cc:11: context_(isolate->GetCurrentContext()), On 2017/04/26 17:47:31, jbroman wrote: > On 2017/04/26 at 15:50:31, Devlin wrote: > > Would it be better to pass in the context and get the isolate from that, > rather than the other way around? That way we have an explicit context for the > SetDataProperty() calls. > > It's a bit of a lie because v8::Object::New already uses the current context. To > make that properly true we'd need a v8::Context::Scope, but the caller is in a > good position to ensure that one is in place (and probably already has). > > I don't feel strongly, so I could budge on this if you feel strongly. Ah, good point. This is fine. On that note, naively, it seems to me we should have a version of v8::Object::New that takes a context, since most things that result in context-related happenings - getting/setting/executing - now seem to. jochen@, can you shed some light on why we don't do this? https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder.h File gin/data_object_builder.h (right): https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.h:35: DataObjectBuilder& Set(base::StringPiece key, T&& value) { On 2017/04/26 17:47:31, jbroman wrote: > On 2017/04/26 at 15:50:31, Devlin wrote: > > Note: this won't work for std::vectors or potentially objects relying on > ToV8Traits(). I think that's probably fine, since the alternative is just > passing in an already converted v8::Value, but it may be worth a comment? > > Yeah, I waffled on this a little. It is easy enough for callers to do the > conversion, and only then would they have to handle the failure. If I make this > support it, I have to figure out what to do if the conversion fails (which may > or may not throw an exception). Probably something like "subsequent calls are > no-ops, and Finish returns an empty MaybeLocal", but it seemed easier to punt. > > I've added a comment, but we could do something more radical if you'd prefer. > (jochen may well have an opinion, too.) Punting to callers sgtm. https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.h:36: v8::Local<v8::String> v8_key = StringToSymbol(isolate_, key); On 2017/04/26 17:47:31, jbroman wrote: > On 2017/04/26 at 15:50:31, Devlin wrote: > > DCHECK(!object_.IsEmpty()) (enforcing single-use)? > > Could do. It will crash anyways when it's dereferenced below, so I didn't think > a DCHECK added much value. DCHECK gives a nice stack trace. nullptr access gives a safe destruction. Since DCHECKs don't really hurt (aren't baked into release binaries), I think they can be useful to a) document usage and b) help the poor folks (usually me :P) that ends up tripping them. https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.h:44: v8::Local<v8::Object> result = object_; On 2017/04/26 17:47:31, jbroman wrote: > On 2017/04/26 at 15:50:31, Devlin wrote: > > here, too, maybe DCHECK(!object_.IsEmpty())? > > I could. It takes away my ability to test it (because it will crash, death tests > are :(, and I don't know how to otherwise test it), but a DCHECK may be > sufficient. WDYT? IMO, DCHECK is better than safely handling a result when you don't expect or desire anyone to ever exercise that behavior (like now), so I'd opt for that. But I don't feel super strongly. https://codereview.chromium.org/2845463002/diff/60001/gin/data_object_builder.h File gin/data_object_builder.h (right): https://codereview.chromium.org/2845463002/diff/60001/gin/data_object_builder... gin/data_object_builder.h:50: v8::Local<v8::Object> Finish() { nitty nit: most Builders seem to have a Build() rather than Finish(), so I'd opt for that. But I don't feel strongly at all.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
agree that Build() would be more in line than Finish() otherwise lgtm
https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder.h File gin/data_object_builder.h (right): https://codereview.chromium.org/2845463002/diff/40001/gin/data_object_builder... gin/data_object_builder.h:36: v8::Local<v8::String> v8_key = StringToSymbol(isolate_, key); On 2017/04/26 at 18:30:23, Devlin wrote: > On 2017/04/26 17:47:31, jbroman wrote: > > On 2017/04/26 at 15:50:31, Devlin wrote: > > > DCHECK(!object_.IsEmpty()) (enforcing single-use)? > > > > Could do. It will crash anyways when it's dereferenced below, so I didn't think > > a DCHECK added much value. > > DCHECK gives a nice stack trace. nullptr access gives a safe destruction. Since DCHECKs don't really hurt (aren't baked into release binaries), I think they can be useful to a) document usage and b) help the poor folks (usually me :P) that ends up tripping them. Done. https://codereview.chromium.org/2845463002/diff/60001/gin/data_object_builder.h File gin/data_object_builder.h (right): https://codereview.chromium.org/2845463002/diff/60001/gin/data_object_builder... gin/data_object_builder.h:50: v8::Local<v8::Object> Finish() { On 2017/04/26 at 18:30:24, Devlin wrote: > nitty nit: most Builders seem to have a Build() rather than Finish(), so I'd opt for that. But I don't feel strongly at all. OK, done. I agree that it's more usual, though it feels a little silly as this is the only step that doesn't build anything. :D
The CQ bit was checked by jbroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2845463002/#ps80001 (title: "devlin, jochen")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493306842578560,
"parent_rev": "d548cc04b6568f8edc08853940134707fcd6c455", "commit_rev":
"5967a866a003e060b471aa2686de87d5adb9bde4"}
Message was sent while issue was closed.
Description was changed from ========== gin: Create a DataObjectBuilder class to help create simple data objects. It's easy to get this wrong and use Set instead, which can invoke author script in unexpected places. This is a convenient helper that does the right thing, similar to blink::V8ObjectBuilder. A unit test and two "real-world" uses are included. BUG= ========== to ========== gin: Create a DataObjectBuilder class to help create simple data objects. It's easy to get this wrong and use Set instead, which can invoke author script in unexpected places. This is a convenient helper that does the right thing, similar to blink::V8ObjectBuilder. A unit test and two "real-world" uses are included. BUG= Review-Url: https://codereview.chromium.org/2845463002 Cr-Commit-Position: refs/heads/master@{#467711} Committed: https://chromium.googlesource.com/chromium/src/+/5967a866a003e060b471aa2686de... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/5967a866a003e060b471aa2686de... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
