plumb network upload into the task manager

chrome/browser/task_manager/providers/task.h

Index: chrome/browser/task_manager/providers/task.h
diff --git a/chrome/browser/task_manager/providers/task.h b/chrome/browser/task_manager/providers/task.h
index a3e91ceeeb21cf116c49c285f3bac1ca7f0957a6..3b9829f517f34124e2d16ea22ce7c0cc9575ff68 100644
--- a/chrome/browser/task_manager/providers/task.h
+++ b/chrome/browser/task_manager/providers/task.h
@@ -85,9 +85,14 @@ class Task {
 
   // Will receive this notification through the task manager from
   // |ChromeNetworkDelegate::OnNetworkBytesReceived()|. The task will add to the
-  // |current_byte_count_| in this refresh cycle.
+  // |current_read_byte_count_| in this refresh cycle.

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

|current_read_byte_count_| no longer exists.

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

Done. Changed to |cumulative_read_bytes_| as is done in the function

   void OnNetworkBytesRead(int64_t bytes_read);
 
+  // Will receive this notification through the task manager from
+  // |ChromeNetworkDelegate::OnNetworkBytesSent()|. The task will add to the
+  // |current_sent_byte_count_| in this refresh cycle.

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

|current_sent_byte_count_| no longer exists.

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

Done. Changed to |cumulative_sent_bytes_| as is done in the function

+  void OnNetworkBytesSent(int64_t bytes_sent);
+
   // Returns the task type.
   virtual Type GetType() const = 0;
 
@@ -142,7 +147,21 @@ class Task {
   bool ReportsNetworkUsage() const;
 
   int64_t task_id() const { return task_id_; }
-  int64_t network_usage() const { return network_usage_; }
+

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

// Returns the instantaneous rate, in bytes per second, of network usage (sent
and recieved), as measured over the last refresh cycle.

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

Agreed and added.

+  int64_t network_usage_rate() const {
+    if (cumulative_bytes_sent_ == -1 && cumulative_bytes_read_ == -1)
+      return -1;
+    return network_sent_rate_ + network_read_rate_;
+  }

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

These methods aren't quite trivial, so they should be declared in the .h file
and defined in the .cc file, per the rule here:
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i...


However, if you take my later suggestion of not starting cumulative_bytes_read_
off at -1, this method probably becomes trivial enough (cumulative_bytes_sent_ +
cumulative_bytes_read_) to keep in the .h file.

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

I agree with the later suggestion and simplified these based on the impact of
the changes.

+

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

// Returns the cumulative number of bytes of network usage (sent and received)
since this task was created. This value is monotonically increasing over the
lifetime of the task.

... and depending on how you resolve my next comment, maybe clarify to add
something like:

This value is updated each time Refresh() is called.

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

I have kept it to be as up to date as possible. I looked at it with the last
refresh as what is returned and the naming conventions just felt off to me. I am
not sold that the most up to date way is the best way to do this, but the names
"feel" more correct as this way. 

+  int64_t cumulative_network_usage() const {
+    int64_t cumulative_bytes_read =
+        cumulative_bytes_read_ == -1 ? 0 : cumulative_bytes_read_;
+    int64_t cumulative_bytes_sent =
+        cumulative_bytes_sent_ == -1 ? 0 : cumulative_bytes_sent_;
+    return cumulative_bytes_sent + cumulative_bytes_read;

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

Should these be based on cumulative_bytes_read_ or
last_refresh_cumulative_bytes_read_ ? It's not actually totally clear to me. We
could prefer the freshest values, or we could provide a stable value that only
updates at Refresh() time. I could go either way.

What do you think?

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

I think it should be the most up to date information we can provide, though I
think use is what matters too. If this were to be used to display information in
the task manager in the future I think switching to
last_refresh_cumulative_bytes_* make more sense because it will be in sync with
the refreshes on the task manager.
 
Also from the names of the variables I think it makes more sense to keep it
aligned with what might be expected from just reading the names.

I could see adding in a getter for the last_refresh_cumulative_ for testing as
well. 

+  }
+
   const base::string16& title() const { return title_; }
   const std::string& rappor_sample_name() const { return rappor_sample_name_; }
   const gfx::ImageSkia& icon() const { return icon_; }
@@ -160,14 +179,23 @@ class Task {
   // The unique ID of this task.
   const int64_t task_id_;
 
-  // The task's network usage in the current refresh cycle measured in bytes per
-  // second. A value of -1 means this task doesn't report network usage data.
-  int64_t network_usage_;
+  // Keeps track of previous number of bytes sent on a refresh
+  int64_t last_refresh_cumulative_bytes_sent_;
+
+  // Keeps track of previous number of bytes read on a refresh
+  int64_t last_refresh_cumulative_bytes_read_;
+
+  // Keeps track of current number of bytes sent on a refresh

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

After you finalize the code comment for cumulative_network_usage(), use the same
language down here.

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

Updated the language. 

+  int64_t cumulative_bytes_sent_;
+
+  // Keeps track of current number of bytes read on a refresh
+  int64_t cumulative_bytes_read_;
+
+  // Keeps track of the current network reading rate
+  int64_t network_sent_rate_;
 
-  // The current network bytes received by this task during the current refresh
-  // cycle. A value of -1 means this task has never been notified of any network
-  // usage.
-  int64_t current_byte_count_;
+  // Keeps track of the current network sending rate

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

In all of these comments, the "Keeps track of" can be dropped. Match the style
of the existing data member comments, including ending with a period.

For this one, I'd write something like:

// The download rate (in bytes per second) for this task during the most recent
refresh window.

Often it's useful to phrase comments in terms alternate to those used in the
variable name (e.g. "download" instead of "read"), because otherwise you wind up
with comments like the following, that add no signal:

  // The network read rate.
  network_read_rate_;

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

Updated the comments to have more signals to the purpose of the variables. 

+  int64_t network_read_rate_;
 
   // The title of the task.
   base::string16 title_;