[Chromoting] Use ProcessStatsSender in DaemonProcess (daemon process)

[Chromoting] Use ProcessStatsSender in DaemonProcess (daemon process)

This is a copy & paste from DesktopSessionAgent
(https://codereview.chromium.org/2933203002/) to DaemonProcess to allow daemon
process to report process statistics data to network process.

BUG=650926
Review-Url: https://codereview.chromium.org/2950993003
Cr-Commit-Position: refs/heads/master@{#483539}
Committed: https://chromium.googlesource.com/chromium/src/+/dda3acb809c43e2d633c335778ff4a1522927e59

[Hzj_jie, 2017-06-21 00:59:59]

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

[commit-bot: I haz the power, 2017-06-21 01:00:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2017-06-21 01:30:47]

Description was changed from

==========
[Chromoting] Use ProcessStatsSender in DaemonProcess (daemon process)

This is a copy & paste from DesktopSessionAgent
(https://codereview.chromium.org/2933203002/) to DaemonProcess to allow daemon
process to report process statistics data to network process.

BUG=650926
==========

to

==========
[Chromoting] Use ProcessStatsSender in DaemonProcess (daemon process)

This is a copy & paste from DesktopSessionAgent
(https://codereview.chromium.org/2933203002/) to DaemonProcess to allow daemon
process to report process statistics data to network process.

BUG=650926
==========

[Hzj_jie, 2017-06-21 01:30:47]

zijiehe@chromium.org changed reviewers:
+ joedow@chromium.org

[commit-bot: I haz the power, 2017-06-21 01:49:28]

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

[commit-bot: I haz the power, 2017-06-21 01:49:29]

Dry run: This issue passed the CQ dry run.

[joedow, 2017-06-21 15:30:50]

https://codereview.chromium.org/2950993003/diff/1/remoting/host/daemon_process.h
File remoting/host/daemon_process.h (right):

https://codereview.chromium.org/2950993003/diff/1/remoting/host/daemon_proces...
remoting/host/daemon_process.h:207: // Daemon and Network processes manages
multiple desktop sessions. Some of
Process stats data is tied to a specific ClientSession (which lives in the
network process), it be initiated from the desktop session right?

My mental model was that the Network process (specifically the ClientSession
instance) would send the initial start/stop IPC messages to the other processes.

Individual desktop sessions would start/stop on their own as they receive IPC
messages and are created / destroyed.  We shouldn't need ref-counting in the
Daemon process to track them.

[joedow, 2017-06-21 15:32:04]

https://codereview.chromium.org/2950993003/diff/1/remoting/host/daemon_process.h
File remoting/host/daemon_process.h (right):

https://codereview.chromium.org/2950993003/diff/1/remoting/host/daemon_proces...
remoting/host/daemon_process.h:207: // Daemon and Network processes manages
multiple desktop sessions. Some of
On 2017/06/21 15:30:50, joedow wrote:
> Process stats data is tied to a specific ClientSession (which lives in the
> network process), it be initiated from the desktop session right?
> 
> My mental model was that the Network process (specifically the ClientSession
> instance) would send the initial start/stop IPC messages to the other
processes.
> 
> Individual desktop sessions would start/stop on their own as they receive IPC
> messages and are created / destroyed.  We shouldn't need ref-counting in the
> Daemon process to track them.
> 

Oops., the top line should say:
Process stats data is tied to a specific ClientSession (which lives in the
network process), it shouldn't be initiated from the desktop session right?

[Hzj_jie, 2017-06-21 18:53:36]

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

[commit-bot: I haz the power, 2017-06-21 18:53:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 19:28:45]

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

[commit-bot: I haz the power, 2017-06-21 19:28:46]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Hzj_jie, 2017-06-21 20:16:25]

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

[Hzj_jie, 2017-06-21 20:16:28]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-06-21 20:16:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2017-06-21 20:52:15]

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

[commit-bot: I haz the power, 2017-06-21 20:52:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2017-06-21 20:57:25]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-06-21 21:34:15]

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

[commit-bot: I haz the power, 2017-06-21 21:34:16]

Dry run: This issue passed the CQ dry run.

[Hzj_jie, 2017-06-21 21:42:56]

https://codereview.chromium.org/2950993003/diff/1/remoting/host/daemon_process.h
File remoting/host/daemon_process.h (right):

https://codereview.chromium.org/2950993003/diff/1/remoting/host/daemon_proces...
remoting/host/daemon_process.h:207: // Daemon and Network processes manages
multiple desktop sessions. Some of
On 2017/06/21 15:32:04, joedow wrote:
> On 2017/06/21 15:30:50, joedow wrote:
> > Process stats data is tied to a specific ClientSession (which lives in the
> > network process), it be initiated from the desktop session right?
> > 
> > My mental model was that the Network process (specifically the ClientSession
> > instance) would send the initial start/stop IPC messages to the other
> processes.
> > 
> > Individual desktop sessions would start/stop on their own as they receive
IPC
> > messages and are created / destroyed.  We shouldn't need ref-counting in the
> > Daemon process to track them.
> > 
> 
> Oops., the top line should say:
> Process stats data is tied to a specific ClientSession (which lives in the
> network process), it shouldn't be initiated from the desktop session right?

After some more detailed investigations, I believe current solution is still
reasonable.
The ref-counter is just a two-line change, but it matches existing design of the
host instance in network process. If it was a 20-line change, it would be
questionable whether we should support this non-supported feature. But a
two-line change seems like a no-brainer to me: why should not we avoid the
confusion for free?
From the analysis perspective, daemon process is far more important than the RDP
process. Daemon process handles all functions RDP process handles, plus
configuration update, host status monitoring, connect / disconnect client
sessions, etc. More important is that daemon process is running on every
machine. But RDP process is running when curtain mode is on, which, I doubt, is
for Googlers only.
I have no concern to also let RDP process report its resource usage though.

[joedow, 2017-06-21 22:00:20]

https://codereview.chromium.org/2950993003/diff/1/remoting/host/daemon_process.h
File remoting/host/daemon_process.h (right):

https://codereview.chromium.org/2950993003/diff/1/remoting/host/daemon_proces...
remoting/host/daemon_process.h:207: // Daemon and Network processes manages
multiple desktop sessions. Some of
On 2017/06/21 21:42:56, Hzj_jie wrote:
> On 2017/06/21 15:32:04, joedow wrote:
> > On 2017/06/21 15:30:50, joedow wrote:
> > > Process stats data is tied to a specific ClientSession (which lives in the
> > > network process), it be initiated from the desktop session right?
> > > 
> > > My mental model was that the Network process (specifically the
ClientSession
> > > instance) would send the initial start/stop IPC messages to the other
> > processes.
> > > 
> > > Individual desktop sessions would start/stop on their own as they receive
> IPC
> > > messages and are created / destroyed.  We shouldn't need ref-counting in
the
> > > Daemon process to track them.
> > > 
> > 
> > Oops., the top line should say:
> > Process stats data is tied to a specific ClientSession (which lives in the
> > network process), it shouldn't be initiated from the desktop session right?
> 
> After some more detailed investigations, I believe current solution is still
> reasonable.
> The ref-counter is just a two-line change, but it matches existing design of
the
> host instance in network process. If it was a 20-line change, it would be
> questionable whether we should support this non-supported feature. But a
> two-line change seems like a no-brainer to me: why should not we avoid the
> confusion for free?
> From the analysis perspective, daemon process is far more important than the
RDP
> process. Daemon process handles all functions RDP process handles, plus
> configuration update, host status monitoring, connect / disconnect client
> sessions, etc. More important is that daemon process is running on every
> machine. But RDP process is running when curtain mode is on, which, I doubt,
is
> for Googlers only.
> I have no concern to also let RDP process report its resource usage though.

My concerns are:
1.) The ref count code isn't going to be exercised since we don't support
multiple connections
2.) LOC doesn't really matter, if code is never run then it doesn't provide
value (whether it is 2 lines or 20)
3.) I don't think the Daemon process or RDP session is interesting enough to
monitor.  The question I posed is whether anyone can think of a scenario that we
require the data, so far I have not been able to.

