New Task Manager - Phase 1.1: Implement Browser Process Task Providing

chrome/browser/task_management/private/tasks_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_management/private/tasks_providers/task.h"
+
+namespace task_management {
+
+namespace {
+
+int64 one_second_time_delta = base::TimeDelta::FromSeconds(1).InSeconds();

[ncarter (slow), 2015/03/27 18:16:21]

This is a pretty fancy way of saying "int64 var = 1;" :)

One of the reasons we have a TimeDelta class, is to lessen the risk of
units-conversion bugs that crop up when you store time intervals in ints. I
think you don't need this constant.

[afakhry, 2015/03/31 00:19:20]

Removed.

+
+}  // namespace
+
+
+// ----------------------------------------------------------------------
+// Public Members:
+// ----------------------------------------------------------------------
+
+base::string16 Task::GetProfileName() const {
+  return base::string16();
+}
+
+int Task::GetRoutingID() const {
+  return 0;
+}
+
+size_t Task::GetSqliteMemoryUsed() const {
+  return -1U;
+}
+
+size_t Task::GetV8MemoryAllocated() const {
+  return -1U;
+}
+
+size_t Task::GetV8MemoryUsed() const {
+  return -1U;
+}
+
+bool Task::ReportsWebCacheStats() const {
+  return false;
+}
+
+blink::WebCache::ResourceTypeStats Task::GetWebCacheStats() const {
+  return blink::WebCache::ResourceTypeStats();
+}
+
+void Task::Refresh(const base::TimeDelta& update_time) {

[ncarter (slow), 2015/03/27 18:16:21]

I'd probably call this variable sampling_interval or update_duration, since it's
an interval.

[afakhry, 2015/03/31 00:19:20]

Done.

+  // TODO(afakhry): Add code here to skip this when network usage refresh has
+  // never been requested.
+
+  int64 update_time_secs = update_time.InSeconds();
+  if (update_time_secs > one_second_time_delta)
+    network_usage_ = current_byte_count_ / update_time_secs;
+  else
+    network_usage_ = current_byte_count_ * (1 / update_time_secs);

[ncarter (slow), 2015/03/27 18:16:21]

This math seems buggy. The else branch is executed if update_time_secs has the
integer value 1 or 0, in which cases the result is identical to the if branch.
What were you trying to do?

Would you be better off using floating point, like:

network_usage_ = static_cast<int>(current_byte_count /
update_time.InSecondsF());

[afakhry, 2015/03/31 00:19:20]

Fixed.

+
+  // Reset the current byte count for this resource.
+  current_byte_count_ = 0;
+}
+
+void Task::OnBytesRead(int64 bytes_read) {
+  current_byte_count_ += bytes_read;
+}
+
+// ----------------------------------------------------------------------
+// Protected Members:
+// ----------------------------------------------------------------------

[ncarter (slow), 2015/03/27 18:16:21]

Remove this comment.

+
+Task::Task(const base::string16& title,
+           const gfx::ImageSkia& icon,
+           base::ProcessHandle handle)
+    : network_usage_(0),
+      current_byte_count_(0),
+      task_id_(last_id_++),
+      title_(title),
+      icon_(icon),

[ncarter (slow), 2015/03/27 18:16:21]

Just mentioning it in case you didn't know: copying ImageSkias like this is
supposed to be very fast, since it'll just take a reference to the underlying
pixel content.

[afakhry, 2015/03/31 00:19:19]

Yep, I already know that, but thanks a lot for making sure! :) 

+      process_handle_(handle),
+      is_first_in_group_(true) {

[ncarter (slow), 2015/03/27 18:16:21]

"is_first_in_group_" doesn't make sense to me as an explicit data member of a
Task. Since Tasks that share a group have independent lifetimes, when the first
task in a group goes away, the second one needs to be updated, so you'd need a
mechanism (e.g. a list of tasks in a group) of finding the second one. And so,
if you have such a mechanism, and want to know what the first task in the group
is, why not just look at the list?

[afakhry, 2015/03/31 00:19:20]

This is the responsibility of the TaskGroup in the future. Upon addition /
removal of tasks from their group this field will be updated properly.

+}
+
+Task::~Task() {
+}
+
+// ----------------------------------------------------------------------
+// Private Members:
+// ----------------------------------------------------------------------

[ncarter (slow), 2015/03/27 18:16:21]

Remove this comment.

+
+// static
+size_t Task::last_id_ = 0;
+
+}  // namespace task_management