plumb network upload into the task manager

chrome/browser/task_manager/providers/task.h

 // 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.
 
 #ifndef CHROME_BROWSER_TASK_MANAGER_PROVIDERS_TASK_H_
 #define CHROME_BROWSER_TASK_MANAGER_PROVIDERS_TASK_H_
 
 #include <stdint.h>
 
 #include <string>
@@ skipping 67 matching lines @@
 
   // Will be called to let the task refresh itself between refresh cycles.
   // |update_interval| is the time since the last task manager refresh.
   // the |refresh_flags| indicate which resources should be calculated on each
   // refresh.
   virtual void Refresh(const base::TimeDelta& update_interval,
                        int64_t refresh_flags);
 
   // 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;
 
   // This is the unique ID of the BrowserChildProcessHost/RenderProcessHost. It
   // is not the PID nor the handle of the process.
   // For a task that represents the browser process, the return value is 0. For
   // other tasks that represent renderers and other child processes, the return
   // value is whatever unique IDs of their hosts in the browser process.
   virtual int GetChildProcessUniqueID() const = 0;
 
@@ skipping 34 matching lines @@
   virtual bool ReportsWebCacheStats() const;
   virtual blink::WebCache::ResourceTypeStats GetWebCacheStats() const;
 
   // Returns the keep-alive counter if the Task is an event page, -1 otherwise.
   virtual int GetKeepaliveCount() const;
 
   // Checking whether the task reports network usage.
   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_; }
   const base::ProcessHandle& process_handle() const { return process_handle_; }
   const base::ProcessId& process_id() const { return process_id_; }
 
  protected:
   void set_title(const base::string16& new_title) { title_ = new_title; }
   void set_rappor_sample_name(const std::string& sample) {
     rappor_sample_name_ = sample;
   }
   void set_icon(const gfx::ImageSkia& new_icon) { icon_ = new_icon; }
 
  private:
   // 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_;
 
-  // 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 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_;
+
+  // 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_;
 
   // The name of the sample representing this task when a Rappor sample needs to
   // be recorded for it.
   std::string rappor_sample_name_;
 
   // The favicon.
   gfx::ImageSkia icon_;
 
   // The handle of the process on which this task is running.
   const base::ProcessHandle process_handle_;
 
   // The PID of the process on which this task is running.
   const base::ProcessId process_id_;
 
   DISALLOW_COPY_AND_ASSIGN(Task);
 };
 
 }  // namespace task_manager
 
 #endif  // CHROME_BROWSER_TASK_MANAGER_PROVIDERS_TASK_H_