Mojo: more idiomatic C++ bindings

Mojo: more idiomatic C++ bindings

This change eliminates AllocationScope in favor of heap allocating the "wrapper" classes, which are now no longer wrappers in the true sense but rather deep copies of the archived structs. We still use the term "wrapper" in the code generator.

With this design, the fact that structs are encoded as pointers (that may be null) is more apparent. For example, a Foo struct may be allocated and stored in a FooPtr. The FooPtr implements move-only semantics. This is because some of the members of Foo may be move-only (e.g., handles).

Strings are now just thin wrappers around std::string that impose nullability and a more restrictive API. It is not possible to mutate the elements of a string after it has been created. String acts like a pointer to a possibly null array of characters, which is very similar to a char*.

Arrays are now just thin wrappers around std::vector that impose nullability and a more restrictive API. Unlike String, Array does support mutation of its elements after creation, but there is no support for resizing an array. (We can add support for push_back and resize if needed.)

BUG=365922
R=yzshen@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273527

[sky, 2014-05-22 14:12:04]

https://codereview.chromium.org/294833002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc (right):

https://codereview.chromium.org/294833002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:40:
AutocompleteAdditionalInfoPtr item(AutocompleteAdditionalInfo::New());
It's too bad the empty constructor can't create the new for me, eg:
AutocompleteAdditionalInfoPtr item;
But I get why this is undesirable in other cases.

https://codereview.chromium.org/294833002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:41: item->key =
mojo::String::From(i->first);
I like the old in this case. How come mojo::String::From is needed now?

[darin (slow to review), 2014-05-22 14:26:38]

Yeah, I went with FooPtr() being null since it represents a pointer
variable. I went with Foo::New rather than "new Foo" to allow control over
how the Foo gets allocated.

I should be able to add a String::operator= that supports assignment from a
fixed set of types: std::string and const char*. I can't support arbitrary
conversions without interfering with the operator=(RValue) needed to
support .Pass().
On May 22, 2014 7:12 AM, <sky@chromium.org> wrote:

>
> https://codereview.chromium.org/294833002/diff/60001/
> chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc
> File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc (right):
>
> https://codereview.chromium.org/294833002/diff/60001/
> chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc#newcode40
> chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:40:
> AutocompleteAdditionalInfoPtr item(AutocompleteAdditionalInfo::New());
> It's too bad the empty constructor can't create the new for me, eg:
> AutocompleteAdditionalInfoPtr item;
> But I get why this is undesirable in other cases.
>
> https://codereview.chromium.org/294833002/diff/60001/
> chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc#newcode41
> chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:41: item->key =
> mojo::String::From(i->first);
> I like the old in this case. How come mojo::String::From is needed now?
>
> https://codereview.chromium.org/294833002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2014-05-22 17:13:52]

Here are a few comments. I will need to read and think in more details...

Thanks!

https://codereview.chromium.org/294833002/diff/60001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/array.h (right):

