Adding a unit test to verify the IPC channel between the network and desktop processes.

Adding a unit test to verify the IPC channel between the network and desktop processes.

Related changes:
  - The fake screen capturer now uses shared buffers when they available.
  - DesktopSessionAgent frees the client end handle of the network-to-desktop pipe once the connection has been established.

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

[alexeypa (please no reviews), 2013-01-31 00:09:46]

PTAL:

cdn@: remoting/host/chromoting_messages.h
sergeyu@ the rest.

[alexeypa (please no reviews), 2013-02-04 19:10:43]

ping.

[Sergey Ulanov, 2013-02-04 21:04:41]

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/daemon_p...
File remoting/host/daemon_process_win.cc (right):

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/daemon_p...
remoting/host/daemon_process_win.cc:114: base::ProcessHandle
desktop_process_for_transit =
Why do you need this change?

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/desktop_...
File remoting/host/desktop_session_agent.cc (right):

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_agent.cc:30: #include
"base/file_descriptor_posix.h"
nit: don't need #ifdef here - that header should compile on windows too.
Alternatively you can include base/file_util.h

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_agent.cc:82: DCHECK(desktop_pipe_ ==
IPC::InvalidPlatformFileForTransit());
DCHECK_EQ

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/desktop_...
File remoting/host/desktop_session_proxy.cc (right):

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_proxy.cc:118: DCHECK(desktop_process_ ==
base::kNullProcessHandle);
nit: DCHECK_EQ

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_proxy.cc:144: desktop_channel_.reset(new
IPC::ChannelProxy(
I think you can move this out of the ifdef. It's only ChannelHandle() creation
that's platform-specific here, so it will need to be kept in ifdef, but
everything elase is not.

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_proxy.cc:148: caller_task_runner_));
Should this be changed to io_task_runner_ too?

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_proxy.cc:220:
PostCaptureCompleted(scoped_refptr<media::ScreenCaptureData>());
Why do we need to do this? I don't see anything in IpcVideoFrameCapturer that
can handle the case when capture_data is NULL.

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/ipc_desk...
File remoting/host/ipc_desktop_environment_unittest.cc (right):

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
2013 please

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment_unittest.cc:206:
video_capturer_->Start(&screen_capturer_delegate_);
It looks wrong that all this code is in SetUp() method. Normally SetUp() is used
to initialize basic objects that are common between tests (e.g. start threads
that the test may use). There is only one test right now, so it doesn't make
sense to put all this code in SetUp(). I think most of it should be placed
either in the body of the test or maybe some helper methods.
It's also good idea to add another test that creates a video capturer, but
doesn't call CaputureFrame().

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment_unittest.cc:209: void
IpcDesktopEnvironmentTest::TearDown() {
nit: No need to override this method if it's empty.

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment_unittest.cc:307:
video_capturer_->CaptureFrame();
I think it's better to move this call to the body of the test, so that it's
easier to add new tests. E.g. you can quit |run_loop_| here and then call
CaptureFrame() in the test itself.

[alexeypa (please no reviews), 2013-02-04 23:45:58]

PTAL

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/daemon_p...
File remoting/host/daemon_process_win.cc (right):

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/daemon_p...
remoting/host/daemon_process_win.cc:114: base::ProcessHandle
desktop_process_for_transit =
On 2013/02/04 21:04:41, sergeyu wrote:
> Why do you need this change?

This is caused by changing DesktopSessionProxy::AttachToDesktop()'s parameter
from IPC::PlatformFileForTransit to base::ProcessHandle. This way I don't need
to add more #if defined(OS_WIN) in all places where |desktop_process| handle is
passed. It also allows me to use process utility functions on the handle without
additional #ifdefs.

I changed this code to call DuplicateHandle() to make it more apparent what is
going on here. This is a Windows-only source so it is safe.

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/desktop_...
File remoting/host/desktop_session_agent.cc (right):

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_agent.cc:30: #include
"base/file_descriptor_posix.h"
On 2013/02/04 21:04:41, sergeyu wrote:
> nit: don't need #ifdef here - that header should compile on windows too.
> Alternatively you can include base/file_util.h

Done.

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_agent.cc:82: DCHECK(desktop_pipe_ ==
IPC::InvalidPlatformFileForTransit());
On 2013/02/04 21:04:41, sergeyu wrote:
> DCHECK_EQ

Done.

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/desktop_...
File remoting/host/desktop_session_proxy.cc (right):

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_proxy.cc:118: DCHECK(desktop_process_ ==
base::kNullProcessHandle);
On 2013/02/04 21:04:41, sergeyu wrote:
> nit: DCHECK_EQ

Done.

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_proxy.cc:144: desktop_channel_.reset(new
IPC::ChannelProxy(
On 2013/02/04 21:04:41, sergeyu wrote:
> I think you can move this out of the ifdef. It's only ChannelHandle() creation
> that's platform-specific here, so it will need to be kept in ifdef, but
> everything elase is not.

Done.

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_proxy.cc:148: caller_task_runner_));
On 2013/02/04 21:04:41, sergeyu wrote:
> Should this be changed to io_task_runner_ too?

Done.

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_proxy.cc:220:
PostCaptureCompleted(scoped_refptr<media::ScreenCaptureData>());
On 2013/02/04 21:04:41, sergeyu wrote:
> Why do we need to do this? I don't see anything in IpcVideoFrameCapturer that
> can handle the case when capture_data is NULL.

VideoFrameCapturer::Delegate requires OnCaptureCompleted() to be called as many
time as CaptureFrame() was called. VideoScheduler relies on this to shutdown
correctly.

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/ipc_desk...
File remoting/host/ipc_desktop_environment_unittest.cc (right):

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/02/04 21:04:41, sergeyu wrote:
> 2013 please

Done.

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment_unittest.cc:206:
video_capturer_->Start(&screen_capturer_delegate_);
On 2013/02/04 21:04:41, sergeyu wrote:
> It looks wrong that all this code is in SetUp() method. Normally SetUp() is
used
> to initialize basic objects that are common between tests (e.g. start threads
> that the test may use). There is only one test right now, so it doesn't make
> sense to put all this code in SetUp(). I think most of it should be placed
> either in the body of the test or maybe some helper methods.
> It's also good idea to add another test that creates a video capturer, but
> doesn't call CaputureFrame().

Done.

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment_unittest.cc:209: void
IpcDesktopEnvironmentTest::TearDown() {
On 2013/02/04 21:04:41, sergeyu wrote:
> nit: No need to override this method if it's empty.

Done.

https://chromiumcodereview.appspot.com/12096071/diff/1/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment_unittest.cc:307:
video_capturer_->CaptureFrame();
On 2013/02/04 21:04:41, sergeyu wrote:
> I think it's better to move this call to the body of the test, so that it's
> easier to add new tests. E.g. you can quit |run_loop_| here and then call
> CaptureFrame() in the test itself.

Done.

[Cris Neckar, 2013-02-06 19:31:24]

IPC security audit LGTM

https://codereview.chromium.org/12096071/diff/7002/media/video/capture/screen...
File media/video/capture/screen/screen_capturer_fake.cc (right):

https://codereview.chromium.org/12096071/diff/7002/media/video/capture/screen...
media/video/capture/screen/screen_capturer_fake.cc:44: int buffer_size =
size_.height() * bytes_per_row_;
nit: size_t

[Sergey Ulanov, 2013-02-06 19:36:59]

lgtm

[alexeypa (please no reviews), 2013-02-06 22:58:39]

FYI

https://chromiumcodereview.appspot.com/12096071/diff/7002/media/video/capture...
File media/video/capture/screen/screen_capturer_fake.cc (right):

https://chromiumcodereview.appspot.com/12096071/diff/7002/media/video/capture...
media/video/capture/screen/screen_capturer_fake.cc:44: int buffer_size =
size_.height() * bytes_per_row_;
On 2013/02/06 19:31:24, Cris Neckar wrote:
> nit: size_t

width, height and bytes_per_row_ are all |int| here. buffer_size is passed to
CreateSharedBuffer() which also expects it to be int. I guess it is better to
keep it as |int| in spite of the fact that it represents a memory buffer size.

[commit-bot: I haz the power, 2013-02-06 23:25:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/12096071/17003

[commit-bot: I haz the power, 2013-02-06 23:58:23]

Failed to trigger a try job on linux_clang
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-02-06 23:58:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/12096071/21005

[commit-bot: I haz the power, 2013-02-07 00:53:28]

Failed to trigger a try job on linux_chromeos
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-02-07 00:55:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/12096071/15013

[commit-bot: I haz the power, 2013-02-07 05:54:25]

Change committed as 181221