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

chrome/browser/task_management/providers/task.h

Index: chrome/browser/task_management/providers/task.h
diff --git a/chrome/browser/task_management/providers/task.h b/chrome/browser/task_management/providers/task.h
new file mode 100644
index 0000000000000000000000000000000000000000..0d7ab3da8ee8c8c920dbed8dbcc6a5544373b881
--- /dev/null
+++ b/chrome/browser/task_management/providers/task.h
@@ -0,0 +1,101 @@
+// 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_MANAGEMENT_PROVIDERS_TASK_H_
+#define CHROME_BROWSER_TASK_MANAGEMENT_PROVIDERS_TASK_H_
+
+#include "base/basictypes.h"
+#include "base/process/process_handle.h"
+#include "base/strings/string16.h"
+#include "base/time/time.h"
+#include "third_party/WebKit/public/web/WebCache.h"
+#include "ui/gfx/image/image_skia.h"
+
+namespace task_management {
+
+// Defines a task that corresponds to a tab, an app, an extension, ... etc. It
+// represents one row in the task manager table. Multiple tasks can share the
+// same process, in which case they're grouped together in the task manager
+// table. See |task_management::TaskGroup| which represents a process possibly
+// shared by multiple tasks.
+class Task {
+ public:
+  enum Type {
+    UNKNOWN = 0,
+    BROWSER,        /* The main browser process. */
+    RENDERER,       /* A normal WebContents renderer process. */
+    EXTENSION,      /* An extension or app process. */
+    NOTIFICATION,   /* A notification process. */

[Lei Zhang, 2015/04/01 22:47:51]

Is this a thing? I'm just curious what it is.

[afakhry, 2015/04/03 01:51:01]

I think ncarter@ can answer this question better than me. This was in the old
task manager and was taken as is. I've never seen a notification process.

[ncarter (slow), 2015/04/03 03:32:00]

We can remove this. jam@ deleted the balloon notification code about a year ago;
we just missed this spot. [ see https://codereview.chromium.org/231723006 ]

[afakhry, 2015/04/03 16:04:26]

Done.

+    GUEST,          /* A browser plugin guest process. */
+    PLUGIN,         /* A plugin process. */
+    WORKER,         /* A web worker process. */
+    NACL,           /* A NativeClient loader or broker process. */
+    UTILITY,        /* A browser utility process. */
+    ZYGOTE,         /* A Linux zygote process. */
+    SANDBOX_HELPER, /* A sandbox helper process. */
+    GPU,            /* A graphics process. */
+  };
+
+  Task(const base::string16& title,

[Lei Zhang, 2015/04/01 22:47:51]

Document parameters.

[afakhry, 2015/04/03 01:51:01]

Done.

+         const gfx::ImageSkia& icon,

[Lei Zhang, 2015/04/01 22:47:50]

nit: indentation.

[afakhry, 2015/04/03 01:51:01]

Done.

+         base::ProcessHandle handle);
+  virtual ~Task();
+
+  // Returns the task type.
+  virtual Type GetType() const = 0;
+
+  // This is the unique ID of the BrowserChildProcessHost/RenderProcessHost. It

[Lei Zhang, 2015/04/01 22:47:51]

What does this provide over process_handle() ?

[afakhry, 2015/04/03 01:51:01]

It's totally different. As the comment says it's not the PID nor the process
handle.

It's the unique ID of the process host instance.

[Lei Zhang, 2015/04/03 02:39:37]

Is the process host to not a 1:1 mapping with the process handle, with some easy
way to look up one from the other? I just don't understand how this will be
used, so I'm wondering why it's needed. Is it just here because we need this
info often so it's provided for convenience?

[afakhry, 2015/04/03 16:04:26]

It is here for convenience indeed, we'll need this often when we refresh the
task's NaCl Gdb debug stub port.

+  // is not the PID nor the handle of the process.
+  virtual int GetChildProcessUniqueID() const = 0;

[Lei Zhang, 2015/04/01 22:47:51]

Is the browser a "child" process? Are all return values valid?

[afakhry, 2015/04/03 01:51:01]

In the case of the browser process, the return value is 0 since there's no
browser process host.

But for renderers and other child processes which they have hosts in the browser
process with valid unique IDs we return those.

[Lei Zhang, 2015/04/03 02:39:37]

Can you document this in the comments? What is the valid range for IDs? Is 0
valid? -1?

[afakhry, 2015/04/03 16:04:26]

Done.

+
+  virtual base::string16 GetProfileName() const;

[Lei Zhang, 2015/04/01 22:47:50]

By making this and the following virtual methods const, you are restricting what
implementations can do. For example, if getting the profile name requires an one
time expensive operation, and the value can be cached for later queries, that's
not possibly if the method is const.

[Lei Zhang, 2015/04/01 22:47:51]

Can you clarify what "profile" means in this case?

[afakhry, 2015/04/03 01:51:01]

I understand, in this case however this will only be set once upon the
construction of a renderer task and it will never change afterwards.

[afakhry, 2015/04/03 01:51:01]

Done.

+
+  virtual int GetRoutingID() const;

[Lei Zhang, 2015/04/01 22:47:51]

What is the "routing id" exactly? There are several classes that all hand out
routing ids, but they are not interchangable.

[afakhry, 2015/04/03 01:51:01]

It's the routing ID of the render view host if this task represents a renderer,
otherwise it will return 0.

A comment has been added.

+
+  // Returns the Sqlite used memory (in bytes). Not all tasks reports Sqlite
+  // memory, in this case a default invalid value of -1U will be returned.

[Lei Zhang, 2015/04/01 22:47:51]

Why can't the invalid value just be 0?

[afakhry, 2015/04/03 01:51:01]

0 is a valid value for memory usage.

+  virtual size_t GetSqliteMemoryUsed() const;

[Lei Zhang, 2015/04/01 22:47:51]

Why do you have ReportsWebCacheStats(), but no ReportsSqliteMemoryUsed() ?

[afakhry, 2015/04/03 01:51:01]

In SqliteMemory we augmented this check in the return value by checking for -1U.

We can't do this however in WebCache stats since it's a large object with many
fields.

[Lei Zhang, 2015/04/03 02:39:37]

So why not do ReportsFoo() / GetFoo() for all types for consistency, and forgo
the sentinel value? Then the caller code can look like:

if (ReportsFoo())
  foo = GetFoo()
if (ReportsBar())
  bar = GetBar()

instead of:

if (ReportsFoo())
  foo = GetFoo()
bar = GetBar()
if (bar == -1)
  // handle invalid case

[afakhry, 2015/04/03 16:04:26]

Done.

+
+  // Returns the allocated and used V8 memory (in bytes). Not all tasks reports
+  // V8 memory, in this case a default invalid value of -1U will be returned.
+  virtual size_t GetV8MemoryAllocated() const;
+  virtual size_t GetV8MemoryUsed() const;
+
+  // Checking if the task reports Webkit resource cache statistics and getting
+  // them if it does.
+  virtual bool ReportsWebCacheStats() const;
+  virtual blink::WebCache::ResourceTypeStats GetWebCacheStats() const;
+
+  // Will be called to let the task refresh itself between refresh cycles.

[Lei Zhang, 2015/04/01 22:47:51]

Can you put Refresh() and OnBytesRead() on top? Right now it's a section of
getters, these methods, and more getters.

[afakhry, 2015/04/03 01:51:01]

Done.

+  // |update_interval| is the time since the last task manager refresh.
+  virtual void Refresh(const base::TimeDelta& update_interval);
+
+  // Will receive this notification through the task manager from
+  // |ChromeNetworkDelegate::OnRawBytesRead()|. The task will add to the
+  // |current_byte_count_| in this refresh cycle.
+  void OnBytesRead(int64 bytes_read);

[Lei Zhang, 2015/04/01 22:47:50]

Maybe OnNetworkBytesRead() ?

[afakhry, 2015/04/03 01:51:01]

Done.

+
+  int64 network_usage() const { return network_usage_; }

[Lei Zhang, 2015/04/01 22:47:50]

You should document these too. For example, is this return value in bytes? Does
-1 indicate an error?

[afakhry, 2015/04/03 01:51:00]

I added comments to the fields which they get.

+  int64 task_id() const { return task_id_; }
+  base::string16 title() const { return title_; }

[Lei Zhang, 2015/04/01 22:47:50]

You can return a const base::string16& for the non-POD return types.

[afakhry, 2015/04/03 01:51:01]

Done.

[Lei Zhang, 2015/04/03 02:39:37]

Same goes for icon() and process_handle().

[afakhry, 2015/04/03 16:04:26]

Done.

+  gfx::ImageSkia icon() const { return icon_; }
+  base::ProcessHandle process_handle() const { return process_handle_; }
+
+ private:
+  static int64 last_id_;

[Lei Zhang, 2015/04/01 22:47:51]

Just put this in an anonymous namespace in the .cc file as |g_last_id|, unless
you need some friendly test to be able to access this variable directly.

[afakhry, 2015/04/03 01:51:01]

Done.

+
+  // This is the unique ID of this task.
+  int64 task_id_;
+  int64 network_usage_;
+  int64 current_byte_count_;
+  base::string16 title_;

[Lei Zhang, 2015/04/01 22:47:51]

These never change, make them const?

[afakhry, 2015/04/03 01:51:01]

Done.

[Lei Zhang, 2015/04/03 02:39:37]

Can |title_| and |icon_| ever change?

[ncarter (slow), 2015/04/03 04:09:03]

Probably. A page can change its title (or favicon) and these definitely ought to
show up in the taskmanager. Historically we did that via tearing down and
recreating the Task, but afaik that was out of laziness.

+  gfx::ImageSkia icon_;
+  base::ProcessHandle process_handle_;
+
+  DISALLOW_COPY_AND_ASSIGN(Task);
+};
+
+}  // namespace task_management
+
+#endif  // CHROME_BROWSER_TASK_MANAGEMENT_PROVIDERS_TASK_H_