https://codereview.chromium.org/294833002/diff/60001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/array.h:32: explicit Array(size_t size) : vec_(size),
is_null_(true) {
nit: is_null_(false)?

https://codereview.chromium.org/294833002/diff/60001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/array_internal.h (right):

https://codereview.chromium.org/294833002/diff/60001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/array_internal.h:369: new (vec->at(i).buf) T();
Is there potential alignment issue here?

One alternative is to wrap move-only objects with something like SharedData<>.
Although there is some some storage overhead, it seems less hacky to me.
Besides, those methods of Array returning "const std::vector<StorageType>&" make
more sense this way.

https://codereview.chromium.org/294833002/diff/60001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/string.h (right):

https://codereview.chromium.org/294833002/diff/60001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/string.h:28: explicit String(const char chars[N])
I think this is probably not needed, we could rely on string(const char* chars)
to handle the String("literal") case.

https://codereview.chromium.org/294833002/diff/60001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl (right):

https://codereview.chromium.org/294833002/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl:48: typedef
mojo::InlinedStructPtr<{{struct.name}}> {{struct.name}}Ptr;
It seems users always create/use FooPtr instead of Foo directly.
If that is true, does it make sense to change the names:
Foo -> Foo_Data
FooPtr -> Foo

Pros:
- It seems less verbose to the users.
- It is more consistent with the mojom definition: if user defines struct Foo{}
in the mojom file, he/she will use a cpp class called Foo.
- It is more consistent with Array and String, because they are not called
ArrayPtr and StringPtr.

What do you think?

[darin (slow to review), 2014-05-22 19:49:24]

On Thu, May 22, 2014 at 10:13 AM, <yzshen@chromium.org> wrote:

> Here are a few comments. I will need to read and think in more details...
>
> Thanks!
>
>
> https://codereview.chromium.org/294833002/diff/60001/mojo/
> public/cpp/bindings/array.h
> File mojo/public/cpp/bindings/array.h (right):
>
> https://codereview.chromium.org/294833002/diff/60001/mojo/
> public/cpp/bindings/array.h#newcode32
> mojo/public/cpp/bindings/array.h:32: explicit Array(size_t size) :
> vec_(size), is_null_(true) {
> nit: is_null_(false)?
>

Good catch


>
> https://codereview.chromium.org/294833002/diff/60001/mojo/
> public/cpp/bindings/lib/array_internal.h
> File mojo/public/cpp/bindings/lib/array_internal.h (right):
>
> https://codereview.chromium.org/294833002/diff/60001/mojo/
> public/cpp/bindings/lib/array_internal.h#newcode369
> mojo/public/cpp/bindings/lib/array_internal.h:369: new (vec->at(i).buf)
> T();
> Is there potential alignment issue here?
>
> One alternative is to wrap move-only objects with something like
> SharedData<>. Although there is some some storage overhead, it seems
> less hacky to me. Besides, those methods of Array returning "const
> std::vector<StorageType>&" make more sense this way.
>

Hmm... I was hoping to avoid reference counting. Good catch about the
alignment issue. I will need to add some padding.


> https://codereview.chromium.org/294833002/diff/60001/mojo/
> public/cpp/bindings/string.h
> File mojo/public/cpp/bindings/string.h (right):
>
> https://codereview.chromium.org/294833002/diff/60001/mojo/
> public/cpp/bindings/string.h#newcode28
> mojo/public/cpp/bindings/string.h:28: explicit String(const char
> chars[N])
> I think this is probably not needed, we could rely on string(const char*
> chars) to handle the String("literal") case.
>

This is just an optimization to avoid having to call strlen.



>
> https://codereview.chromium.org/294833002/diff/60001/mojo/
> public/tools/bindings/generators/cpp_templates/module.h.tmpl
> File mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl
> (right):
>
> https://codereview.chromium.org/294833002/diff/60001/mojo/
> public/tools/bindings/generators/cpp_templates/module.h.tmpl#newcode48
> mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl:48:
> typedef mojo::InlinedStructPtr<{{struct.name}}> {{struct.name}}Ptr;
> It seems users always create/use FooPtr instead of Foo directly.
> If that is true, does it make sense to change the names:
> Foo -> Foo_Data
> FooPtr -> Foo
>
> Pros:
> - It seems less verbose to the users.
> - It is more consistent with the mojom definition: if user defines
> struct Foo{} in the mojom file, he/she will use a cpp class called Foo.
> - It is more consistent with Array and String, because they are not
> called ArrayPtr and StringPtr.
>

The "Ptr" suffix helps relay the fact that these are treated like pointers
(i.e., you have to use operator-> to access members). Hopefully it'll be
thought of like scoped_ptr<T> to users.

Array and String don't need the Ptr suffix because they kind of already are
pointers. You think of arrays and strings as things that you can use
operator[] on.

I agree though that shorter names would be a plus. I just worry that when
you see Foo, you expect to use the "." operator to access members.

-Darin


>
> What do you think?
>
> https://codereview.chromium.org/294833002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2014-05-22 19:49:59]

On Thu, May 22, 2014 at 12:49 PM, Darin Fisher <darin@chromium.org> wrote:

>
>
>
> On Thu, May 22, 2014 at 10:13 AM, <yzshen@chromium.org> wrote:
>
>> Here are a few comments. I will need to read and think in more details...
>>
>> Thanks!
>>
>>
>> https://codereview.chromium.org/294833002/diff/60001/mojo/
>> public/cpp/bindings/array.h
>> File mojo/public/cpp/bindings/array.h (right):
>>
>> https://codereview.chromium.org/294833002/diff/60001/mojo/
>> public/cpp/bindings/array.h#newcode32
>> mojo/public/cpp/bindings/array.h:32: explicit Array(size_t size) :
>> vec_(size), is_null_(true) {
>> nit: is_null_(false)?
>>
>
> Good catch
>
>
>>
>> https://codereview.chromium.org/294833002/diff/60001/mojo/
>> public/cpp/bindings/lib/array_internal.h
>> File mojo/public/cpp/bindings/lib/array_internal.h (right):
>>
>> https://codereview.chromium.org/294833002/diff/60001/mojo/
>> public/cpp/bindings/lib/array_internal.h#newcode369
>> mojo/public/cpp/bindings/lib/array_internal.h:369: new (vec->at(i).buf)
>> T();
>> Is there potential alignment issue here?
>>
>> One alternative is to wrap move-only objects with something like
>> SharedData<>. Although there is some some storage overhead, it seems
>> less hacky to me. Besides, those methods of Array returning "const
>> std::vector<StorageType>&" make more sense this way.
>>
>
> Hmm... I was hoping to avoid reference counting. Good catch about the
> alignment issue. I will need to add some padding.
>
>
>> https://codereview.chromium.org/294833002/diff/60001/mojo/
>> public/cpp/bindings/string.h
>> File mojo/public/cpp/bindings/string.h (right):
>>
>> https://codereview.chromium.org/294833002/diff/60001/mojo/
>> public/cpp/bindings/string.h#newcode28
>> mojo/public/cpp/bindings/string.h:28: explicit String(const char
>> chars[N])
>> I think this is probably not needed, we could rely on string(const char*
>> chars) to handle the String("literal") case.
>>
>
> This is just an optimization to avoid having to call strlen.
>
>
>
>>
>> https://codereview.chromium.org/294833002/diff/60001/mojo/
>> public/tools/bindings/generators/cpp_templates/module.h.tmpl
>> File mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl
>> (right):
>>
>> https://codereview.chromium.org/294833002/diff/60001/mojo/
>> public/tools/bindings/generators/cpp_templates/module.h.tmpl#newcode48
>> mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl:48:
>> typedef mojo::InlinedStructPtr<{{struct.name}}> {{struct.name}}Ptr;
>> It seems users always create/use FooPtr instead of Foo directly.
>> If that is true, does it make sense to change the names:
>> Foo -> Foo_Data
>> FooPtr -> Foo
>>
>> Pros:
>> - It seems less verbose to the users.
>> - It is more consistent with the mojom definition: if user defines
>> struct Foo{} in the mojom file, he/she will use a cpp class called Foo.
>> - It is more consistent with Array and String, because they are not
>> called ArrayPtr and StringPtr.
>>
>
> The "Ptr" suffix helps relay the fact that these are treated like pointers
> (i.e., you have to use operator-> to access members). Hopefully it'll be
> thought of like scoped_ptr<T> to users.
>
> Array and String don't need the Ptr suffix because they kind of already
> are pointers. You think of arrays and strings as things that you can use
> operator[] on.
>
> I agree though that shorter names would be a plus. I just worry that when
> you see Foo, you expect to use the "." operator to access members.
>

Also, FooPtr for structs is similar to InterfacePtr<Interface> and the
associated typedefs.


>
> -Darin
>
>
>>
>> What do you think?
>>
>> https://codereview.chromium.org/294833002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2014-05-23 06:08:32]

Latest WIP patch makes mojo::String no longer have move semantics. This allows
for implicit assignment and construction from a std::string, const char*, etc. I
didn't enable it for string16 or other inputs that require a TypeConverter.
Perhaps folks should have to think before they use such conversions.

[yzshen1, 2014-05-23 17:45:04]

I like the new String!

https://codereview.chromium.org/294833002/diff/140001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/array_internal.h (right):

https://codereview.chromium.org/294833002/diff/140001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_internal.h:363: char buf[sizeof(T) + (8 -
(sizeof(T) % 8)) % 8];  // Make 8-byte aligned.
What I was trying to ask is that with |vec| of type std::vector<StorageType>,
whether it is guaranteed that |vec[0].buf| is aligned properly for type T. (If
that is true, I think sizeof(StorgeType) == sizeof(T) is sufficient to make
|vec[x].buf| aligned properly.)

After some thoughts, I think it should be safe: because we use the default
allocator for std::vector, the memory is dynamic allocated so |vec[0].buf|
should be 
aligned for any T. :)

