[Chromoting] Allow ProcessStatsSender to receive nullptr ProcessStatsAgent

[Chromoting] Allow ProcessStatsSender to receive nullptr ProcessStatsAgent

This change allows ProcessStatsSender to receive nullptr ProcessStatsAgent, so
the caller does not need to care about this issue.
Meanwhile FakeProcessStatsAgent is also moved to a standalone class to share
with other tests.
Refer to pending change https://codereview.chromium.org/2858813002/ for the
usage.

BUG=650926

[Hzj_jie, 2017-05-03 18:50:08]

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

[commit-bot: I haz the power, 2017-05-03 18:50:44]

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-05-03 18:57:07]

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

[commit-bot: I haz the power, 2017-05-03 18:57:12]

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-05-03 18:57:18]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-05-03 20:37:32]

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

[commit-bot: I haz the power, 2017-05-03 20:37:33]

Dry run: This issue passed the CQ dry run.

[Hzj_jie, 2017-05-03 21:05:38]

Description was changed from

==========
[Chromoting] Allow ProcessStatsSender to receive nullptr ProcessStatsAgent

This change allows ProcessStatsSender to receive nullptr ProcessStatsAgent, so
the caller does not need to care about this issue.
Meanwhile FakeProcessStatsAgent is also moved to a standalone class to share
with other tests.
Refer to pending change https://codereview.chromium.org/2858813002/ for the
usage.

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

to

==========
[Chromoting] Allow ProcessStatsSender to receive nullptr ProcessStatsAgent

This change allows ProcessStatsSender to receive nullptr ProcessStatsAgent, so
the caller does not need to care about this issue.
Meanwhile FakeProcessStatsAgent is also moved to a standalone class to share
with other tests.
Refer to pending change https://codereview.chromium.org/2858813002/ for the
usage.

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

[Hzj_jie, 2017-05-03 21:05:38]

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

[joedow, 2017-05-03 21:41:02]

https://codereview.chromium.org/2860803002/diff/20001/remoting/host/process_s...
File remoting/host/process_stats_sender.cc (right):

https://codereview.chromium.org/2860803002/diff/20001/remoting/host/process_s...
remoting/host/process_stats_sender.cc:36: agents_(RemoveNullAgent(agents)),
I'm not sure about the approach here.  It seems much simpler just to pass the
data over the event channel.  I think that would simplify your code quite a bit
and remove the need for code like this (which is pretty easy to break if a bug
ever occurred).

I can't imagine there will be enough data that sending it over the event channel
would be a problem.

https://codereview.chromium.org/2860803002/diff/20001/remoting/host/process_s...
remoting/host/process_stats_sender.cc:40: DCHECK(interval > base::TimeDelta());
The !is_zero() check seems more readable, why the change?

https://codereview.chromium.org/2860803002/diff/20001/remoting/protocol/fake_...
File remoting/protocol/fake_process_stats_stub.h (right):

https://codereview.chromium.org/2860803002/diff/20001/remoting/protocol/fake_...
remoting/protocol/fake_process_stats_stub.h:25: void
set_expected_usage_count(size_t expected_usage_count);
nit: trivial setters like this can be written inline.

[Hzj_jie, 2017-05-03 22:18:04]

https://codereview.chromium.org/2860803002/diff/20001/remoting/host/process_s...
File remoting/host/process_stats_sender.cc (right):

https://codereview.chromium.org/2860803002/diff/20001/remoting/host/process_s...
remoting/host/process_stats_sender.cc:36: agents_(RemoveNullAgent(agents)),
On 2017/05/03 21:41:01, joedow wrote:
> I'm not sure about the approach here.  It seems much simpler just to pass the
> data over the event channel.  I think that would simplify your code quite a
bit
> and remove the need for code like this (which is pretty easy to break if a bug
> ever occurred).
> 
> I can't imagine there will be enough data that sending it over the event
channel
> would be a problem.

:)

https://codereview.chromium.org/2860803002/diff/20001/remoting/host/process_s...
remoting/host/process_stats_sender.cc:40: DCHECK(interval > base::TimeDelta());
On 2017/05/03 21:41:01, joedow wrote:
> The !is_zero() check seems more readable, why the change?

Timer handles negative interval as 0:
https://cs.chromium.org/chromium/src/base/timer/timer.cc?rcl=e6baa90f9f21576e...

https://codereview.chromium.org/2860803002/diff/20001/remoting/protocol/fake_...
File remoting/protocol/fake_process_stats_stub.h (right):

https://codereview.chromium.org/2860803002/diff/20001/remoting/protocol/fake_...
remoting/protocol/fake_process_stats_stub.h:25: void
set_expected_usage_count(size_t expected_usage_count);
On 2017/05/03 21:41:02, joedow wrote:
> nit: trivial setters like this can be written inline.

Since we have a cc file anyway, I believe avoiding inline functions in header
file should be preferred.

[joedow, 2017-05-08 16:25:41]

https://codereview.chromium.org/2860803002/diff/20001/remoting/host/process_s...
File remoting/host/process_stats_sender.cc (right):