My proposal is to only report Network and Desktop process info.
- This is future proof with respect to concurrent connections
- It is resilient in that the cleanup is guaranteed
  - desktop session is closed when session ends
  - Client session owns the Network process sender which is destroyed when the
session is closed
- It provide data for the processes we are most likely to want data on
- You can rename the IPC messages to be more specific (remove the 'any'
sender/recipient term and replace with the actual process name.  This makes the
IPC message code more readable

If we are able to conceive a scenario where having the Daemon or RDP process
data is useful then adding more code is fine.  I can't think of anything that
isn't a stretch.

[Hzj_jie, 2017-06-21 23:53:59]

On 2017/06/21 22:00:20, joedow wrote:
>
https://codereview.chromium.org/2950993003/diff/1/remoting/host/daemon_process.h
> File remoting/host/daemon_process.h (right):
> 
>
https://codereview.chromium.org/2950993003/diff/1/remoting/host/daemon_proces...
> remoting/host/daemon_process.h:207: // Daemon and Network processes manages
> multiple desktop sessions. Some of
> On 2017/06/21 21:42:56, Hzj_jie wrote:
> > On 2017/06/21 15:32:04, joedow wrote:
> > > On 2017/06/21 15:30:50, joedow wrote:
> > > > Process stats data is tied to a specific ClientSession (which lives in
the
> > > > network process), it be initiated from the desktop session right?
> > > > 
> > > > My mental model was that the Network process (specifically the
> ClientSession
> > > > instance) would send the initial start/stop IPC messages to the other
> > > processes.
> > > > 
> > > > Individual desktop sessions would start/stop on their own as they
receive
> > IPC
> > > > messages and are created / destroyed.  We shouldn't need ref-counting in
> the
> > > > Daemon process to track them.
> > > > 
> > > 
> > > Oops., the top line should say:
> > > Process stats data is tied to a specific ClientSession (which lives in the
> > > network process), it shouldn't be initiated from the desktop session
right?
> > 
> > After some more detailed investigations, I believe current solution is still
> > reasonable.
> > The ref-counter is just a two-line change, but it matches existing design of
> the
> > host instance in network process. If it was a 20-line change, it would be
> > questionable whether we should support this non-supported feature. But a
> > two-line change seems like a no-brainer to me: why should not we avoid the
> > confusion for free?
> > From the analysis perspective, daemon process is far more important than the
> RDP
> > process. Daemon process handles all functions RDP process handles, plus
> > configuration update, host status monitoring, connect / disconnect client
> > sessions, etc. More important is that daemon process is running on every
> > machine. But RDP process is running when curtain mode is on, which, I doubt,
> is
> > for Googlers only.
> > I have no concern to also let RDP process report its resource usage though.
> 
> My concerns are:
> 1.) The ref count code isn't going to be exercised since we don't support
> multiple connections
> 2.) LOC doesn't really matter, if code is never run then it doesn't provide
> value (whether it is 2 lines or 20)

