[mojo] Use base::Pickle for Message storage.

[mojo] Use base::Pickle for Message storage.

This CL modifies the mojo::Message implementation so that
its storage is always backed by a base::Pickle.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. https://codereview.chromium.org/1520153002
  5. This CL
  6. https://codereview.chromium.org/1526533002
  7. https://codereview.chromium.org/1524703002

BUG=569669
Committed: https://crrev.com/0776a5005caf1c15e92ccf93546236737f935d0c
Cr-Commit-Position: refs/heads/master@{#365766}

[Ken Rockot(use gerrit already), 2015-12-12 04:50:09]

Description was changed from

==========
[mojo] Use base::Pickle for Message storage.

This CL modifies the mojo::Message implementation so that
its storage is always backed by a base::Pickle.

Part of a series of changes to reuse existing serialization
traits with new Mojo bindings:

  1. This CL
  2.
  3.
  4.

BUG=
==========

to

==========
[mojo] Use base::Pickle for Message storage.

This CL modifies the mojo::Message implementation so that
its storage is always backed by a base::Pickle.

Part of a series of changes to reuse existing serialization
traits with new Mojo bindings:

  1. This CL
  2. https://codereview.chromium.org/1515423002
  3. https://codereview.chromium.org/1517043004
  4. https://codereview.chromium.org/1518293002

BUG=
==========

[Ken Rockot(use gerrit already), 2015-12-14 07:03:24]

Description was changed from

==========
[mojo] Use base::Pickle for Message storage.

This CL modifies the mojo::Message implementation so that
its storage is always backed by a base::Pickle.

Part of a series of changes to reuse existing serialization
traits with new Mojo bindings:

  1. This CL
  2. https://codereview.chromium.org/1515423002
  3. https://codereview.chromium.org/1517043004
  4. https://codereview.chromium.org/1518293002

BUG=
==========

to

==========
[mojo] Use base::Pickle for Message storage.

This CL modifies the mojo::Message implementation so that
its storage is always backed by a base::Pickle.

Part of a series of changes to support custom mojom serialization:

  1. This CL
  2. https://codereview.chromium.org/1515423002
  3. https://codereview.chromium.org/1517043004
  4. https://codereview.chromium.org/1524693002
  5. https://codereview.chromium.org/1520153002
  6. https://codereview.chromium.org/1526533002
  7. https://codereview.chromium.org/1524703002

BUG=
==========

[Ken Rockot(use gerrit already), 2015-12-14 20:19:36]

Description was changed from

==========
[mojo] Use base::Pickle for Message storage.

This CL modifies the mojo::Message implementation so that
its storage is always backed by a base::Pickle.

Part of a series of changes to support custom mojom serialization:

  1. This CL
  2. https://codereview.chromium.org/1515423002
  3. https://codereview.chromium.org/1517043004
  4. https://codereview.chromium.org/1524693002
  5. https://codereview.chromium.org/1520153002
  6. https://codereview.chromium.org/1526533002
  7. https://codereview.chromium.org/1524703002

BUG=
==========

to

==========
[mojo] Use base::Pickle for Message storage.

This CL modifies the mojo::Message implementation so that
its storage is always backed by a base::Pickle.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. https://codereview.chromium.org/1520153002
  5. This CL
  6. https://codereview.chromium.org/1526533002
  7. https://codereview.chromium.org/1524703002

BUG=
==========

[Ken Rockot(use gerrit already), 2015-12-14 20:38:57]

rockot@chromium.org changed reviewers:
+ yzshen@chromium.org

[Ken Rockot(use gerrit already), 2015-12-14 20:38:58]

Here's the patch which moves mojo::Message onto base::Pickle.

I looked at a few different strategies here. My second favorite was to not
change mojo::Message at all, and instead add a base::PickleWriter interface that
base::Pickle could implement (or that could be constructed over a base::Pickle
and have special privileges). Then we could implement a PickleWriter for writing
to a mojo::internal::Buffer, and update ParamTraits to Write to a PickleWriter.

That still might be the right approach in the end, but I went with this because
it has the least potential (i.e. zero) impact on existing Pickle consumers as it
adds no new indirection.

PTAL!

[Ken Rockot(use gerrit already), 2015-12-14 21:08:56]

Description was changed from

==========
[mojo] Use base::Pickle for Message storage.

This CL modifies the mojo::Message implementation so that
its storage is always backed by a base::Pickle.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. https://codereview.chromium.org/1520153002
  5. This CL
  6. https://codereview.chromium.org/1526533002
  7. https://codereview.chromium.org/1524703002

BUG=
==========

to

==========
[mojo] Use base::Pickle for Message storage.

This CL modifies the mojo::Message implementation so that
its storage is always backed by a base::Pickle.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. https://codereview.chromium.org/1520153002
  5. This CL
  6. https://codereview.chromium.org/1526533002
  7. https://codereview.chromium.org/1524703002

BUG=569669
==========

[yzshen1, 2015-12-15 19:07:02]

https://codereview.chromium.org/1524613002/diff/100001/base/pickle.h
File base/pickle.h (right):

https://codereview.chromium.org/1524613002/diff/100001/base/pickle.h#newcode261
base/pickle.h:261: // Claims |num_bytes| bytes of payload. This is similar to
Reserve in that it
style nit: Reserve -> Reserve() and |ClaimBytes| -> ClaimBytes().
I think that is more commonly used and consistent with line 210.

(And I think I saw some other places in this CL, too.)

https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/pickle_buffer.cc (right):

https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/pickle_buffer.cc:10: #include
"mojo/public/cpp/bindings/lib/bindings_serialization.h"
This is not used. Maybe you want to use internal::Align() from it in Allocate()?

https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/pickle_buffer.h (right):

https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/pickle_buffer.h:18: class PickleBuffer : public
base::Pickle, public Buffer {
Does it make sense to let PickleBuffer has a Pickle member instead?
IMO unless there are compelling reasons, it is nicer to avoid implementation
inheritance. Moreover, this class hides the data() method of base::Pickle.

https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/pickle_buffer.h:24: return reinterpret_cast<const
void*>(
nit: I think both reinterpret_cast can be static_cast.

https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/pickle_buffer.h:46: // necessary, but you can avoid
this by calling AllocData to preallocate when
I noticed that there is a DCHECK in the code, but maybe we should DCHECK before
making the change and fail the allocation if it needs to grow the Pickle's
capacity?

Because it violates the interface definition of Buffer: "Allocations remain
valid for the lifetime of the Buffer."

https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/pickle_buffer.h:62: // We can adopt Pickle's header
format for Mojo messages (or do any number of
I personally prefer not to change our wire format to fit Pickle definition.
Pickle seems to be something internal and (hopefully) transitional. If there is
one thing that needs to be changed, I think it should be Pickle.

https://codereview.chromium.org/1524613002/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/message.cc (right):

https://codereview.chromium.org/1524613002/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/message.cc:40: 
Please consider removing line 41 and 47-48, and replacing them with a  Reset()
call at the end.

https://codereview.chromium.org/1524613002/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/message.cc:80: void* mutable_data =
message.buffer()->Allocate(num_bytes);
It seems fairly confusing to require two allocate in a row. Without looking at
the code carefully, now I don't know whether I can access Message::data() before
doing message.buffer()->Allocate(), for example.

https://codereview.chromium.org/1524613002/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/message.h (right):

https://codereview.chromium.org/1524613002/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/message.h:48: return reinterpret_cast<const
internal::MessageHeader*>(buffer_->data());
nit: static_cast here and the two methods above?

[Ken Rockot(use gerrit already), 2015-12-15 19:37:35]

Thanks! I also added some unit tests for base::Pickle::ClaimBytes.

On 2015/12/15 at 19:07:02, yzshen wrote:
> https://codereview.chromium.org/1524613002/diff/100001/base/pickle.h
> File base/pickle.h (right):
> 
>
https://codereview.chromium.org/1524613002/diff/100001/base/pickle.h#newcode261
> base/pickle.h:261: // Claims |num_bytes| bytes of payload. This is similar to
Reserve in that it
> style nit: Reserve -> Reserve() and |ClaimBytes| -> ClaimBytes().
> I think that is more commonly used and consistent with line 210.
> 
> (And I think I saw some other places in this CL, too.)

Done (here, and also in message.h)

> 
>
https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
> File mojo/public/cpp/bindings/lib/pickle_buffer.cc (right):
> 
>
https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
> mojo/public/cpp/bindings/lib/pickle_buffer.cc:10: #include
"mojo/public/cpp/bindings/lib/bindings_serialization.h"
> This is not used. Maybe you want to use internal::Align() from it in
Allocate()?
> 

Ah right, that was my original intent. I'm using it now.

>
https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
> File mojo/public/cpp/bindings/lib/pickle_buffer.h (right):
> 
>
https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
> mojo/public/cpp/bindings/lib/pickle_buffer.h:18: class PickleBuffer : public
base::Pickle, public Buffer {
> Does it make sense to let PickleBuffer has a Pickle member instead?
> IMO unless there are compelling reasons, it is nicer to avoid implementation
inheritance. Moreover, this class hides the data() method of base::Pickle.

The reason I didn't do this is that I explicitly wanted to keep Pickle's
ClaimBytes interface protected from general use. So something would need to
inherit base::Pickle, and PickleBuffer is pretty lightweight. I think the
alternative is to make ClaimBytes a public method, but I'm not comfortable with
that since I suspect it will be easy to misuse.

> 
>
https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
> mojo/public/cpp/bindings/lib/pickle_buffer.h:24: return reinterpret_cast<const
void*>(
> nit: I think both reinterpret_cast can be static_cast.

static_cast will not cast between arbitrary pointer types (like void* and
char*).

> 
>
https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
> mojo/public/cpp/bindings/lib/pickle_buffer.h:46: // necessary, but you can
avoid this by calling AllocData to preallocate when
> I noticed that there is a DCHECK in the code, but maybe we should DCHECK
before making the change and fail the allocation if it needs to grow the
Pickle's capacity?
> 
> Because it violates the interface definition of Buffer: "Allocations remain
valid for the lifetime of the Buffer."

Oops, this is outdated documentation. The DCHECK is correct and I've updated the
docs. Not sure if I understand what you mean, but we do fail if Allocate() can't
succeed without realloc.

> 
>
https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
> mojo/public/cpp/bindings/lib/pickle_buffer.h:62: // We can adopt Pickle's
header format for Mojo messages (or do any number of
> I personally prefer not to change our wire format to fit Pickle definition.
Pickle seems to be something internal and (hopefully) transitional. If there is
one thing that needs to be changed, I think it should be Pickle.

In hindsight I agree. I've updated the comment to remove the bit about adopting
Pickle's format. I don't think we can change Pickle either, realistically,
because a lot of stuff Pickle creates is persisted. The format needs to remain
stable and never really left any bits for version signaling. So we'll probably
just allocate 8 extra bytes per message until we get rid of pickled IPC.

> 
>
https://codereview.chromium.org/1524613002/diff/140001/mojo/public/cpp/bindin...
> File mojo/public/cpp/bindings/lib/message.cc (right):
> 
>
https://codereview.chromium.org/1524613002/diff/140001/mojo/public/cpp/bindin...
> mojo/public/cpp/bindings/lib/message.cc:40: 
> Please consider removing line 41 and 47-48, and replacing them with a  Reset()
call at the end.

Good catch. :)  Done

> 
>
https://codereview.chromium.org/1524613002/diff/140001/mojo/public/cpp/bindin...
> mojo/public/cpp/bindings/lib/message.cc:80: void* mutable_data =
message.buffer()->Allocate(num_bytes);
> It seems fairly confusing to require two allocate in a row. Without looking at
the code carefully, now I don't know whether I can access Message::data() before
doing message.buffer()->Allocate(), for example.

I agree. Perhaps the Alloc*Data() methods should have a different name, like
Set*Capacity? Unfortunately I think there does need to be some dual interface
here: one for doing actual memory allocation, and one for claiming portions of
the allocated buffer. Really Allocate() should change its name, but it's used
all over the place and that would create a lot of churn.

> 
>
https://codereview.chromium.org/1524613002/diff/140001/mojo/public/cpp/bindin...
> File mojo/public/cpp/bindings/message.h (right):
> 
>
https://codereview.chromium.org/1524613002/diff/140001/mojo/public/cpp/bindin...
> mojo/public/cpp/bindings/message.h:48: return reinterpret_cast<const
internal::MessageHeader*>(buffer_->data());
> nit: static_cast here and the two methods above?

Cannot static_cast between arbitrary pointer types (only pointers of types along
an inheritance chain)

[Ken Rockot(use gerrit already), 2015-12-15 21:16:08]

So I made a few more changes here:

On 2015/12/15 at 19:37:35, Ken Rockot wrote:
> Thanks! I also added some unit tests for base::Pickle::ClaimBytes.
> 
> On 2015/12/15 at 19:07:02, yzshen wrote:
> > https://codereview.chromium.org/1524613002/diff/100001/base/pickle.h
> > File base/pickle.h (right):
> > 
> >
https://codereview.chromium.org/1524613002/diff/100001/base/pickle.h#newcode261
> > base/pickle.h:261: // Claims |num_bytes| bytes of payload. This is similar
to Reserve in that it
> > style nit: Reserve -> Reserve() and |ClaimBytes| -> ClaimBytes().
> > I think that is more commonly used and consistent with line 210.
> > 
> > (And I think I saw some other places in this CL, too.)
> 
> Done (here, and also in message.h)
> 
> > 
> >
https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
> > File mojo/public/cpp/bindings/lib/pickle_buffer.cc (right):
> > 
> >
https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
> > mojo/public/cpp/bindings/lib/pickle_buffer.cc:10: #include
"mojo/public/cpp/bindings/lib/bindings_serialization.h"
> > This is not used. Maybe you want to use internal::Align() from it in
Allocate()?
> > 
> 
> Ah right, that was my original intent. I'm using it now.
> 
> >
https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
> > File mojo/public/cpp/bindings/lib/pickle_buffer.h (right):
> > 
> >
https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
> > mojo/public/cpp/bindings/lib/pickle_buffer.h:18: class PickleBuffer : public
base::Pickle, public Buffer {
> > Does it make sense to let PickleBuffer has a Pickle member instead?
> > IMO unless there are compelling reasons, it is nicer to avoid implementation
inheritance. Moreover, this class hides the data() method of base::Pickle.
> 
> The reason I didn't do this is that I explicitly wanted to keep Pickle's
ClaimBytes interface protected from general use. So something would need to
inherit base::Pickle, and PickleBuffer is pretty lightweight. I think the
alternative is to make ClaimBytes a public method, but I'm not comfortable with
that since I suspect it will be easy to misuse.
> 
> > 
> >
https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
> > mojo/public/cpp/bindings/lib/pickle_buffer.h:24: return
reinterpret_cast<const void*>(
> > nit: I think both reinterpret_cast can be static_cast.
> 
> static_cast will not cast between arbitrary pointer types (like void* and
char*).
> 
> > 
> >
https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
> > mojo/public/cpp/bindings/lib/pickle_buffer.h:46: // necessary, but you can
avoid this by calling AllocData to preallocate when
> > I noticed that there is a DCHECK in the code, but maybe we should DCHECK
before making the change and fail the allocation if it needs to grow the
Pickle's capacity?
> > 
> > Because it violates the interface definition of Buffer: "Allocations remain
valid for the lifetime of the Buffer."
> 
> Oops, this is outdated documentation. The DCHECK is correct and I've updated
the docs. Not sure if I understand what you mean, but we do fail if Allocate()
can't succeed without realloc.

I think I got what you mean. I changed this so we'll return nullptr if the
available capacity is less than what we're about to Allocate().

A lot of code uses Buffer::Allocate without checking the return value though, so
I included a NOTREACHED in that code path anyway.

I also explicitly check that the memory location of data() is unchanged before
and after Allocate.

> 
> > 
> >
https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
> > mojo/public/cpp/bindings/lib/pickle_buffer.h:62: // We can adopt Pickle's
header format for Mojo messages (or do any number of
> > I personally prefer not to change our wire format to fit Pickle definition.
Pickle seems to be something internal and (hopefully) transitional. If there is
one thing that needs to be changed, I think it should be Pickle.
> 
> In hindsight I agree. I've updated the comment to remove the bit about
adopting Pickle's format. I don't think we can change Pickle either,
realistically, because a lot of stuff Pickle creates is persisted. The format
needs to remain stable and never really left any bits for version signaling. So
we'll probably just allocate 8 extra bytes per message until we get rid of
pickled IPC.
> 
> > 
> >
https://codereview.chromium.org/1524613002/diff/140001/mojo/public/cpp/bindin...
> > File mojo/public/cpp/bindings/lib/message.cc (right):
> > 
> >
https://codereview.chromium.org/1524613002/diff/140001/mojo/public/cpp/bindin...
> > mojo/public/cpp/bindings/lib/message.cc:40: 
> > Please consider removing line 41 and 47-48, and replacing them with a 
Reset() call at the end.
> 
> Good catch. :)  Done
> 
> > 
> >
https://codereview.chromium.org/1524613002/diff/140001/mojo/public/cpp/bindin...
> > mojo/public/cpp/bindings/lib/message.cc:80: void* mutable_data =
message.buffer()->Allocate(num_bytes);
> > It seems fairly confusing to require two allocate in a row. Without looking
at the code carefully, now I don't know whether I can access Message::data()
before doing message.buffer()->Allocate(), for example.
> 
> I agree. Perhaps the Alloc*Data() methods should have a different name, like
Set*Capacity? Unfortunately I think there does need to be some dual interface
here: one for doing actual memory allocation, and one for claiming portions of
the allocated buffer. Really Allocate() should change its name, but it's used
all over the place and that would create a lot of churn.

I've renamed the real allocation methods to ReserveCapacity and
ReserveUninitializedCapacity.

> 
> > 
> >
https://codereview.chromium.org/1524613002/diff/140001/mojo/public/cpp/bindin...
> > File mojo/public/cpp/bindings/message.h (right):
> > 
> >
https://codereview.chromium.org/1524613002/diff/140001/mojo/public/cpp/bindin...
> > mojo/public/cpp/bindings/message.h:48: return reinterpret_cast<const
internal::MessageHeader*>(buffer_->data());
> > nit: static_cast here and the two methods above?
> 
> Cannot static_cast between arbitrary pointer types (only pointers of types
along an inheritance chain)

[yzshen1, 2015-12-15 22:27:08]

> > >
>
https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
> > > File mojo/public/cpp/bindings/lib/pickle_buffer.h (right):
> > > 
> > >
>
https://codereview.chromium.org/1524613002/diff/100001/mojo/public/cpp/bindin...
> > > mojo/public/cpp/bindings/lib/pickle_buffer.h:18: class PickleBuffer :
public
> base::Pickle, public Buffer {
> > > Does it make sense to let PickleBuffer has a Pickle member instead?
> > > IMO unless there are compelling reasons, it is nicer to avoid
implementation
> inheritance. Moreover, this class hides the data() method of base::Pickle.
> > 
> > The reason I didn't do this is that I explicitly wanted to keep Pickle's
> ClaimBytes interface protected from general use. So something would need to
> inherit base::Pickle, and PickleBuffer is pretty lightweight. I think the
> alternative is to make ClaimBytes a public method, but I'm not comfortable
with
> that since I suspect it will be easy to misuse.

I agree that it is nice to keep Pickle's ClaimBytes protected.
But I am still not quite sure about mixing the interfaces of Pickle and Buffer,
especially having different meaning of data(), which is not a virtual method so
it is static-bound.

IMO, it is better to define a Pickle subclass whose sole purpose is to expose
ClaimBytes, and make it a member of PickleBuffer. This approach has exactly the
same memory footprint as directly inheriting Pickle, and it seems a little
cleaner.

https://codereview.chromium.org/1524613002/diff/180001/base/pickle_unittest.cc
File base/pickle_unittest.cc (right):

https://codereview.chromium.org/1524613002/diff/180001/base/pickle_unittest.c...
base/pickle_unittest.cc:547: EXPECT_EQ(0, reinterpret_cast<char*>(bytes)[i]);
(In response to your previous comment about reinterpret_cast<> has to be used.)
Have you tried it out? :)
Actually static_cast<> can be used between void* and other pointer types. I
always use it that way.

I don't think it is an important issue, but static_cast<> can not be used with
arbitrary types so it is a little safer.
(And some other reasons could be seen here:
http://stackoverflow.com/questions/310451/should-i-use-static-cast-or-reinter...)

https://codereview.chromium.org/1524613002/diff/180001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/message.cc (right):

https://codereview.chromium.org/1524613002/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/message.cc:30: void
Message::ReserveCapacity(uint32_t num_bytes) {
According to our discussion, let's make it Initialize(num_bytes, zero_out) or
something like that.

https://codereview.chromium.org/1524613002/diff/180001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/pickle_buffer.h (right):

https://codereview.chromium.org/1524613002/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/pickle_buffer.h:36: void ReserveCapacity(uint32_t
num_bytes);
"Reserve capacity" doesn't sound like it zero out the memory.

Maybe we should call it ReserveCapacityAndClear(), and the next one
ReserveCapacity().

As you said, the best names probably should be Allocate once we change the
current Allocate to Claim.

[Ken Rockot(use gerrit already), 2015-12-16 00:09:33]

I split the pickle storage out from PickleBuffer, that should work

https://codereview.chromium.org/1524613002/diff/180001/base/pickle_unittest.cc
File base/pickle_unittest.cc (right):

https://codereview.chromium.org/1524613002/diff/180001/base/pickle_unittest.c...
base/pickle_unittest.cc:547: EXPECT_EQ(0, reinterpret_cast<char*>(bytes)[i]);
On 2015/12/15 at 22:27:08, yzshen1 wrote:
> (In response to your previous comment about reinterpret_cast<> has to be
used.)
> Have you tried it out? :)

I thought I had... :D

> Actually static_cast<> can be used between void* and other pointer types. I
always use it that way.
> 
> I don't think it is an important issue, but static_cast<> can not be used with
arbitrary types so it is a little safer.
> (And some other reasons could be seen here:
>
http://stackoverflow.com/questions/310451/should-i-use-static-cast-or-reinter...)

I've updated things to static_cast

https://codereview.chromium.org/1524613002/diff/180001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/message.cc (right):

https://codereview.chromium.org/1524613002/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/message.cc:30: void
Message::ReserveCapacity(uint32_t num_bytes) {
On 2015/12/15 at 22:27:08, yzshen1 wrote:
> According to our discussion, let's make it Initialize(num_bytes, zero_out) or
something like that.

Done

https://codereview.chromium.org/1524613002/diff/180001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/pickle_buffer.h (right):

https://codereview.chromium.org/1524613002/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/pickle_buffer.h:36: void ReserveCapacity(uint32_t
num_bytes);
On 2015/12/15 at 22:27:08, yzshen1 wrote:
> "Reserve capacity" doesn't sound like it zero out the memory.
> 
> Maybe we should call it ReserveCapacityAndClear(), and the next one
ReserveCapacity().
> 
> As you said, the best names probably should be Allocate once we change the
current Allocate to Claim.

PickleBuffer doesn't actually need to be resizable if we treat allocation as an
initialization-only step of Message. I've changed PickleBuffer to take size and
zero_initialized args in its constructor.

[yzshen1, 2015-12-16 00:29:28]

Only two nits. LGTM. Thanks!

https://codereview.chromium.org/1524613002/diff/200001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/message.cc (right):

https://codereview.chromium.org/1524613002/diff/200001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/message.cc:26: buffer_.reset();
Is there anyone calling Reset()?

If not, maybe it makes more sense to remove it.

If yes, please consider commenting explicitly that it returns to the
pre-initialize state and Initialize must be called again.

WDYT?

https://codereview.chromium.org/1524613002/diff/200001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/pickle_buffer.h (right):

https://codereview.chromium.org/1524613002/diff/200001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/pickle_buffer.h:35: PickleBuffer* AsPickleBuffer()
override;
nit: It seems nice to put this method and Allocate in the same section because
they are from the same base class.

[Ken Rockot(use gerrit already), 2015-12-16 01:08:48]

rockot@chromium.org changed reviewers:
+ thestig@chromium.org

[Ken Rockot(use gerrit already), 2015-12-16 01:08:49]

Lei: Would you please review the protected addition to base::Pickle? Thanks!

[Lei Zhang, 2015-12-16 01:46:15]

lgtm

https://codereview.chromium.org/1524613002/diff/240001/base/pickle.h
File base/pickle.h (right):

https://codereview.chromium.org/1524613002/diff/240001/base/pickle.h#newcode264
base/pickle.h:264: void* ClaimBytes(size_t num_bytes);
Can you document the return value?

https://codereview.chromium.org/1524613002/diff/240001/base/pickle_unittest.cc
File base/pickle_unittest.cc (right):

https://codereview.chromium.org/1524613002/diff/240001/base/pickle_unittest.c...
base/pickle_unittest.cc:530: class TestingPickle : public base::Pickle {
no need for base:: because you are already in namespace base.

https://codereview.chromium.org/1524613002/diff/240001/base/pickle_unittest.c...
base/pickle_unittest.cc:545: void* bytes = pickle.ClaimBytes(kChunkSize);
You can also just static_cast here instead of in the for loop?

[Ken Rockot(use gerrit already), 2015-12-16 01:53:41]

Thanks!

https://codereview.chromium.org/1524613002/diff/240001/base/pickle.h
File base/pickle.h (right):

https://codereview.chromium.org/1524613002/diff/240001/base/pickle.h#newcode264
base/pickle.h:264: void* ClaimBytes(size_t num_bytes);
On 2015/12/16 at 01:46:15, Lei Zhang wrote:
> Can you document the return value?

Done

https://codereview.chromium.org/1524613002/diff/240001/base/pickle_unittest.cc
File base/pickle_unittest.cc (right):

https://codereview.chromium.org/1524613002/diff/240001/base/pickle_unittest.c...
base/pickle_unittest.cc:530: class TestingPickle : public base::Pickle {
On 2015/12/16 at 01:46:15, Lei Zhang wrote:
> no need for base:: because you are already in namespace base.

done

https://codereview.chromium.org/1524613002/diff/240001/base/pickle_unittest.c...
base/pickle_unittest.cc:545: void* bytes = pickle.ClaimBytes(kChunkSize);
On 2015/12/16 at 01:46:15, Lei Zhang wrote:
> You can also just static_cast here instead of in the for loop?

Done

[Ken Rockot(use gerrit already), 2015-12-17 01:20:19]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-17 02:35:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1524613002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1524613002/340001

[commit-bot: I haz the power, 2015-12-17 04:04:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-17 04:04:55]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2015-12-17 06:14:54]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2015-12-17 06:14:55]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org,
thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1524613002/#ps340001
(title: "happy compiler happy robots")

[commit-bot: I haz the power, 2015-12-17 06:15:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1524613002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1524613002/340001

[commit-bot: I haz the power, 2015-12-17 06:19:56]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2015-12-17 06:20:53]

Description was changed from

==========
[mojo] Use base::Pickle for Message storage.

This CL modifies the mojo::Message implementation so that
its storage is always backed by a base::Pickle.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. https://codereview.chromium.org/1520153002
  5. This CL
  6. https://codereview.chromium.org/1526533002
  7. https://codereview.chromium.org/1524703002

BUG=569669
==========

to

==========
[mojo] Use base::Pickle for Message storage.

This CL modifies the mojo::Message implementation so that
its storage is always backed by a base::Pickle.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. https://codereview.chromium.org/1520153002
  5. This CL
  6. https://codereview.chromium.org/1526533002
  7. https://codereview.chromium.org/1524703002

BUG=569669

Committed: https://crrev.com/0776a5005caf1c15e92ccf93546236737f935d0c
Cr-Commit-Position: refs/heads/master@{#365766}
==========

[commit-bot: I haz the power, 2015-12-17 06:20:53]

Patchset 18 (id:??) landed as
https://crrev.com/0776a5005caf1c15e92ccf93546236737f935d0c
Cr-Commit-Position: refs/heads/master@{#365766}