[Chromoting] Allow ProcessStatsSender to receive nullptr ProcessStatsAgent

remoting/host/process_stats_sender.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/host/process_stats_sender.h"
 
 #include <utility>
 
 #include "base/location.h"
 #include "base/logging.h"
 #include "remoting/host/process_stats_agent.h"
 #include "remoting/host/process_stats_util.h"
 
 namespace remoting {
 
+namespace {
+
+std::vector<ProcessStatsAgent*> RemoveNullAgent(
+    std::initializer_list<ProcessStatsAgent*> agents) {
+  std::vector<ProcessStatsAgent*> result;
+  for (auto* const agent : agents) {
+    if (agent) {
+      result.push_back(agent);
+    }
+  }
+  return result;
+}
+
+}  // namespace
+
 ProcessStatsSender::ProcessStatsSender(
     protocol::ProcessStatsStub* host_stats_stub,
     base::TimeDelta interval,
     std::initializer_list<ProcessStatsAgent*> agents)
     : host_stats_stub_(host_stats_stub),
-      agents_(agents),
+      agents_(RemoveNullAgent(agents)),

[joedow, 2017/05/03 21:41:01]

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.

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

:)

       thread_checker_() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(host_stats_stub_);
-  DCHECK(!interval.is_zero());
+  DCHECK(interval > base::TimeDelta());

[joedow, 2017/05/03 21:41:01]

The !is_zero() check seems more readable, why the change?

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

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

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

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()'.

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

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.

   DCHECK(!agents_.empty());
 
   timer_.Start(FROM_HERE, interval, this, &ProcessStatsSender::ReportUsage);
 }
 
 ProcessStatsSender::~ProcessStatsSender() {
   DCHECK(thread_checker_.CalledOnValidThread());
   timer_.Stop();
 }
 
 void ProcessStatsSender::ReportUsage() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   std::vector<protocol::ProcessResourceUsage> usages;
   for (auto* const agent : agents_) {
     DCHECK(agent);
     protocol::ProcessResourceUsage usage = agent->GetResourceUsage();
     if (!IsEmptyProcessResourceUsage(usage)) {
       usages.push_back(std::move(usage));
     }
   }
 
   host_stats_stub_->OnProcessStats(AggregateProcessResourceUsage(usages));
 }
 
 }  // namespace remoting