Refactoring: Move MessagePipeReader subclasess out from ChannelMojo

Refactoring: Move MessagePipeReader subclasess out from ChannelMojo

There are a few MessagePipeReader sublcasses defined as inter-classes
of ChannelMojo. This makes it harder to hook them for unit testing.
This CL moves these classes their own file. Now unittest can
define subclasses of these to hook some behavor for testing.

This is a preparation for a crash fix.

BUG=410813
TEST=none
R=viettrungluu@chromium.org,yzshen@chromium.org

Committed: https://crrev.com/3b41d6ca4deea242d5fa916d4c4864fd08bddf06
Cr-Commit-Position: refs/heads/master@{#294439}

[yzshen1, 2014-09-09 23:52:40]

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo.cc
File ipc/mojo/ipc_channel_mojo.cc (right):

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo.cc...
ipc/mojo/ipc_channel_mojo.cc:243: return false;
This is supposed to be a MojoResult.

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo.h
File ipc/mojo/ipc_channel_mojo.h (right):

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo.h#...
ipc/mojo/ipc_channel_mojo.h:86: // has ChannelMojo as a friend class.
nit: please make the previous line as close to 80 chars as possible.

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo.h#...
ipc/mojo/ipc_channel_mojo.h:88: Message& message,
I think our style prefer pointer to non-const ref. And output parameter is
supposed to be at the end.

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo.h#...
ipc/mojo/ipc_channel_mojo.h:91: std::vector<MojoHandle>& handles);
Please change it to pointer.

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo_re...
File ipc/mojo/ipc_channel_mojo_readers.cc (right):

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo_re...
ipc/mojo/ipc_channel_mojo_readers.cc:19:
//------------------------------------------------------------------------------
This separator seems unnecessary.

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo_re...
File ipc/mojo/ipc_channel_mojo_readers.h (right):

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo_re...
ipc/mojo/ipc_channel_mojo_readers.h:5: #ifndef IPC_IPC_CHANNEL_MOJO_READERS_H_
MOJO_, please.

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo_re...
ipc/mojo/ipc_channel_mojo_readers.h:42: ChannelMojo* owner_;
DISALLOW_COPY_AND_ASSIGN?
(here and below)

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo_re...
ipc/mojo/ipc_channel_mojo_readers.h:51: virtual bool Connect();
Please have an empty line below it.
And add comments such as "Subclasses may override this to ...."

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo_re...
ipc/mojo/ipc_channel_mojo_readers.h:52: virtual void OnPipeClosed() OVERRIDE;
It seems nice to add a comment "MessagePipeReader implementation". (here and
below)

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo_re...
ipc/mojo/ipc_channel_mojo_readers.h:65: virtual bool Connect() OVERRIDE;
It seems nice to add a comment "ControlReader overrides". (here and below)

[Hajime Morrita, 2014-09-10 17:16:35]

Thanks for the review Yuzhu! PTAL?

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo.h
File ipc/mojo/ipc_channel_mojo.h (right):

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo.h#...
ipc/mojo/ipc_channel_mojo.h:86: // has ChannelMojo as a friend class.
On 2014/09/09 23:52:40, yzshen1 wrote:
> nit: please make the previous line as close to 80 chars as possible.

Done.

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo.h#...
ipc/mojo/ipc_channel_mojo.h:88: Message& message,
On 2014/09/09 23:52:39, yzshen1 wrote:
> I think our style prefer pointer to non-const ref. And output parameter is
> supposed to be at the end.

Done.

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo.h#...
ipc/mojo/ipc_channel_mojo.h:91: std::vector<MojoHandle>& handles);
On 2014/09/09 23:52:40, yzshen1 wrote:
> Please change it to pointer.

Done.

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo_re...
File ipc/mojo/ipc_channel_mojo_readers.h (right):

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo_re...
ipc/mojo/ipc_channel_mojo_readers.h:5: #ifndef IPC_IPC_CHANNEL_MOJO_READERS_H_
On 2014/09/09 23:52:40, yzshen1 wrote:
> MOJO_, please.