I think my main point is "matching existing design". The StartProcessStatsReport
requests come from IpcDesktopEnvironment. And we do support multiple
IpcDesktopEnvironment instances in ChromotingHost. Simply ignoring this existing
design would make it more confusing.

> 3.) I don't think the Daemon process or RDP session is interesting enough to
> monitor.  The question I posed is whether anyone can think of a scenario that
we
> require the data, so far I have not been able to.

The other point is *daemon process is far more important than the RDP process*.
I do not think that "we do not have it for a long time" equals to "we do not
need it at all".
And I also do not think we need to actively monitor the resource usage of daemon
process individually, but include it in to the total resource usage is still
reasonable: it's always running on end users' machines and it has more
functionalities.

> 
> My proposal is to only report Network and Desktop process info.
> - This is future proof with respect to concurrent connections
> - It is resilient in that the cleanup is guaranteed
>   - desktop session is closed when session ends
>   - Client session owns the Network process sender which is destroyed when the
> session is closed
> - It provide data for the processes we are most likely to want data on
> - You can rename the IPC messages to be more specific (remove the 'any'
> sender/recipient term and replace with the actual process name.  This makes
the
> IPC message code more readable
> 
> If we are able to conceive a scenario where having the Daemon or RDP process
> data is useful then adding more code is fine.  I can't think of anything that
> isn't a stretch.

[joedow, 2017-06-23 18:03:08]

A few comments on this approach if you decide not to go the centralized monitor
process (using process IDs) route.

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
File remoting/host/daemon_process.cc (right):

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
remoting/host/daemon_process.cc:148: void
DaemonProcess::OnWorkerProcessStopped(int exit_code) {
You can remove the exit_code param since it isn't being used.

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
remoting/host/daemon_process.cc:389: DCHECK(process_stats_request_count_ > 0);
If you want to keep the request_count, I'd prefer that we
DCHECK_EQ(process_stats_request_count_, 0); here instead (with a comment that
the functionality needs to be tested before enabling if we support multiple,
concurrent sessions in the future.

The reason is that since we can't test the code, I'd prefer to indicate that
explicitly instead of assuming it will work later.

The other option is to remove the process_stats_request_count_ completely and
DCHECK on whether the stats_sender_ is valid.

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
remoting/host/daemon_process.cc:392: DCHECK_EQ(process_stats_request_count_ ==
1, !stats_sender_);
I don't think you should allow calling start multiple times with different
intervals, that just complicates the code.  I don't think the ability to change
the interval dynamically is useful, either way the originating process can send
a stop IPC and then restart the timers with the new interval.

Removing this extra condition means you can simplify the code too as
process_stats_request_count_ is guaranteed to be 1:

DCHECK(!stats_sender_);

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
remoting/host/daemon_process.cc:403: DCHECK(process_stats_request_count_ >= 0);
DCHECK_EQ(process_stats_request_count_, 0)

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
File remoting/host/daemon_process_unittest.cc (right):

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
remoting/host/daemon_process_unittest.cc:363: }
If you feel strongly about keeping the counter, then please add tests to
validate it (multiple start calls).  Also add a test for dynamic interval
changes too if you keep that functionality.

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/worker_pr...
File remoting/host/worker_process_ipc_delegate.h (right):

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/worker_pr...
remoting/host/worker_process_ipc_delegate.h:33: // Notifies that the worker
process stops for any reason.
Wouldn't we know this because the IPC channel gets disconnected?  I'm not sure
that this method is necessary.

Seeing the additional changes makes me think that using the Daemon process (or
network process) with process IDs is a much simpler approach.

[Hzj_jie, 2017-06-23 23:46:41]

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

[commit-bot: I haz the power, 2017-06-23 23:47:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-24 00:22:12]

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

[commit-bot: I haz the power, 2017-06-24 00:22:13]

Dry run: This issue passed the CQ dry run.

[Hzj_jie, 2017-06-24 00:51:41]

I think it's still reasonable to use this solution before finishing the
investigation of other approaches. I can hard to say that fetching process
states from daemon process is a very cheap change which is worthy to fully
ignore all existing work. We may still face other problems during the
implementations.
For example,
 - What if process stats are required from multiple sessions (well, at least we
can enable the feature for one ME2ME session and one IT2ME session)? Should we
cache the result for a while or calling native API each time?
 - How should we ensure the platform independence? Current solution uses a
platform neutral API in base which is supported on each platform. (Though I know
technically speaking Daemon Process is used by Windows only.)

So before we figured out all the issues we may face, I still think it's worthy
to make current solution work.

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
File remoting/host/daemon_process.cc (right):

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
remoting/host/daemon_process.cc:148: void
DaemonProcess::OnWorkerProcessStopped(int exit_code) {
On 2017/06/23 18:03:07, joedow wrote:
> You can remove the exit_code param since it isn't being used.

From the perspective of WorkerProcessLauncher, it's pretty reasonable to export
the exit_code in OnWorkerProcessStopped() callback. The same parameter exists in
OnPermanentError() callback.

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
remoting/host/daemon_process.cc:389: DCHECK(process_stats_request_count_ > 0);
On 2017/06/23 18:03:07, joedow wrote:
> If you want to keep the request_count, I'd prefer that we
> DCHECK_EQ(process_stats_request_count_, 0); here instead (with a comment that
> the functionality needs to be tested before enabling if we support multiple,
> concurrent sessions in the future.
> 
> The reason is that since we can't test the code, I'd prefer to indicate that
> explicitly instead of assuming it will work later.
> 
> The other option is to remove the process_stats_request_count_ completely and
> DCHECK on whether the stats_sender_ is valid.

Test cases are added to cover the scenarios.
I think it's pretty fine to assume DaemonProcess can work as expected now :)

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
remoting/host/daemon_process.cc:392: DCHECK_EQ(process_stats_request_count_ ==
1, !stats_sender_);
On 2017/06/23 18:03:07, joedow wrote:
> I don't think you should allow calling start multiple times with different
> intervals, that just complicates the code.  I don't think the ability to
change
> the interval dynamically is useful, either way the originating process can
send
> a stop IPC and then restart the timers with the new interval.
> 
> Removing this extra condition means you can simplify the code too as
> process_stats_request_count_ is guaranteed to be 1:
> 
> DCHECK(!stats_sender_);

Here the trouble is different sessions may request different interval. I have
added test case to cover it anyway.

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
remoting/host/daemon_process.cc:403: DCHECK(process_stats_request_count_ >= 0);
On 2017/06/23 18:03:07, joedow wrote:
> DCHECK_EQ(process_stats_request_count_, 0)

Done.

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
File remoting/host/daemon_process_unittest.cc (right):

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
remoting/host/daemon_process_unittest.cc:363: }
On 2017/06/23 18:03:07, joedow wrote:
> If you feel strongly about keeping the counter, then please add tests to
> validate it (multiple start calls).  Also add a test for dynamic interval
> changes too if you keep that functionality.

Done.

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/worker_pr...
File remoting/host/worker_process_ipc_delegate.h (right):

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/worker_pr...
remoting/host/worker_process_ipc_delegate.h:33: // Notifies that the worker
process stops for any reason.
On 2017/06/23 18:03:08, joedow wrote:
> Wouldn't we know this because the IPC channel gets disconnected?  I'm not sure
> that this method is necessary.
> 
> Seeing the additional changes makes me think that using the Daemon process (or
> network process) with process IDs is a much simpler approach.

I do not think the disconnecting event has been sent to WorkerProcessIpcDelegate
/ DaemonProcess. DaemonProcess does not own the IPC channel to the worker
process AFAIK.

[Hzj_jie, 2017-06-29 00:17:07]

On 2017/06/24 00:51:41, Hzj_jie wrote:
> I think it's still reasonable to use this solution before finishing the
> investigation of other approaches. I can hard to say that fetching process
> states from daemon process is a very cheap change which is worthy to fully
> ignore all existing work. We may still face other problems during the
> implementations.
> For example,
>  - What if process stats are required from multiple sessions (well, at least
we
> can enable the feature for one ME2ME session and one IT2ME session)? Should we
> cache the result for a while or calling native API each time?
>  - How should we ensure the platform independence? Current solution uses a
> platform neutral API in base which is supported on each platform. (Though I
know
> technically speaking Daemon Process is used by Windows only.)
> 
> So before we figured out all the issues we may face, I still think it's worthy
> to make current solution work.
> 
>
https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
> File remoting/host/daemon_process.cc (right):
> 
>
https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
> remoting/host/daemon_process.cc:148: void
> DaemonProcess::OnWorkerProcessStopped(int exit_code) {
> On 2017/06/23 18:03:07, joedow wrote:
> > You can remove the exit_code param since it isn't being used.
> 
> From the perspective of WorkerProcessLauncher, it's pretty reasonable to
export
> the exit_code in OnWorkerProcessStopped() callback. The same parameter exists
in
> OnPermanentError() callback.
> 
>
https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
> remoting/host/daemon_process.cc:389: DCHECK(process_stats_request_count_ > 0);
> On 2017/06/23 18:03:07, joedow wrote:
> > If you want to keep the request_count, I'd prefer that we
> > DCHECK_EQ(process_stats_request_count_, 0); here instead (with a comment
that
> > the functionality needs to be tested before enabling if we support multiple,
> > concurrent sessions in the future.
> > 
> > The reason is that since we can't test the code, I'd prefer to indicate that
> > explicitly instead of assuming it will work later.
> > 
> > The other option is to remove the process_stats_request_count_ completely
and
> > DCHECK on whether the stats_sender_ is valid.
> 
> Test cases are added to cover the scenarios.
> I think it's pretty fine to assume DaemonProcess can work as expected now :)
> 
>
https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
> remoting/host/daemon_process.cc:392: DCHECK_EQ(process_stats_request_count_ ==
> 1, !stats_sender_);
> On 2017/06/23 18:03:07, joedow wrote:
> > I don't think you should allow calling start multiple times with different
> > intervals, that just complicates the code.  I don't think the ability to
> change
> > the interval dynamically is useful, either way the originating process can
> send
> > a stop IPC and then restart the timers with the new interval.
> > 
> > Removing this extra condition means you can simplify the code too as
> > process_stats_request_count_ is guaranteed to be 1:
> > 
> > DCHECK(!stats_sender_);
> 
> Here the trouble is different sessions may request different interval. I have
> added test case to cover it anyway.
> 
>
https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
> remoting/host/daemon_process.cc:403: DCHECK(process_stats_request_count_ >=
0);
> On 2017/06/23 18:03:07, joedow wrote:
> > DCHECK_EQ(process_stats_request_count_, 0)
> 
> Done.
> 
>
https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
> File remoting/host/daemon_process_unittest.cc (right):
> 
>
https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
> remoting/host/daemon_process_unittest.cc:363: }
> On 2017/06/23 18:03:07, joedow wrote:
> > If you feel strongly about keeping the counter, then please add tests to
> > validate it (multiple start calls).  Also add a test for dynamic interval
> > changes too if you keep that functionality.
> 
> Done.
> 
>
https://codereview.chromium.org/2950993003/diff/60001/remoting/host/worker_pr...
> File remoting/host/worker_process_ipc_delegate.h (right):
> 
>
https://codereview.chromium.org/2950993003/diff/60001/remoting/host/worker_pr...
> remoting/host/worker_process_ipc_delegate.h:33: // Notifies that the worker
> process stops for any reason.
> On 2017/06/23 18:03:08, joedow wrote:
> > Wouldn't we know this because the IPC channel gets disconnected?  I'm not
sure
> > that this method is necessary.
> > 
> > Seeing the additional changes makes me think that using the Daemon process
(or
> > network process) with process IDs is a much simpler approach.
> 
> I do not think the disconnecting event has been sent to
WorkerProcessIpcDelegate
> / DaemonProcess. DaemonProcess does not own the IPC channel to the worker
> process AFAIK.

Any comments to this change?

[joedow, 2017-06-29 17:24:03]

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
File remoting/host/daemon_process.cc (right):

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
remoting/host/daemon_process.cc:148: void
DaemonProcess::OnWorkerProcessStopped(int exit_code) {
On 2017/06/24 00:51:40, Hzj_jie wrote:
> On 2017/06/23 18:03:07, joedow wrote:
> > You can remove the exit_code param since it isn't being used.
> 
> From the perspective of WorkerProcessLauncher, it's pretty reasonable to
export
> the exit_code in OnWorkerProcessStopped() callback. The same parameter exists
in
> OnPermanentError() callback.

I think we should prefer a cleaner function signature unless you can think of a
usage of the exit_code.  It is only used in a DCHECK in the permanent error
method (which is OK) but is completely unused here.  If we decide we need it at
some point in the future, we can add it back.

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
remoting/host/daemon_process.cc:403: DCHECK(process_stats_request_count_ >= 0);
On 2017/06/24 00:51:40, Hzj_jie wrote:
> On 2017/06/23 18:03:07, joedow wrote:
> > DCHECK_EQ(process_stats_request_count_, 0)
> 
> Done.

I don't see this change in the latest patchset.

If you want to keep the condition, then you can use DCHECK_GE.

[Hzj_jie, 2017-06-29 18:32:39]

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

[commit-bot: I haz the power, 2017-06-29 18:33:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 18:50:50]

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

[commit-bot: I haz the power, 2017-06-29 18:50:51]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Hzj_jie, 2017-06-29 18:53:05]

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

[Hzj_jie, 2017-06-29 18:53:14]

Patchset #4 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-06-29 18:53:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 19:31:17]

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

[commit-bot: I haz the power, 2017-06-29 19:31:18]

Dry run: This issue passed the CQ dry run.

[Hzj_jie, 2017-06-29 20:24:57]

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
File remoting/host/daemon_process.cc (right):

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
remoting/host/daemon_process.cc:148: void
DaemonProcess::OnWorkerProcessStopped(int exit_code) {
On 2017/06/29 17:24:03, joedow wrote:
> On 2017/06/24 00:51:40, Hzj_jie wrote:
> > On 2017/06/23 18:03:07, joedow wrote:
> > > You can remove the exit_code param since it isn't being used.
> > 
> > From the perspective of WorkerProcessLauncher, it's pretty reasonable to
> export
> > the exit_code in OnWorkerProcessStopped() callback. The same parameter
exists
> in
> > OnPermanentError() callback.
> 
> I think we should prefer a cleaner function signature unless you can think of
a
> usage of the exit_code.  It is only used in a DCHECK in the permanent error
> method (which is OK) but is completely unused here.  If we decide we need it
at
> some point in the future, we can add it back.

Removed.

https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr...
remoting/host/daemon_process.cc:403: DCHECK(process_stats_request_count_ >= 0);
On 2017/06/29 17:24:03, joedow wrote:
> On 2017/06/24 00:51:40, Hzj_jie wrote:
> > On 2017/06/23 18:03:07, joedow wrote:
> > > DCHECK_EQ(process_stats_request_count_, 0)
> > 
> > Done.
> 
> I don't see this change in the latest patchset.
> 
> If you want to keep the condition, then you can use DCHECK_GE.

Sorry.

[joedow, 2017-06-29 20:30:17]

lgtm

[Hzj_jie, 2017-06-29 23:13:48]

The CQ bit was checked by zijiehe@chromium.org

[commit-bot: I haz the power, 2017-06-29 23:14:07]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 23:19:20]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1498778028052410,
"parent_rev": "09d842cd48364ad7cb992ae0b84415a6bdeb148e", "commit_rev":
"dda3acb809c43e2d633c335778ff4a1522927e59"}

[commit-bot: I haz the power, 2017-06-29 23:19:34]

Description was changed from

==========
[Chromoting] Use ProcessStatsSender in DaemonProcess (daemon process)

This is a copy & paste from DesktopSessionAgent
(https://codereview.chromium.org/2933203002/) to DaemonProcess to allow daemon
process to report process statistics data to network process.

BUG=650926
==========

to

==========
[Chromoting] Use ProcessStatsSender in DaemonProcess (daemon process)

This is a copy & paste from DesktopSessionAgent
(https://codereview.chromium.org/2933203002/) to DaemonProcess to allow daemon
process to report process statistics data to network process.

BUG=650926

Review-Url: https://codereview.chromium.org/2950993003
Cr-Commit-Position: refs/heads/master@{#483539}
Committed:
https://chromium.googlesource.com/chromium/src/+/dda3acb809c43e2d633c335778ff...
==========

[commit-bot: I haz the power, 2017-06-29 23:19:36]

Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/dda3acb809c43e2d633c335778ff...