plumb network upload into the task manager

chrome/browser/task_manager/providers/task.cc

Index: chrome/browser/task_manager/providers/task.cc
diff --git a/chrome/browser/task_manager/providers/task.cc b/chrome/browser/task_manager/providers/task.cc
index a49032f6875cd6612cc2777b5606b34ff56d9356..950c958f4959d4299a1b143d08415ca121886b66 100644
--- a/chrome/browser/task_manager/providers/task.cc
+++ b/chrome/browser/task_manager/providers/task.cc
@@ -30,8 +30,12 @@ Task::Task(const base::string16& title,
            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()),
@@ -73,21 +77,44 @@ void Task::Refresh(const base::TimeDelta& update_interval,
   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;
+  }
+
+  cumulative_bytes_read_ += bytes_read;
+}
+
+void Task::OnNetworkBytesSent(int64_t bytes_sent) {
+  if (cumulative_bytes_sent_ == -1) {
+    cumulative_bytes_sent_ = 0;
+  }
 
-  current_byte_count_ += bytes_read;
+  cumulative_bytes_sent_ += bytes_sent;
 }
 
 void Task::GetTerminationStatus(base::TerminationStatus* out_status,
@@ -147,8 +174,10 @@ 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