https://codereview.chromium.org/2860803002/diff/20001/remoting/host/process_s...
remoting/host/process_stats_sender.cc:40: DCHECK(interval > base::TimeDelta());
On 2017/05/03 22:18:04, Hzj_jie wrote:
> On 2017/05/03 21:41:01, joedow wrote:
> > The !is_zero() check seems more readable, why the change?
> 
> Timer handles negative interval as 0:
>
https://cs.chromium.org/chromium/src/base/timer/timer.cc?rcl=e6baa90f9f21576e...

I haven't experienced negative TimeDeltas being a problem but it seems
reasonable (though less readable) to check for it.

Perhaps a cleaner approach would be to add a helper method to TimeDelta which is
'is_positive()'.

https://codereview.chromium.org/2860803002/diff/20001/remoting/protocol/fake_...
File remoting/protocol/fake_process_stats_stub.h (right):

https://codereview.chromium.org/2860803002/diff/20001/remoting/protocol/fake_...
remoting/protocol/fake_process_stats_stub.h:25: void
set_expected_usage_count(size_t expected_usage_count);
On 2017/05/03 22:18:04, Hzj_jie wrote:
> On 2017/05/03 21:41:02, joedow wrote:
> > nit: trivial setters like this can be written inline.
> 
> Since we have a cc file anyway, I believe avoiding inline functions in header
> file should be preferred.

inline functions are fine for trivial getters:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[Hzj_jie, 2017-05-08 18:16:19]

https://codereview.chromium.org/2860803002/diff/20001/remoting/host/process_s...
File remoting/host/process_stats_sender.cc (right):

https://codereview.chromium.org/2860803002/diff/20001/remoting/host/process_s...
remoting/host/process_stats_sender.cc:40: DCHECK(interval > base::TimeDelta());
On 2017/05/08 16:25:41, joedow wrote:
> On 2017/05/03 22:18:04, Hzj_jie wrote:
> > On 2017/05/03 21:41:01, joedow wrote:
> > > The !is_zero() check seems more readable, why the change?
> > 
> > Timer handles negative interval as 0:
> >
>
https://cs.chromium.org/chromium/src/base/timer/timer.cc?rcl=e6baa90f9f21576e...
> 
> I haven't experienced negative TimeDeltas being a problem but it seems
> reasonable (though less readable) to check for it.
> 
> Perhaps a cleaner approach would be to add a helper method to TimeDelta which
is
> 'is_positive()'.

Yes, and I also think silently accepting negative delay is not a right thing in
Timer. But unfortunately it looks like a very destructive change to disallow
negative delay in Timer.

https://codereview.chromium.org/2860803002/diff/20001/remoting/protocol/fake_...
File remoting/protocol/fake_process_stats_stub.h (right):

https://codereview.chromium.org/2860803002/diff/20001/remoting/protocol/fake_...
remoting/protocol/fake_process_stats_stub.h:25: void
set_expected_usage_count(size_t expected_usage_count);
On 2017/05/08 16:25:41, joedow wrote:
> On 2017/05/03 22:18:04, Hzj_jie wrote:
> > On 2017/05/03 21:41:02, joedow wrote:
> > > nit: trivial setters like this can be written inline.
> > 
> > Since we have a cc file anyway, I believe avoiding inline functions in
header
> > file should be preferred.
> 
> inline functions are fine for trivial getters:
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

But I believe it's not a must-have.
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i...
:)

[joedow, 2017-05-08 18:18:53]

https://codereview.chromium.org/2860803002/diff/20001/remoting/protocol/fake_...
File remoting/protocol/fake_process_stats_stub.h (right):

https://codereview.chromium.org/2860803002/diff/20001/remoting/protocol/fake_...
remoting/protocol/fake_process_stats_stub.h:25: void
set_expected_usage_count(size_t expected_usage_count);
On 2017/05/08 18:16:19, Hzj_jie wrote:
> On 2017/05/08 16:25:41, joedow wrote:
> > On 2017/05/03 22:18:04, Hzj_jie wrote:
> > > On 2017/05/03 21:41:02, joedow wrote:
> > > > nit: trivial setters like this can be written inline.
> > > 
> > > Since we have a cc file anyway, I believe avoiding inline functions in
> header
> > > file should be preferred.
> > 
> > inline functions are fine for trivial getters:
> >
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> 
> But I believe it's not a must-have.
>
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i...
> :)

That's fine, my comment was marked as a nit

[Hzj_jie, 2017-05-08 19:17:50]

Any other comments for this change?

[joedow, 2017-05-26 16:06:16]

I still have concerns over filtering nulls out in the c'tor when setting up the
agents instead of using an architecture which is more deterministic (i.e. agents
only register if they intend to send data and unregister when done).

[Hzj_jie, 2017-05-28 20:04:53]

On 2017/05/26 16:06:16, joedow wrote:
> I still have concerns over filtering nulls out in the c'tor when setting up
the
> agents instead of using an architecture which is more deterministic (i.e.
agents
> only register if they intend to send data and unregister when done).

According to the setup of the host, a DesktopEnvironment (IpcDesktopEnvironment)
may return a ProcessStatsAgent to receive statistics from another process or
nullptr if no other process is created. So we always need to decide whether it's
a nullptr or not, either in ClientSession or in ProcessStatsSender. Personally I
do not see a huge difference between these two solutions.

[joedow, 2017-05-30 15:20:26]

I'm not concerned about where the null checking code goes but rather why we need
it in the first place.

Let's talk through your design when you get in.