Done.

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo_re...
ipc/mojo/ipc_channel_mojo_readers.h:42: ChannelMojo* owner_;
On 2014/09/09 23:52:40, yzshen1 wrote:
> DISALLOW_COPY_AND_ASSIGN?
> (here and below)

Done.

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo_re...
ipc/mojo/ipc_channel_mojo_readers.h:42: ChannelMojo* owner_;
On 2014/09/09 23:52:40, yzshen1 wrote:
> DISALLOW_COPY_AND_ASSIGN?
> (here and below)

Done.

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo_re...
ipc/mojo/ipc_channel_mojo_readers.h:51: virtual bool Connect();
On 2014/09/09 23:52:40, yzshen1 wrote:
> Please have an empty line below it.
> And add comments such as "Subclasses may override this to ...."

Done.

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo_re...
ipc/mojo/ipc_channel_mojo_readers.h:52: virtual void OnPipeClosed() OVERRIDE;
On 2014/09/09 23:52:40, yzshen1 wrote:
> It seems nice to add a comment "MessagePipeReader implementation". (here and
> below)

Done.

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo_re...
ipc/mojo/ipc_channel_mojo_readers.h:65: virtual bool Connect() OVERRIDE;
On 2014/09/09 23:52:40, yzshen1 wrote:
> It seems nice to add a comment "ControlReader overrides". (here and below)

Done.

[yzshen1, 2014-09-10 17:31:09]

Only a few nits.

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo_re...
File ipc/mojo/ipc_channel_mojo_readers.cc (right):

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo_re...
ipc/mojo/ipc_channel_mojo_readers.cc:19:
//------------------------------------------------------------------------------
On 2014/09/09 23:52:40, yzshen1 wrote:
> This separator seems unnecessary.

?

https://codereview.chromium.org/559723002/diff/20001/ipc/mojo/ipc_channel_moj...
File ipc/mojo/ipc_channel_mojo.cc (right):

https://codereview.chromium.org/559723002/diff/20001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo.cc:243: return false;
This issue hasn't been addressed. It is supposed to return MojoResult

https://codereview.chromium.org/559723002/diff/20001/ipc/mojo/ipc_channel_moj...
File ipc/mojo/ipc_channel_mojo_readers.h (right):

https://codereview.chromium.org/559723002/diff/20001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo_readers.h:73: // ControlReader implemenation
To be exact, this is "override" instead of "implementation", because the base
class has provided an implementation.

[Hajime Morrita, 2014-09-10 18:04:02]

PTAL?

https://codereview.chromium.org/559723002/diff/20001/ipc/mojo/ipc_channel_moj...
File ipc/mojo/ipc_channel_mojo.cc (right):

https://codereview.chromium.org/559723002/diff/20001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo.cc:243: return false;
On 2014/09/10 17:31:09, yzshen1 wrote:
> This issue hasn't been addressed. It is supposed to return MojoResult

Oops. done.

https://codereview.chromium.org/559723002/diff/20001/ipc/mojo/ipc_channel_moj...
File ipc/mojo/ipc_channel_mojo_readers.h (right):

https://codereview.chromium.org/559723002/diff/20001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo_readers.h:73: // ControlReader implemenation
On 2014/09/10 17:31:09, yzshen1 wrote:
> To be exact, this is "override" instead of "implementation", because the base
> class has provided an implementation.

Done.

[yzshen1, 2014-09-10 18:06:16]

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo_re...
File ipc/mojo/ipc_channel_mojo_readers.cc (right):

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo_re...
ipc/mojo/ipc_channel_mojo_readers.cc:19:
//------------------------------------------------------------------------------
On 2014/09/10 17:31:09, yzshen1 wrote:
> On 2014/09/09 23:52:40, yzshen1 wrote:
> > This separator seems unnecessary.
> 
> ?

This is a very trivial suggestion. But if you don't think it is useful, please
respond so I know you have noticed this comment. :)

