Me2me Native Messaging host on Windows: restructure NativeMessagingHost and NativeMessagingChannel.…

Me2me Native Messaging host on Windows: restructure NativeMessagingHost and NativeMessagingChannel.

1. Reverse the ownership relation between NativeMessagingHost and NativeMessagingChannel.
    a. The host now owns the channel. This gets us ready for the next change in which the host will own an addition channel to communicate with the elevated host. I believe the code is also simpler and cleaner after this change.
    b. Removed NativeMessagingChannel::Delegate.
    c. The host object, instead of the host process's main method, is now responsible for setting up and starting the channel.
2. Reimplement the threading logic of and re-enable the NativeMessagingHost unit tests.
3. Add ThreadChecker asserts in NativeMessagingHost methods.

BUG=325567, 323306
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240565

[Sergey Ulanov, 2013-12-12 19:10:27]

https://codereview.chromium.org/103693006/diff/1/remoting/host/it2me/it2me_na...
File remoting/host/it2me/it2me_native_messaging_host.cc (right):

https://codereview.chromium.org/103693006/diff/1/remoting/host/it2me/it2me_na...
remoting/host/it2me/it2me_native_messaging_host.cc:293: void
It2MeNativeMessagingHost::ShutDown() {
Do you really need this method? I think you can just pass quit_closure_ to the
channel. it2me_host_->Disconnect() can be called in the destructor.

https://codereview.chromium.org/103693006/diff/1/remoting/host/it2me/it2me_na...
File remoting/host/it2me/it2me_native_messaging_host.h (right):

https://codereview.chromium.org/103693006/diff/1/remoting/host/it2me/it2me_na...
remoting/host/it2me/it2me_native_messaging_host.h:31: base::PlatformFile input,
would it be better to pass the Channel object here instead of the pair of file
handles?

https://codereview.chromium.org/103693006/diff/1/remoting/host/native_messagi...
File remoting/host/native_messaging/native_messaging_channel.h (right):

https://codereview.chromium.org/103693006/diff/1/remoting/host/native_messagi...
remoting/host/native_messaging/native_messaging_channel.h:35:
SendMessageCallback received_message,
Would it be better to pass this to Start()?

https://codereview.chromium.org/103693006/diff/1/remoting/host/setup/me2me_na...
File remoting/host/setup/me2me_native_messaging_host.cc (right):

https://codereview.chromium.org/103693006/diff/1/remoting/host/setup/me2me_na...
remoting/host/setup/me2me_native_messaging_host.cc:87: void
NativeMessagingHost::ShutDown() {
Don't need this method. Just pass quit_closure_ to the channel when starting it.

[weitao, 2013-12-12 23:33:48]

PTAL.

https://codereview.chromium.org/103693006/diff/1/remoting/host/it2me/it2me_na...
File remoting/host/it2me/it2me_native_messaging_host.cc (right):

https://codereview.chromium.org/103693006/diff/1/remoting/host/it2me/it2me_na...
remoting/host/it2me/it2me_native_messaging_host.cc:293: void
It2MeNativeMessagingHost::ShutDown() {
On 2013/12/12 19:10:28, Sergey Ulanov wrote:
> Do you really need this method? I think you can just pass quit_closure_ to the
> channel. it2me_host_->Disconnect() can be called in the destructor.

Done.

https://codereview.chromium.org/103693006/diff/1/remoting/host/it2me/it2me_na...
File remoting/host/it2me/it2me_native_messaging_host.h (right):

https://codereview.chromium.org/103693006/diff/1/remoting/host/it2me/it2me_na...
remoting/host/it2me/it2me_native_messaging_host.h:31: base::PlatformFile input,
On 2013/12/12 19:10:28, Sergey Ulanov wrote:
> would it be better to pass the Channel object here instead of the pair of file
> handles?

Done.

https://codereview.chromium.org/103693006/diff/1/remoting/host/native_messagi...
File remoting/host/native_messaging/native_messaging_channel.h (right):

https://codereview.chromium.org/103693006/diff/1/remoting/host/native_messagi...
remoting/host/native_messaging/native_messaging_channel.h:35:
SendMessageCallback received_message,
On 2013/12/12 19:10:28, Sergey Ulanov wrote:
> Would it be better to pass this to Start()?

Done.

https://codereview.chromium.org/103693006/diff/1/remoting/host/setup/me2me_na...
File remoting/host/setup/me2me_native_messaging_host.cc (right):

https://codereview.chromium.org/103693006/diff/1/remoting/host/setup/me2me_na...
remoting/host/setup/me2me_native_messaging_host.cc:87: void
NativeMessagingHost::ShutDown() {
On 2013/12/12 19:10:28, Sergey Ulanov wrote:
> Don't need this method. Just pass quit_closure_ to the channel when starting
it.

Done.

[Sergey Ulanov, 2013-12-12 23:55:15]

lgtm

https://codereview.chromium.org/103693006/diff/40001/remoting/host/it2me/it2m...
File remoting/host/it2me/it2me_native_messaging_host_unittest.cc (right):

https://codereview.chromium.org/103693006/diff/40001/remoting/host/it2me/it2m...
remoting/host/it2me/it2me_native_messaging_host_unittest.cc:429:
ASSERT_TRUE(MakePipe(&input_read_handle, &input_write_handle_));
FWIW, not required for this CL: now that dependency on IO pipes is isolated in
NativeMessaginChannel you could make it an abstract interface and use mock here
instead of actual implementation. That would mean that you wouldn't need to
create these IO pipes and it would make the the test significantly simpler

https://codereview.chromium.org/103693006/diff/40001/remoting/host/setup/me2m...
File remoting/host/setup/me2me_native_messaging_host.cc (right):

https://codereview.chromium.org/103693006/diff/40001/remoting/host/setup/me2m...
remoting/host/setup/me2me_native_messaging_host.cc:144:
DCHECK(thread_checker_.CalledOnValidThread());
I'm not sure these checks in Process*() are useful, except in ProcessMessage().
These functions are called only from ProcessMessage() and you are already have
DCHECK() there. But feel free to disagree and keep these checks.

[commit-bot: I haz the power, 2013-12-13 00:30:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/weitaosu@chromium.org/103693006/40001

[commit-bot: I haz the power, 2013-12-13 09:15:37]

Change committed as 240565