[darin (slow to review), 2014-05-23 17:50:02]

But sizeof(T) might not be a multiple of 8. This change tries to ensure
that subsequent elements are aligned.
On May 23, 2014 10:45 AM, <yzshen@chromium.org> wrote:

> I like the new String!
>
>
> https://codereview.chromium.org/294833002/diff/140001/
> mojo/public/cpp/bindings/lib/array_internal.h
> File mojo/public/cpp/bindings/lib/array_internal.h (right):
>
> https://codereview.chromium.org/294833002/diff/140001/
> mojo/public/cpp/bindings/lib/array_internal.h#newcode363
> mojo/public/cpp/bindings/lib/array_internal.h:363: char buf[sizeof(T) +
> (8 - (sizeof(T) % 8)) % 8];  // Make 8-byte aligned.
> What I was trying to ask is that with |vec| of type
> std::vector<StorageType>, whether it is guaranteed that |vec[0].buf| is
> aligned properly for type T. (If that is true, I think
> sizeof(StorgeType) == sizeof(T) is sufficient to make |vec[x].buf|
> aligned properly.)
>
> After some thoughts, I think it should be safe: because we use the
> default allocator for std::vector, the memory is dynamic allocated so
> |vec[0].buf| should be
> aligned for any T. :)
>
> https://codereview.chromium.org/294833002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-05-23 17:56:54]