[Hajime Morrita, 2014-09-10 20:29:53]

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo_re...
File ipc/mojo/ipc_channel_mojo_readers.cc (right):

https://codereview.chromium.org/559723002/diff/1/ipc/mojo/ipc_channel_mojo_re...
ipc/mojo/ipc_channel_mojo_readers.cc:19:
//------------------------------------------------------------------------------
On 2014/09/10 18:06:16, yzshen1 wrote:
> On 2014/09/10 17:31:09, yzshen1 wrote:
> > On 2014/09/09 23:52:40, yzshen1 wrote:
> > > This separator seems unnecessary.
> > 
> > ?
> 
> This is a very trivial suggestion. But if you don't think it is useful, please
> respond so I know you have noticed this comment. :)
Oops again. I overlooked this one. I'm sorry about that...
Removed.

[yzshen1, 2014-09-10 20:36:10]

lgtm

[Hajime Morrita, 2014-09-10 21:54:33]

Trung, could you take a look? Just moving classes to different file.

[viettrungluu, 2014-09-11 17:10:25]

Rubberstamp LGTM w/nits.

https://codereview.chromium.org/559723002/diff/60001/ipc/mojo/ipc_channel_moj...
File ipc/mojo/ipc_channel_mojo_readers.h (right):

https://codereview.chromium.org/559723002/diff/60001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo_readers.h:11: #include "base/memory/scoped_vector.h"
nit: I don't think you use scoped_vector.h

https://codereview.chromium.org/559723002/diff/60001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo_readers.h:12: #include "base/memory/weak_ptr.h"
... nor weak_ptr

https://codereview.chromium.org/559723002/diff/60001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo_readers.h:15: #include "ipc/ipc_export.h"
nit: I don't think this file has any exports

https://codereview.chromium.org/559723002/diff/60001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo_readers.h:31: // A MessagePipeReader implemenation for
IPC::Message communication.
nit: implemenation -> implementation

https://codereview.chromium.org/559723002/diff/60001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo_readers.h:84: };
nit: probably you should have a DISALLOW_COPY_AND_ASSIGN (and probably also
include base/macros.h for it)

https://codereview.chromium.org/559723002/diff/60001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo_readers.h:96: };
ditto

[Hajime Morrita, 2014-09-11 17:19:11]

Thanks for the review! Landing with fix.

https://codereview.chromium.org/559723002/diff/60001/ipc/mojo/ipc_channel_moj...
File ipc/mojo/ipc_channel_mojo_readers.h (right):

https://codereview.chromium.org/559723002/diff/60001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo_readers.h:11: #include "base/memory/scoped_vector.h"
On 2014/09/11 17:10:25, viettrungluu wrote:
> nit: I don't think you use scoped_vector.h

Done.

https://codereview.chromium.org/559723002/diff/60001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo_readers.h:12: #include "base/memory/weak_ptr.h"
On 2014/09/11 17:10:24, viettrungluu wrote:
> ... nor weak_ptr

Done.

https://codereview.chromium.org/559723002/diff/60001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo_readers.h:15: #include "ipc/ipc_export.h"
On 2014/09/11 17:10:25, viettrungluu wrote:
> nit: I don't think this file has any exports

Done.

https://codereview.chromium.org/559723002/diff/60001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo_readers.h:31: // A MessagePipeReader implemenation for
IPC::Message communication.
On 2014/09/11 17:10:25, viettrungluu wrote:
> nit: implemenation -> implementation

Done.

[Hajime Morrita, 2014-09-11 17:32:34]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-09-11 17:40:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/559723002/80001

[commit-bot: I haz the power, 2014-09-11 19:07:34]

Committed patchset #5 (id:80001) as 8fa7675d96fccf24a98d0f4bc9d4438b09f9c20e

[commit-bot: I haz the power, 2014-09-11 19:12:36]

Patchset 5 (id:??) landed as
https://crrev.com/3b41d6ca4deea242d5fa916d4c4864fd08bddf06
Cr-Commit-Position: refs/heads/master@{#294439}