gin: Create a DataObjectBuilder class to help create simple data objects.

gin/data_object_builder.h

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef GIN_DATA_OBJECT_BUILDER_H_
+#define GIN_DATA_OBJECT_BUILDER_H_
+
+#include <utility>
+
+#include "base/strings/string_piece.h"
+#include "gin/converter.h"
+#include "gin/gin_export.h"
+#include "v8/include/v8.h"
+
+namespace gin {
+
+// Constructs a JavaScript object with a series of data properties.

[Devlin, 2017/04/26 15:50:31]

Optional: Maybe worth describing that the new property will be configurable,
writable, and enumerable?  (Maybe not - that might be the expectation)

[jbroman, 2017/04/26 17:47:31]

Done.

+// Values are automatically converted using gin::Converter.
+// This class avoids the pitfall of using Set, which may invoke setters on the

[Devlin, 2017/04/26 15:50:31]

nit: maybe specify v8::Object::Set()?

[jbroman, 2017/04/26 17:47:31]

Done.

+// object prototype.
+//
+// Expected usage:
+// v8::Local<v8::Object> object = gin::DataObjectBuilder(isolate)
+//     .Set("boolean", true)
+//     .Set("integer", 42)
+//     .Finish();
+//
+// Because this builder class contains local handles, callers must ensure it
+// does not outlive the scope in which it is created.
+class GIN_EXPORT DataObjectBuilder {
+ public:
+  explicit DataObjectBuilder(v8::Isolate* isolate);
+
+  template <typename T>
+  DataObjectBuilder& Set(base::StringPiece key, T&& value) {

[Devlin, 2017/04/26 15:50:31]

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?

[jbroman, 2017/04/26 17:47:31]

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.)

[Devlin, 2017/04/26 18:30:23]

Punting to callers sgtm.

+    v8::Local<v8::String> v8_key = StringToSymbol(isolate_, key);

[Devlin, 2017/04/26 15:50:31]

DCHECK(!object_.IsEmpty()) (enforcing single-use)?

[jbroman, 2017/04/26 17:47:31]

Could do. It will crash anyways when it's dereferenced below, so I didn't think
a DCHECK added much value.

[Devlin, 2017/04/26 18:30:23]

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.

[jbroman, 2017/04/27 15:26:30]

Done.

+    v8::Local<v8::Value> v8_value =
+        ConvertToV8(isolate_, std::forward<T>(value));
+    CHECK(object_->CreateDataProperty(context_, v8_key, v8_value).ToChecked());
+    return *this;

[Devlin, 2017/04/26 15:50:31]

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.

[jbroman, 2017/04/26 17:47:31]

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.)

+  }
+
+  v8::Local<v8::Object> Finish() {

[Devlin, 2017/04/26 15:50:31]

is it worth defining this inline?

[jbroman, 2017/04/26 17:47:31]

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.

+    v8::Local<v8::Object> result = object_;

[Devlin, 2017/04/26 15:50:31]

here, too, maybe DCHECK(!object_.IsEmpty())?

[jbroman, 2017/04/26 17:47:31]

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?

[Devlin, 2017/04/26 18:30:23]

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.

+    object_.Clear();
+    return result;
+  }
+
+ private:
+  v8::Isolate* isolate_;
+  v8::Local<v8::Context> context_;
+  v8::Local<v8::Object> object_;

[Devlin, 2017/04/26 15:50:32]

DISALLOW_COPY_AND_ASSIGN()

[jbroman, 2017/04/26 17:47:31]

Done.

+};
+
+}  // namespace gin
+
+#endif  // GIN_DATA_OBJECT_BUILDER_H_