On Fri, May 23, 2014 at 10:50 AM, Darin Fisher <darin@chromium.org> wrote:

> But sizeof(T) might not be a multiple of 8. This change tries to ensure
> that subsequent elements are aligned.
>
Struct T itself is padded to make sure that each element of a T array is
properly aligned.
Therefore, I think sizeof(StorageType) == sizeof(T) is sufficient.

Are you concerned that the following scenario is possible?
  struct StorageType {
    char buf[sizeof(T)];
  };
and
  sizeof(StorageType) != sizeof(T)



On May 23, 2014 10:45 AM, <yzshen@chromium.org> wrote:
>
>> I like the new String!
>>
>>
>> https://codereview.chromium.org/294833002/diff/140001/
>> mojo/public/cpp/bindings/lib/array_internal.h
>> File mojo/public/cpp/bindings/lib/array_internal.h (right):
>>
>> https://codereview.chromium.org/294833002/diff/140001/
>> mojo/public/cpp/bindings/lib/array_internal.h#newcode363
>> mojo/public/cpp/bindings/lib/array_internal.h:363: char buf[sizeof(T) +
>> (8 - (sizeof(T) % 8)) % 8];  // Make 8-byte aligned.
>> What I was trying to ask is that with |vec| of type
>> std::vector<StorageType>, whether it is guaranteed that |vec[0].buf| is
>> aligned properly for type T. (If that is true, I think
>> sizeof(StorgeType) == sizeof(T) is sufficient to make |vec[x].buf|
>> aligned properly.)
>>
>> After some thoughts, I think it should be safe: because we use the
>> default allocator for std::vector, the memory is dynamic allocated so
>> |vec[0].buf| should be
>> aligned for any T. :)
>>
>> https://codereview.chromium.org/294833002/
>>
>


-- 
Best regards,
Yuzhu Shen.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2014-05-23 18:56:05]

The wrapper classes (i.e the "structs" held in this array are not padded
out).
On May 23, 2014 10:56 AM, "Yuzhu Shen" <yzshen@google.com> wrote:

> On Fri, May 23, 2014 at 10:50 AM, Darin Fisher <darin@chromium.org> wrote:
>
>> But sizeof(T) might not be a multiple of 8. This change tries to ensure
>> that subsequent elements are aligned.
>>
> Struct T itself is padded to make sure that each element of a T array is
> properly aligned.
> Therefore, I think sizeof(StorageType) == sizeof(T) is sufficient.
>
> Are you concerned that the following scenario is possible?
>   struct StorageType {
>     char buf[sizeof(T)];
>   };
> and
>   sizeof(StorageType) != sizeof(T)
>
>
>
>  On May 23, 2014 10:45 AM, <yzshen@chromium.org> wrote:
>>
>>> I like the new String!
>>>
>>>
>>> https://codereview.chromium.org/294833002/diff/140001/
>>> mojo/public/cpp/bindings/lib/array_internal.h
>>> File mojo/public/cpp/bindings/lib/array_internal.h (right):
>>>
>>> https://codereview.chromium.org/294833002/diff/140001/
>>> mojo/public/cpp/bindings/lib/array_internal.h#newcode363
>>> mojo/public/cpp/bindings/lib/array_internal.h:363: char buf[sizeof(T) +
>>> (8 - (sizeof(T) % 8)) % 8];  // Make 8-byte aligned.
>>> What I was trying to ask is that with |vec| of type
>>> std::vector<StorageType>, whether it is guaranteed that |vec[0].buf| is
>>> aligned properly for type T. (If that is true, I think
>>> sizeof(StorgeType) == sizeof(T) is sufficient to make |vec[x].buf|
>>> aligned properly.)
>>>
>>> After some thoughts, I think it should be safe: because we use the
>>> default allocator for std::vector, the memory is dynamic allocated so
>>> |vec[0].buf| should be
>>> aligned for any T. :)
>>>
>>> https://codereview.chromium.org/294833002/
>>>
>>
>
>
> --
> Best regards,
> Yuzhu Shen.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2014-05-27 05:15:43]

OK, I think this is ready for review. PTAL, thanks! (Sorry for the big CL.)

See delta from patchset #1 in https://codereview.chromium.org/293393009/ to see
how sample_service.mojom{-internal.h,.h,.cc} are affected by this CL.

