Make DataPipe own an impl.

Make DataPipe own an impl.

* Add a DataPipeImpl class (basically consisting of the
  DataPipe::...ImplNoLock() methods).
* Make LocalDataPipeImpl subclass that, instead of DataPipe.
* Give DataPipe own such an impl (and call its methods, instead of the
  ...ImplNoLock() methods).

This is so we'll be able to switch impls.

R=yzshen@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/736e0cc30313290b17243d7a6513fcc8e12821a2

[viettrungluu, 2015-02-18 20:39:57]

(Dependent on https://codereview.chromium.org/936173002/, obviously.)

[yzshen1, 2015-02-19 18:27:11]

lgtm

https://codereview.chromium.org/926553006/diff/20001/mojo/edk/system/data_pip...
File mojo/edk/system/data_pipe_impl.h (right):

https://codereview.chromium.org/926553006/diff/20001/mojo/edk/system/data_pip...
mojo/edk/system/data_pipe_impl.h:23: class MOJO_SYSTEM_IMPL_EXPORT DataPipeImpl
{
Does it make sense to have comment talk about how this class is used? (owned by
XXX; methods are called under XXX's lock; etc.)

https://codereview.chromium.org/926553006/diff/20001/mojo/edk/system/data_pip...
mojo/edk/system/data_pipe_impl.h:30: // |num_bytes.Get()| will be a nonzero
multiple of |element_num_bytes_|.
nit: there isn't a |element_num_bytes_| defined in this class.

https://codereview.chromium.org/926553006/diff/20001/mojo/edk/system/local_da...
File mojo/edk/system/local_data_pipe_impl.h (right):

https://codereview.chromium.org/926553006/diff/20001/mojo/edk/system/local_da...
mojo/edk/system/local_data_pipe_impl.h:30: static DataPipe* Create(const
MojoCreateDataPipeOptions& validated_options);
nit: It seems a little bit unusual for a static Create() method to create an
object of a different type (and not its base class).

[viettrungluu, 2015-02-19 18:34:01]

On 2015/02/19 18:27:11, yzshen1 wrote:
> lgtm
> 
>
https://codereview.chromium.org/926553006/diff/20001/mojo/edk/system/data_pip...
> File mojo/edk/system/data_pipe_impl.h (right):
> 
>
https://codereview.chromium.org/926553006/diff/20001/mojo/edk/system/data_pip...
> mojo/edk/system/data_pipe_impl.h:23: class MOJO_SYSTEM_IMPL_EXPORT
DataPipeImpl
> {
> Does it make sense to have comment talk about how this class is used? (owned
by
> XXX; methods are called under XXX's lock; etc.)

Yes, I'll do that.

> 
>
https://codereview.chromium.org/926553006/diff/20001/mojo/edk/system/data_pip...
> mojo/edk/system/data_pipe_impl.h:30: // |num_bytes.Get()| will be a nonzero
> multiple of |element_num_bytes_|.
> nit: there isn't a |element_num_bytes_| defined in this class.

Will fix.

> 
>
https://codereview.chromium.org/926553006/diff/20001/mojo/edk/system/local_da...
> File mojo/edk/system/local_data_pipe_impl.h (right):
> 
>
https://codereview.chromium.org/926553006/diff/20001/mojo/edk/system/local_da...
> mojo/edk/system/local_data_pipe_impl.h:30: static DataPipe* Create(const
> MojoCreateDataPipeOptions& validated_options);
> nit: It seems a little bit unusual for a static Create() method to create an
> object of a different type (and not its base class).

Agreed (that did occur to me). It's somewhat related to DataPipe's suboptimal
constructor.

Maybe I should move it to DataPipe (and call it "CreateLocal" or something like
that).

I don't love that either, but then at least it'd be consistent with MessagePipe
(etc.), and then I could make DataPipe's suboptimal constructor private.

WDYT?

[yzshen1, 2015-02-19 18:41:23]

On 2015/02/19 18:34:01, viettrungluu wrote:
> On 2015/02/19 18:27:11, yzshen1 wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/926553006/diff/20001/mojo/edk/system/data_pip...
> > File mojo/edk/system/data_pipe_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/926553006/diff/20001/mojo/edk/system/data_pip...
> > mojo/edk/system/data_pipe_impl.h:23: class MOJO_SYSTEM_IMPL_EXPORT
> DataPipeImpl
> > {
> > Does it make sense to have comment talk about how this class is used? (owned
> by
> > XXX; methods are called under XXX's lock; etc.)
> 
> Yes, I'll do that.
> 
> > 
> >
>
https://codereview.chromium.org/926553006/diff/20001/mojo/edk/system/data_pip...
> > mojo/edk/system/data_pipe_impl.h:30: // |num_bytes.Get()| will be a nonzero
> > multiple of |element_num_bytes_|.
> > nit: there isn't a |element_num_bytes_| defined in this class.
> 
> Will fix.
> 
> > 
> >
>
https://codereview.chromium.org/926553006/diff/20001/mojo/edk/system/local_da...
> > File mojo/edk/system/local_data_pipe_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/926553006/diff/20001/mojo/edk/system/local_da...
> > mojo/edk/system/local_data_pipe_impl.h:30: static DataPipe* Create(const
> > MojoCreateDataPipeOptions& validated_options);
> > nit: It seems a little bit unusual for a static Create() method to create an
> > object of a different type (and not its base class).
> 
> Agreed (that did occur to me). It's somewhat related to DataPipe's suboptimal
> constructor.
> 
> Maybe I should move it to DataPipe (and call it "CreateLocal" or something
like
> that).
> 
> I don't love that either, but then at least it'd be consistent with
MessagePipe
> (etc.), and then I could make DataPipe's suboptimal constructor private.
> 
> WDYT?

That sounds good to me. Thanks!

[viettrungluu, 2015-02-19 18:58:12]

New patchsets have been uploaded after l-g-t-m from yzshen@chromium.org

[viettrungluu, 2015-02-19 19:08:46]

OK, I changed it to DataPipe::CreateLocal() (and added/updated comments).

https://codereview.chromium.org/926553006/diff/20001/mojo/edk/system/data_pip...
File mojo/edk/system/data_pipe_impl.h (right):

https://codereview.chromium.org/926553006/diff/20001/mojo/edk/system/data_pip...
mojo/edk/system/data_pipe_impl.h:23: class MOJO_SYSTEM_IMPL_EXPORT DataPipeImpl
{
On 2015/02/19 18:27:11, yzshen1 wrote:
> Does it make sense to have comment talk about how this class is used? (owned
by
> XXX; methods are called under XXX's lock; etc.)

Done.

https://codereview.chromium.org/926553006/diff/20001/mojo/edk/system/data_pip...
mojo/edk/system/data_pipe_impl.h:30: // |num_bytes.Get()| will be a nonzero
multiple of |element_num_bytes_|.
On 2015/02/19 18:27:11, yzshen1 wrote:
> nit: there isn't a |element_num_bytes_| defined in this class.

Done (elsewhere too, obviously).

https://codereview.chromium.org/926553006/diff/20001/mojo/edk/system/local_da...
File mojo/edk/system/local_data_pipe_impl.h (right):

https://codereview.chromium.org/926553006/diff/20001/mojo/edk/system/local_da...
mojo/edk/system/local_data_pipe_impl.h:24: // |validated_options| should be the
output of |DataPipe::ValidateOptions()|.
Oops, this comment was stale too.

[yzshen1, 2015-02-19 19:13:42]

still LGTM

[viettrungluu, 2015-02-19 19:17:07]

Committed patchset #4 (id:60001) manually as
736e0cc30313290b17243d7a6513fcc8e12821a2 (presubmit successful).