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

chrome/browser/task_management/private/tasks_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_MANAGEMENT_PRIVATE_TASKS_PROVIDERS_TASK_H_
+#define CHROME_BROWSER_TASK_MANAGEMENT_PRIVATE_TASKS_PROVIDERS_TASK_H_
+
+#include "base/basictypes.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.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

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

multiple -> Multiple

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

Done.

+// same process, in which case they're grouped together in the task manager
+// table. See |task_management::TaskGroup| which represents a process possibly

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

TaskGroup doesn't exist right now.

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

I know but it will. Do you want me to remove that comment until it does?

+// shared by multiple tasks.
+//
+// A Task is RefCountedThreadSafe and hence refrences have to be scoped_refptr.

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

"refrences" typo

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

Done.

+class Task : public base::RefCountedThreadSafe<Task> {

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

What lifetime problem does reference counting solve here?

I am not yet convinced that tasks need to be RefCounted, let alone
RefCountedThreadSafe. In the old TM the Resources didn't need to be RefCounted
(the ResourceProviders were, and I'm also not sure they needed to be).

Tasks, in my mind, ought to be owned either by the TaskProvider or else the
TaskManager, and don't need to be thread safe.

Refcounting extends the lifetime of an object until all owners go away. Because
side-effects of destruction are very common in C++, this can lead to these
side-effects being triggered at inconsistent and unexpected times, since the
timing depends on the destruction order of the objects that own the refptr. And
when you consider threading, most objects don't really want their dtors to run
on threads other than the thread they're created on. A good chunk of the bugs
I've encountered in Google Chrome had a situation like this as one of the
aggravating factors. There is a place for refcounting, but I have come to prefer
explicit ownership (scoped_ptr members in the owner, rawptr references
on-thread, and WeakPtr references when bound to closures that will run async)
whenever possible, just like I've come to prefer single-threaded objects.

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

Removed as agreed.

+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. */
+    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. */
+  };
+
+  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.
+  virtual int GetChildProcessUniqueID() const = 0;
+
+  virtual base::string16 GetProfileName() const;
+
+  virtual int GetRoutingID() const;
+
+  // 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.
+  virtual size_t GetSqliteMemoryUsed() const;
+
+  // 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;
+
+  virtual bool ReportsWebCacheStats() const;
+  virtual blink::WebCache::ResourceTypeStats GetWebCacheStats() const;

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

These merit a comment.

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

Done.

+
+  // Will be called to let the task refresh itself between refresh cycles.
+  // |update_time| is the time since the last task manager refresh.
+  virtual void Refresh(const base::TimeDelta& update_time);
+
+  // 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);
+
+  int64 network_usage() const { return network_usage_; }
+  size_t task_id() const { return task_id_; }
+  base::string16 title() const { return title_; }
+  gfx::ImageSkia icon() const { return icon_; }
+  base::ProcessHandle process_handle() const { return process_handle_; }
+  bool is_first_in_group() const { return is_first_in_group_; }
+  void set_is_first_in_group(bool val) { is_first_in_group_ = val; }
+
+protected:

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

public:, protected: and private: declarations get a 1-space indent.

https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

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

Yes I know that. I'm not sure why they weren't formated properly. Thanks!

Done!

+  friend class base::RefCountedThreadSafe<Task>;
+
+  Task(const base::string16& title,
+       const gfx::ImageSkia& icon,
+       base::ProcessHandle handle);
+  virtual ~Task();
+
+// ----------------------------------------------------------------------
+// Fields:
+// ----------------------------------------------------------------------

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

Not needed.

+  int64 network_usage_;
+  int64 current_byte_count_;
+  size_t task_id_;

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

It is encouraged (but not required) to comment your data member declarations.
Not all of these need it  ... but here I'd say it's worth explaining what
task_id_, means since often there are all kinds of int IDs floating around, and
it's easy to confuse them.

Also: I think this shouldn't be a size_t; it should be int or int64. There's no
reason for it to behave differently on 32 bit vs. 64 bit builds.

+  base::string16 title_;
+  gfx::ImageSkia icon_;
+  base::ProcessHandle process_handle_;
+  bool is_first_in_group_;

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

These data members should all be private. Derived classes should use getters and
setters as needed.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Access_Control

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

Done.

+
+private:
+  static size_t last_id_;

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

This approach (a static ID) means that tasks can only be minted on the UI
thread, which might be further evidence that Task doesn't need to be
RefCountedThreadSafe.

+
+  DISALLOW_COPY_AND_ASSIGN(Task);
+};
+
+}  // namespace task_management
+
+#endif  // CHROME_BROWSER_TASK_MANAGEMENT_PRIVATE_TASKS_PROVIDERS_TASK_H_