[yzshen1, 2014-05-27 22:16:58]

(Sorry if any of the comments doesn't make sense. Pretty difficult to read
through the CL. :))

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:139:
Deserialize_(input->at(i), &element);
nit: I think we could directly pass in &result[i].

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:166: Array<E>* output) {
Maybe we could consider checking whether |input| is NULL in this function.

I can see that in some places we check NULL pointer before passing it into
Deserialize_(), however, we don't do that in some other places such as
DeserializeElements() above. Maybe we should make it consistent and move the
NULL check into Deserialize_() (for array/struct/string).

(And we could do similar thing for Serialize_(). Currently we don't consistently
check is_null() before calling Serialize_() either.)

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/bindings_internal.h (right):

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bindings_internal.h:72: template <> struct
IsHandle<DataPipeConsumerHandle> {
It might make sense to define IsHandle<> with the help of something like
IsBaseOf<>:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/output_t...

That way we don't have to worry about updating this list when new subclasses of
Handle are added.

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/callback_internal.h (right):

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/callback_internal.h:16: template <typename T, bool
is_move_only = IsMoveOnlyType<T>::value>
It seems we don't need the second template parameter.

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/string_serialization.cc (right):

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/string_serialization.cc:9: #include
"mojo/public/cpp/bindings/lib/array_internal.h"
nit: the .h file has already included it.

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/string_serialization.cc:19: internal::String_Data*
result = internal::String_Data::New(input.size(), buf);
Maybe we need to check whether input is null and set |output| to NULL directly?

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/template_util.h (right):

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/template_util.h:12: struct integral_constant {
style nit: Shall we use IntegralConstant / kValue / ValueType / Type? (And same
style nit for some other classes below.)

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/string.h (right):

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/string.h:17: class String {
I think you mentioned that you would like to add operator=(const std::string&)?

Without it I think the following code still works, but does an extra copy,
right?
(construct a String object from a, and then do default assignment)
=======================
std::string a;
String mojo_a;
mojo_a = a;
=======================

(And it might make sense to add assignment operator for const char* / const
char[N], too.)

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/string.h:126: return input ? String(input) : String();
Do we want the null-handling behavior of String(const char* input) to be
different from String::From(const char* input)?
That seems a little confusing to me.

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/struct_ptr.h (right):

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/struct_ptr.h:18: class StructHelper {
I assume we will extend StructHelper to have more complex behavior later?
It doesn't seem very useful at this point.

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/string_unittest.cc (right):

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/string_unittest.cc:10: namespace {
nit: please remove this empty namespace.

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/type_converter.h (right):

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/type_converter.h:24: //     // Maybe (mutually
exclusive):
Please remove all comments about the macros.

https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/type_converter.h:114: template <typename T> class
TypeConverter<T, T> {
This probably won't work for move only types like our StructPtr?

https://codereview.chromium.org/294833002/diff/340001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl (right):

https://codereview.chromium.org/294833002/diff/340001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl:58: {%     
include "wrapper_class_declaration.tmpl" %}
Why do line 49 and line 58 need to be in different loops?

https://codereview.chromium.org/294833002/diff/340001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl
(right):

https://codereview.chromium.org/294833002/diff/340001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl:2:
size_t size = mojo::internal::Align(sizeof(internal::{{struct.name}}_Data));
nit: I think the size of *_Data is always 8-byte multiples so no need to call
Align().

https://codereview.chromium.org/294833002/diff/340001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/pylib/mojom/generate/generator.py (right):

https://codereview.chromium.org/294833002/diff/340001/mojo/public/tools/bindi...
mojo/public/tools/bindings/pylib/mojom/generate/generator.py:53: return
IsObjectKind(kind) or IsHandleKind(kind) or IsInterfaceKind(kind)
nit: I think IsInterfaceKind() is a subset of IsHandleKind().

[darin (slow to review), 2014-05-28 03:48:46]

On Tue, May 27, 2014 at 3:16 PM, <yzshen@chromium.org> wrote:

> (Sorry if any of the comments doesn't make sense. Pretty difficult to read
> through the CL. :))
>
>
Thanks for the feedback! I really appreciate the detailed review.



>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/lib/array_serialization.h
> File mojo/public/cpp/bindings/lib/array_serialization.h (right):
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/lib/array_serialization.h#newcode139
> mojo/public/cpp/bindings/lib/array_serialization.h:139:
> Deserialize_(input->at(i), &element);
> nit: I think we could directly pass in &result[i].
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/lib/array_serialization.h#newcode166
> mojo/public/cpp/bindings/lib/array_serialization.h:166: Array<E>*
> output) {
> Maybe we could consider checking whether |input| is NULL in this
> function.
>
> I can see that in some places we check NULL pointer before passing it
> into Deserialize_(), however, we don't do that in some other places such
> as DeserializeElements() above. Maybe we should make it consistent and
> move the NULL check into Deserialize_() (for array/struct/string).
>
> (And we could do similar thing for Serialize_(). Currently we don't
> consistently check is_null() before calling Serialize_() either.)
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/lib/bindings_internal.h
> File mojo/public/cpp/bindings/lib/bindings_internal.h (right):
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/lib/bindings_internal.h#newcode72
> mojo/public/cpp/bindings/lib/bindings_internal.h:72: template <> struct
> IsHandle<DataPipeConsumerHandle> {
> It might make sense to define IsHandle<> with the help of something like
> IsBaseOf<>:
> https://code.google.com/p/chromium/codesearch#chromium/
> src/ppapi/cpp/output_traits.h&l=39
>
> That way we don't have to worry about updating this list when new
> subclasses of Handle are added.
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/lib/callback_internal.h
> File mojo/public/cpp/bindings/lib/callback_internal.h (right):
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/lib/callback_internal.h#newcode16
> mojo/public/cpp/bindings/lib/callback_internal.h:16: template <typename
> T, bool is_move_only = IsMoveOnlyType<T>::value>
> It seems we don't need the second template parameter.
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/lib/string_serialization.cc
> File mojo/public/cpp/bindings/lib/string_serialization.cc (right):
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/lib/string_serialization.cc#newcode9
> mojo/public/cpp/bindings/lib/string_serialization.cc:9: #include
> "mojo/public/cpp/bindings/lib/array_internal.h"
> nit: the .h file has already included it.
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/lib/string_serialization.cc#newcode19
> mojo/public/cpp/bindings/lib/string_serialization.cc:19:
> internal::String_Data* result = internal::String_Data::New(input.size(),
> buf);
> Maybe we need to check whether input is null and set |output| to NULL
> directly?
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/lib/template_util.h
> File mojo/public/cpp/bindings/lib/template_util.h (right):
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/lib/template_util.h#newcode12
> mojo/public/cpp/bindings/lib/template_util.h:12: struct
> integral_constant {
> style nit: Shall we use IntegralConstant / kValue / ValueType / Type?
> (And same style nit for some other classes below.)
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/string.h
> File mojo/public/cpp/bindings/string.h (right):
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/string.h#newcode17
> mojo/public/cpp/bindings/string.h:17: class String {
> I think you mentioned that you would like to add operator=(const
> std::string&)?
>
> Without it I think the following code still works, but does an extra
> copy, right?
> (construct a String object from a, and then do default assignment)
> =======================
> std::string a;
> String mojo_a;
> mojo_a = a;
> =======================
>
> (And it might make sense to add assignment operator for const char* /
> const char[N], too.)
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/string.h#newcode126
> mojo/public/cpp/bindings/string.h:126: return input ? String(input) :
> String();
> Do we want the null-handling behavior of String(const char* input) to be
> different from String::From(const char* input)?
> That seems a little confusing to me.
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/struct_ptr.h
> File mojo/public/cpp/bindings/struct_ptr.h (right):
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/struct_ptr.h#newcode18
> mojo/public/cpp/bindings/struct_ptr.h:18: class StructHelper {
> I assume we will extend StructHelper to have more complex behavior
> later?
> It doesn't seem very useful at this point.
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/tests/string_unittest.cc
> File mojo/public/cpp/bindings/tests/string_unittest.cc (right):
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/tests/string_unittest.cc#newcode10
> mojo/public/cpp/bindings/tests/string_unittest.cc:10: namespace {
> nit: please remove this empty namespace.
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/type_converter.h
> File mojo/public/cpp/bindings/type_converter.h (right):
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/type_converter.h#newcode24
> mojo/public/cpp/bindings/type_converter.h:24: //     // Maybe (mutually
> exclusive):
> Please remove all comments about the macros.
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/cpp/bindings/type_converter.h#newcode114
> mojo/public/cpp/bindings/type_converter.h:114: template <typename T>
> class TypeConverter<T, T> {
> This probably won't work for move only types like our StructPtr?
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl
> File mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl
> (right):
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/tools/bindings/generators/cpp_templates/
> module.h.tmpl#newcode58
> mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl:58: {%
>      include "wrapper_class_declaration.tmpl" %}
> Why do line 49 and line 58 need to be in different loops?
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_
> definition.tmpl
> File
> mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_
> definition.tmpl
> (right):
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_
> definition.tmpl#newcode2
> mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_
> definition.tmpl:2:
> size_t size =
> mojo::internal::Align(sizeof(internal::{{struct.name}}_Data));
> nit: I think the size of *_Data is always 8-byte multiples so no need to
> call Align().
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/tools/bindings/pylib/mojom/generate/generator.py
> File mojo/public/tools/bindings/pylib/mojom/generate/generator.py
> (right):
>
> https://codereview.chromium.org/294833002/diff/340001/
> mojo/public/tools/bindings/pylib/mojom/generate/generator.py#newcode53
> mojo/public/tools/bindings/pylib/mojom/generate/generator.py:53: return
> IsObjectKind(kind) or IsHandleKind(kind) or IsInterfaceKind(kind)
> nit: I think IsInterfaceKind() is a subset of IsHandleKind().
>
> https://codereview.chromium.org/294833002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2014-05-28 19:04:05]

On 2014/05/27 22:16:58, yzshen1 wrote:
...
>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/lib/array_serialization.h (right):
> 
>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/lib/array_serialization.h:139:
> Deserialize_(input->at(i), &element);
> nit: I think we could directly pass in &result[i].

Done.

>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/lib/array_serialization.h:166: Array<E>* output) {
> Maybe we could consider checking whether |input| is NULL in this function.
> 
> I can see that in some places we check NULL pointer before passing it into
> Deserialize_(), however, we don't do that in some other places such as
> DeserializeElements() above. Maybe we should make it consistent and move the
> NULL check into Deserialize_() (for array/struct/string).
> 
> (And we could do similar thing for Serialize_(). Currently we don't
consistently
> check is_null() before calling Serialize_() either.)

Good idea. Done.


>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/lib/bindings_internal.h (right):
> 
>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/lib/bindings_internal.h:72: template <> struct
> IsHandle<DataPipeConsumerHandle> {
> It might make sense to define IsHandle<> with the help of something like
> IsBaseOf<>:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/output_t...
> 
> That way we don't have to worry about updating this list when new subclasses
of
> Handle are added.

Very nice!


>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/lib/callback_internal.h (right):
> 
>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/lib/callback_internal.h:16: template <typename T,
bool
> is_move_only = IsMoveOnlyType<T>::value>
> It seems we don't need the second template parameter.

Indeed!


>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/lib/string_serialization.cc (right):
> 
>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/lib/string_serialization.cc:9: #include
> "mojo/public/cpp/bindings/lib/array_internal.h"
> nit: the .h file has already included it.

Thanks.


>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/lib/string_serialization.cc:19:
internal::String_Data*
> result = internal::String_Data::New(input.size(), buf);
> Maybe we need to check whether input is null and set |output| to NULL
directly?

Done.


>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/lib/template_util.h (right):
> 
>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/lib/template_util.h:12: struct integral_constant {
> style nit: Shall we use IntegralConstant / kValue / ValueType / Type? (And
same
> style nit for some other classes below.)

I went with MixedCase but kept "value" for the member. I notice that Pepper does
the
same with IsBaseOf. Ditto for similar templates in src/base/.


>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/string.h (right):
> 
>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/string.h:17: class String {
> I think you mentioned that you would like to add operator=(const
std::string&)?
> 
> Without it I think the following code still works, but does an extra copy,
> right?
> (construct a String object from a, and then do default assignment)
> =======================
> std::string a;
> String mojo_a;
> mojo_a = a;
> =======================
> 
> (And it might make sense to add assignment operator for const char* / const
> char[N], too.)

Added for std::string and const char*. Doesn't seem to work for const char[N]
though.


>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/string.h:126: return input ? String(input) :
String();
> Do we want the null-handling behavior of String(const char* input) to be
> different from String::From(const char* input)?
> That seems a little confusing to me.

I was torn on this, but I'm swayed by the fact that you noticed and thought
that it might be confusing. std::string(NULL) is not valid, so I thought that
I'd make mojo::String(NULL) invalid as well. I thought mojo::String::From(NULL)
could be valid since it is a more explicit conversion request. Plus it would
avoid a branch in the String(const char*) constructor. But, that is probably
too subtle of reasoning, so I've gone with allowing String(NULL).


>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/struct_ptr.h (right):
> 
>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/struct_ptr.h:18: class StructHelper {
> I assume we will extend StructHelper to have more complex behavior later?
> It doesn't seem very useful at this point.

StructHelper allows me to hide the Initialize method. It is not safe to call
Initialize more than once as it clobbers |ptr_| in the StructPtr case. I could
move StructHelper to a lib/struct_ptr_internal.h file so no one has to see it,
but it is just a small class :)


>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/tests/string_unittest.cc (right):
> 
>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/tests/string_unittest.cc:10: namespace {
> nit: please remove this empty namespace.

Oops, thanks.


>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/type_converter.h (right):
> 
>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/type_converter.h:24: //     // Maybe (mutually
> exclusive):
> Please remove all comments about the macros.

Oops, thanks.


>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/type_converter.h:114: template <typename T> class
> TypeConverter<T, T> {
> This probably won't work for move only types like our StructPtr?

Yeah. This specialization was in a view manager specific file. I moved
it here thinking it might be generally useful. It enables code like so:

  std::vector<uint32_t> values;
  Array<uint32_t>::From(values);


>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/tools/bindi...
> File mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl
(right):
> 
>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/tools/bindi...
> mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl:58: {%     
> include "wrapper_class_declaration.tmpl" %}
> Why do line 49 and line 58 need to be in different loops?

Structs can have pointers to other structs, and using InlinedStructPtr<T> as
a member of such a struct requires T to be fully defined so the size of
the InlinedStructPtr<T> member can be known.

I'll add a comment, and I think I'll also introduce a third loop. The first
loop will declare the typedefs. The second loop will define all of the to-be-
inlined structs. The third loop will define all of the non-inlined structs.
(A pre-requisite for being an inlined struct is that it has no pointers.)


>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/tools/bindi...
> File
>
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl
> (right):
> 
>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/tools/bindi...
>
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl:2:
> size_t size = mojo::internal::Align(sizeof(internal::{{struct.name}}_Data));
> nit: I think the size of *_Data is always 8-byte multiples so no need to call
> Align().

True. Good catch.


>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/tools/bindi...
> File mojo/public/tools/bindings/pylib/mojom/generate/generator.py (right):
> 
>
https://codereview.chromium.org/294833002/diff/340001/mojo/public/tools/bindi...
> mojo/public/tools/bindings/pylib/mojom/generate/generator.py:53: return
> IsObjectKind(kind) or IsHandleKind(kind) or IsInterfaceKind(kind)
> nit: I think IsInterfaceKind() is a subset of IsHandleKind().

Thanks.

[yzshen1, 2014-05-28 19:45:49]

LGTM with a few nits. Thanks!

https://codereview.chromium.org/294833002/diff/360001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://codereview.chromium.org/294833002/diff/360001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:88: if (!input)
It seems we need to move this check to GetSerializedSize_(). (Otherwise, we need
to do the same for the other ArraySerializer specializations, too.)

https://codereview.chromium.org/294833002/diff/360001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/template_util.h (right):

https://codereview.chromium.org/294833002/diff/360001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/template_util.h:63: // determine if a given class
is the base class of another given class. It is
nit: please remove the last sentence.

https://codereview.chromium.org/294833002/diff/360001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/string.h (right):

https://codereview.chromium.org/294833002/diff/360001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/string.h:49: is_null_ = false;
nit: shall we use is_null_ = !chars?

https://codereview.chromium.org/294833002/diff/360001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/type_converter.h (right):

https://codereview.chromium.org/294833002/diff/360001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/type_converter.h:63: //     geometry::Point output =
geometry::Point::From(pt);
nit: |output| type is geometry::PointPtr.

[yzshen1, 2014-05-28 19:45:50]

LGTM with a few nits. Thanks!

https://codereview.chromium.org/294833002/diff/360001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://codereview.chromium.org/294833002/diff/360001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:88: if (!input)
It seems we need to move this check to GetSerializedSize_(). (Otherwise, we need
to do the same for the other ArraySerializer specializations, too.)

https://codereview.chromium.org/294833002/diff/360001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/template_util.h (right):

https://codereview.chromium.org/294833002/diff/360001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/template_util.h:63: // determine if a given class
is the base class of another given class. It is
nit: please remove the last sentence.

https://codereview.chromium.org/294833002/diff/360001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/string.h (right):

https://codereview.chromium.org/294833002/diff/360001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/string.h:49: is_null_ = false;
nit: shall we use is_null_ = !chars?

https://codereview.chromium.org/294833002/diff/360001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/type_converter.h (right):

https://codereview.chromium.org/294833002/diff/360001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/type_converter.h:63: //     geometry::Point output =
geometry::Point::From(pt);
nit: |output| type is geometry::PointPtr.

[darin (slow to review), 2014-05-29 00:14:38]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2014-05-29 00:15:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/darin@chromium.org/294833002/380001

[commit-bot: I haz the power, 2014-05-29 05:01:07]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[darin (slow to review), 2014-05-29 17:59:21]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2014-05-29 18:01:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/darin@chromium.org/294833002/400001

[darin (slow to review), 2014-05-29 19:00:55]

Committed patchset #22 manually as r273527 (presubmit successful).