plumb network upload into the task manager

chrome/browser/task_manager/providers/task.cc

 // Copyright 2015 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 "chrome/browser/task_manager/providers/task.h"
 
 #include <stddef.h>
 
 #include "base/process/process.h"
 #include "chrome/browser/browser_process.h"
@@ skipping 12 matching lines @@
 int64_t g_last_id = 0;
 
 }  // namespace
 
 Task::Task(const base::string16& title,
            const std::string& rappor_sample,
            const gfx::ImageSkia* icon,
            base::ProcessHandle handle,
            base::ProcessId process_id)
     : task_id_(g_last_id++),
-      network_usage_(-1),
-      current_byte_count_(-1),
+      last_refresh_cumulative_bytes_sent_(0),
+      last_refresh_cumulative_bytes_read_(0),
+      cumulative_bytes_sent_(-1),

[ncarter (slow), 2017/06/14 20:20:58]

I have a hunch that this code would come out easier if we started
cumulative_bytes_sent_ and cumulative_bytes_read_ off at zero. Then we're in a
state where we never actually have to store the -1 sentinel value, which means
we don't have to special case that value.

The original meaning of a return value of "-1" vs "0" for the rate getter is so
that the caller can distinguish between a task that's never had any network
usage reported, and a task that just happens to not have any data in the last
refresh cycle. If that's important (and it's not clear to me that it is!), we
can just do something like this in the rate getter:

 return ReportsNetworkUsage() ? network_sent_rate_ + network_read_rate_ : -1;

And I think that would be the only place in this class where we have to deal
with the -1 special value.

ReportsNetworkUsage() can detect the "ever got any bytes" case like this:

  return last_refresh_cumulative_bytes_sent_ ||
last_refresh_cumulative_bytes_read_;

[cburn, 2017/06/16 21:52:26]

I agree that this makes more sense. I am unsure if ReportsNetworkUsage() should
be tied to last_refresh_cumulative_bytes_read_ or cumulative_bytes_read_ I am
leaning towards the last_refresh_cumulative_bytes_read_ because in the old
version it is tied into network_usage_ which is only ever updated after a
Refresh. 

+      cumulative_bytes_read_(-1),
+      network_sent_rate_(0),
+      network_read_rate_(0),
       title_(title),
       rappor_sample_name_(rappor_sample),
       icon_(icon ? *icon : gfx::ImageSkia()),
       process_handle_(handle),
       process_id_(process_id != base::kNullProcessId
                       ? process_id
                       : base::GetProcId(handle)) {}
 
 Task::~Task() {}
 
@@ skipping 21 matching lines @@
   DCHECK_NE(process_id(), base::GetCurrentProcId());
   base::Process process = base::Process::Open(process_id());
   process.Terminate(content::RESULT_CODE_KILLED, false);
 }
 
 void Task::Refresh(const base::TimeDelta& update_interval,
                    int64_t refresh_flags) {
   if ((refresh_flags & REFRESH_TYPE_NETWORK_USAGE) == 0)
     return;
 
-  if (current_byte_count_ == -1)
+  if (cumulative_bytes_sent_ == -1 && cumulative_bytes_read_ == -1)
     return;
 
-  network_usage_ =
-      (current_byte_count_ * base::TimeDelta::FromSeconds(1)) / update_interval;
+  int64_t current_cycle_read_byte_count =
+      cumulative_bytes_read_ - last_refresh_cumulative_bytes_read_;
+  network_read_rate_ = 0;
+  if (cumulative_bytes_read_ != -1) {
+    network_read_rate_ =
+        (current_cycle_read_byte_count * base::TimeDelta::FromSeconds(1)) /
+        update_interval;
+  }
 
-  // Reset the current byte count for this task.
-  current_byte_count_ = 0;
+  int64_t current_cycle_sent_byte_count =
+      cumulative_bytes_sent_ - last_refresh_cumulative_bytes_sent_;
+  network_sent_rate_ = 0;
+  if (cumulative_bytes_sent_ != -1) {
+    network_sent_rate_ =
+        (current_cycle_sent_byte_count * base::TimeDelta::FromSeconds(1)) /
+        update_interval;
+  }
+  last_refresh_cumulative_bytes_read_ = cumulative_bytes_read_;
+  last_refresh_cumulative_bytes_sent_ = cumulative_bytes_sent_;
 }
 
 void Task::OnNetworkBytesRead(int64_t bytes_read) {
-  if (current_byte_count_ == -1)
-    current_byte_count_ = 0;
+  if (cumulative_bytes_read_ == -1) {
+    cumulative_bytes_read_ = 0;
+  }
 
-  current_byte_count_ += bytes_read;
+  cumulative_bytes_read_ += bytes_read;
+}
+
+void Task::OnNetworkBytesSent(int64_t bytes_sent) {
+  if (cumulative_bytes_sent_ == -1) {
+    cumulative_bytes_sent_ = 0;
+  }
+
+  cumulative_bytes_sent_ += bytes_sent;
 }
 
 void Task::GetTerminationStatus(base::TerminationStatus* out_status,
                                 int* out_error_code) const {
   DCHECK(out_status);
   DCHECK(out_error_code);
 
   *out_status = base::TERMINATION_STATUS_STILL_RUNNING;
   *out_error_code = 0;
 }
@@ skipping 39 matching lines @@
 }
 
 blink::WebCache::ResourceTypeStats Task::GetWebCacheStats() const {
   return blink::WebCache::ResourceTypeStats();
 }
 
 int Task::GetKeepaliveCount() const {
   return -1;
 }
 
+// If either bytes have been sent or bytes have been read by the task
+// the network has been used

[ncarter (slow), 2017/06/14 20:20:58]

I think this comment is probably better moved to inside the {}'s, since it's
really explaining the meaning of the expression.

[cburn, 2017/06/16 21:52:26]

Deleted since we removed the -1 flag. 

 bool Task::ReportsNetworkUsage() const {
-  return network_usage_ != -1;
+  return cumulative_bytes_read_ != -1 || cumulative_bytes_sent_ != -1;
 }
 
 }  // namespace task_manager