Make old task manager tests work against new task manager.

chrome/browser/task_manager/task_manager_browsertest_util.cc

Index: chrome/browser/task_manager/task_manager_browsertest_util.cc
diff --git a/chrome/browser/task_manager/task_manager_browsertest_util.cc b/chrome/browser/task_manager/task_manager_browsertest_util.cc
index edeccd0f4a2dc840a22a4dd3433dfa79aa03c7b7..fe809b20a65188bc4dba0cd09264f713f45f8f0c 100644
--- a/chrome/browser/task_manager/task_manager_browsertest_util.cc
+++ b/chrome/browser/task_manager/task_manager_browsertest_util.cc
@@ -4,8 +4,10 @@
 
 #include "chrome/browser/task_manager/task_manager_browsertest_util.h"
 
+#include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/location.h"
+#include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/pattern.h"
@@ -17,40 +19,265 @@
 #include "base/timer/timer.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sessions/session_tab_helper.h"
+#include "chrome/browser/task_management/task_manager_interface.h"
 #include "chrome/browser/task_manager/resource_provider.h"
 #include "chrome/browser/task_manager/task_manager.h"
+#include "chrome/browser/ui/task_manager/task_manager_table_model.h"

[afakhry, 2016/04/28 00:03:13]

I don't see you changed any DEPS. Does that mean you don't have to?

[ncarter (slow), 2016/05/02 18:33:28]

It's ui/views (to get the view) that had the deps problem. The model is in
ui/task_manager.

[afakhry, 2016/05/02 18:58:17]

Oh cool. Glad I moved there earlier! :)

 #include "chrome/common/chrome_switches.h"
 #include "chrome/grit/generated_resources.h"
 #include "extensions/strings/grit/extensions_strings.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/base/models/table_model_observer.h"
+
+// TODO(nick): Move everything here (except for LegacyTaskManagerTesterImpl)
+// into the task_management namespace.
+namespace task_management {
+
+// Temporarily intercepts the calls between a TableModel and its Observer,
+// running |callback| whenever anything happens.
+class ScopedInterceptTableModelObserver : public ui::TableModelObserver {
+ public:
+  ScopedInterceptTableModelObserver(
+      ui::TableModel* model,

[afakhry, 2016/04/28 00:03:13]

The naming of the variables can be confusing. Would it be better to name it
|model_to_intercept|.

[ncarter (slow), 2016/05/02 18:33:28]

Done.

+      ui::TableModelObserver* real_table_model_observer,
+      const base::Closure& callback)
+      : model_(model),
+        real_table_model_observer_(real_table_model_observer),
+        callback_(callback) {
+    model_->SetObserver(this);
+  }
+
+  ~ScopedInterceptTableModelObserver() override {
+    model_->SetObserver(real_table_model_observer_);
+  }
+
+  // ui::TableModelObserver:
+  void OnModelChanged() override {
+    real_table_model_observer_->OnModelChanged();
+    callback_.Run();
+  }
+  void OnItemsChanged(int start, int length) override {
+    real_table_model_observer_->OnItemsChanged(start, length);
+    callback_.Run();
+  }
+  void OnItemsAdded(int start, int length) override {
+    real_table_model_observer_->OnItemsAdded(start, length);
+    callback_.Run();
+  }
+  void OnItemsRemoved(int start, int length) override {
+    real_table_model_observer_->OnItemsRemoved(start, length);
+    callback_.Run();
+  }
+
+ private:
+  ui::TableModel* model_;
+  ui::TableModelObserver* real_table_model_observer_;
+  base::Closure callback_;
+};
+
+class TaskManagerTesterImpl
+    : public task_manager::browsertest_util::TaskManagerTester {
+ public:
+  explicit TaskManagerTesterImpl(const base::Closure& on_resource_change)
+      : model_(GetRealModel()) {
+    // Eavesdrop the model->view conversation, since the model only supports
+    // single observation.
+    if (!on_resource_change.is_null()) {
+      interceptor_.reset(new ScopedInterceptTableModelObserver(
+          model_, model_->table_model_observer_, on_resource_change));
+    }
+  }
+
+  ~TaskManagerTesterImpl() override {
+    CHECK_EQ(GetRealModel(), model_) << "Task Manager should not be hidden "
+                                        "while TaskManagerTester is alive. "
+                                        "This indicates a test bug.";
+  }
+
+  // TaskManagerTester:
+  int GetRowCount() override { return model_->RowCount(); }
+
+  base::string16 GetRowTitle(int row) override {
+    return model_->GetText(row, IDS_TASK_MANAGER_TASK_COLUMN);
+  }
+
+  void ToggleColumnVisibility(
+      task_manager::browsertest_util::ColumnSpecifier column) override {
+    int column_id = 0;
+    switch (column) {
+      case task_manager::browsertest_util::COLUMN_NONE:
+        return;
+      case task_manager::browsertest_util::SQLITE_MEMORY_USED:
+        column_id = IDS_TASK_MANAGER_SQLITE_MEMORY_USED_COLUMN;
+        break;
+      case task_manager::browsertest_util::V8_MEMORY_USED:
+      case task_manager::browsertest_util::V8_MEMORY:
+        column_id = IDS_TASK_MANAGER_JAVASCRIPT_MEMORY_ALLOCATED_COLUMN;
+        break;
+    }
+    model_->ToggleColumnVisibility(column_id);
+  }
+
+  int64_t GetColumnValue(task_manager::browsertest_util::ColumnSpecifier column,
+                         int row) override {
+    TaskId task_id = model_->tasks_[row];
+    int64_t value = 0;
+    int64_t ignored = 0;
+    bool success = false;
+
+    switch (column) {
+      case task_manager::browsertest_util::COLUMN_NONE:
+        break;
+      case task_manager::browsertest_util::V8_MEMORY:
+        success = task_manager()->GetV8Memory(task_id, &value, &ignored);
+        break;
+      case task_manager::browsertest_util::V8_MEMORY_USED:
+        success = task_manager()->GetV8Memory(task_id, &ignored, &value);
+        break;
+      case task_manager::browsertest_util::SQLITE_MEMORY_USED:
+        value = task_manager()->GetSqliteMemoryUsed(task_id);
+        success = true;
+        break;
+    }
+    if (!success)
+      return 0;
+    return value;
+  }
+
+  int32_t GetTabId(int row) override {
+    TaskId task_id = model_->tasks_[row];
+    return task_manager()->GetTabId(task_id);
+  }
+
+  void Kill(int row) override { model_->KillTask(row); }
+
+ private:
+  TaskManagerInterface* task_manager() {
+    return model_->observed_task_manager();
+  }
+
+  // Returns the TaskManagerTableModel for the the visible NewTaskManagerView.
+  static TaskManagerTableModel* GetRealModel() {
+    // TODO(nick): Solve the following ui/views layering/deps problem:
+    // TaskManagerTableModel::GetInstanceForTesting() and its global variable
+    // only exist so that we can call it here. That's kind of lame. If we were
+    // allowed to #include ui/views here, we could befriend and call
+    // NewTaskManagerView::GetInstanceForTesting() and get the model from that.
+    TaskManagerTableModel* result =
+        TaskManagerTableModel::GetInstanceForTesting();
+    CHECK(result) << "Task Manager must be visible to get the model.";
+    return result;
+  }
+
+  TaskManagerTableModel* model_;
+  std::unique_ptr<ScopedInterceptTableModelObserver> interceptor_;
+};
+
+}  // namespace task_management
 
 namespace task_manager {
 namespace browsertest_util {
 
 namespace {
 
-class ResourceChangeObserver : public TaskManagerModelObserver {
+class LegacyTaskManagerTesterImpl : public TaskManagerTester,
+                                    public TaskManagerModelObserver {
  public:
-  ResourceChangeObserver(const TaskManagerModel* model,
-                         int required_count,
-                         const base::string16& title_pattern,
-                         ColumnSpecifier column_specifier,
-                         size_t min_column_value)
-      : model_(model),
-        required_count_(required_count),
-        title_pattern_(title_pattern),
-        column_specifier_(column_specifier),
-        min_column_value_(min_column_value) {}
+  explicit LegacyTaskManagerTesterImpl(const base::Closure& on_resource_change)
+      : on_resource_change_(on_resource_change),
+        model_(TaskManager::GetInstance()->model()) {
+    if (!on_resource_change_.is_null())
+      model_->AddObserver(this);
+  }
+  ~LegacyTaskManagerTesterImpl() override {
+    if (!on_resource_change_.is_null())
+      model_->RemoveObserver(this);
+  }
 
-  void OnModelChanged() override { OnResourceChange(); }
+  // TaskManagerTester:
+  int GetRowCount() override { return model_->ResourceCount(); }
 
-  void OnItemsChanged(int start, int length) override { OnResourceChange(); }
+  base::string16 GetRowTitle(int row) override {
+    return model_->GetResourceTitle(row);
+  }
 
-  void OnItemsAdded(int start, int length) override { OnResourceChange(); }
+  int64_t GetColumnValue(ColumnSpecifier column, int row) override {
+    size_t value = 0;
+    bool success = false;
+    switch (column) {
+      case COLUMN_NONE:
+        break;
+      case V8_MEMORY:
+        success = model_->GetV8Memory(row, &value);
+        break;
+      case V8_MEMORY_USED:
+        success = model_->GetV8MemoryUsed(row, &value);
+        break;
+      case SQLITE_MEMORY_USED:
+        success = model_->GetSqliteMemoryUsedBytes(row, &value);
+        break;
+    }
+    if (!success)
+      return 0;
+    return static_cast<int64_t>(value);
+  }
+
+  void ToggleColumnVisibility(ColumnSpecifier column) override {
+    // Doing nothing is okay here; the legacy TaskManager always collects all
+    // stats.
+  }
+
+  int32_t GetTabId(int row) override {
+    if (model_->GetResourceWebContents(row)) {
+      return SessionTabHelper::IdForTab(model_->GetResourceWebContents(row));
+    }
+    return -1;
+  }
 
+  void Kill(int row) override { TaskManager::GetInstance()->KillProcess(row); }
+
+  // TaskManagerModelObserver:
+  void OnModelChanged() override { OnResourceChange(); }
+  void OnItemsChanged(int start, int length) override { OnResourceChange(); }
+  void OnItemsAdded(int start, int length) override { OnResourceChange(); }
   void OnItemsRemoved(int start, int length) override { OnResourceChange(); }
 
+ private:
+  void OnResourceChange() {
+    if (!on_resource_change_.is_null())
+      on_resource_change_.Run();
+  }
+  base::Closure on_resource_change_;
+  TaskManagerModel* model_;
+};
+
+// Helper class to run a message loop until a TaskManagerTester is in an
+// expected state. If timeout occurs, an ASCII version of the task manager's
+// contents, along with a summary of the expected state, are dumped to test
+// output, to assist debugging.
+class ResourceChangeObserver {
+ public:
+  ResourceChangeObserver(int required_count,
+                         const base::string16& title_pattern,
+                         ColumnSpecifier column_specifier,
+                         size_t min_column_value)
+      : required_count_(required_count),
+        title_pattern_(title_pattern),
+        column_specifier_(column_specifier),
+        min_column_value_(min_column_value) {
+    base::Closure on_resource_change = base::Bind(
+        &ResourceChangeObserver::OnResourceChange, base::Unretained(this));
+
+    if (switches::NewTaskManagerEnabled()) {
+      model_.reset(

[afakhry, 2016/04/28 00:03:13]

There are too many things that are called |model_| in this file! :D
Can we call this |task_manager_tester_| instead?

[ncarter (slow), 2016/05/02 18:33:28]

Done.

[afakhry, 2016/05/02 18:58:17]

Mmmm, seems you missed that one?

[ncarter (slow), 2016/05/02 23:09:57]

Apparently I need to hit Ctrl+S before uploading. Thanks for catching that.
Done.

+          new task_management::TaskManagerTesterImpl(on_resource_change));
+    } else {
+      model_.reset(new LegacyTaskManagerTesterImpl(on_resource_change));
+    }
+  }
+
   void RunUntilSatisfied() {
     // See if the condition is satisfied without having to run the loop. This
     // check has to be placed after the installation of the
@@ -58,9 +285,7 @@ class ResourceChangeObserver : public TaskManagerModelObserver {
     if (IsSatisfied())
       return;
 
-    timer_.Start(FROM_HERE,
-                 TestTimeouts::action_timeout(),
-                 this,
+    timer_.Start(FROM_HERE, TestTimeouts::action_timeout(), this,
                  &ResourceChangeObserver::OnTimeout);
 
     run_loop_.Run();
@@ -91,8 +316,8 @@ class ResourceChangeObserver : public TaskManagerModelObserver {
 
   int CountMatches() {
     int match_count = 0;
-    for (int i = 0; i < model_->ResourceCount(); i++) {
-      if (!base::MatchPattern(model_->GetResourceTitle(i), title_pattern_))
+    for (int i = 0; i < model_->GetRowCount(); i++) {
+      if (!base::MatchPattern(model_->GetRowTitle(i), title_pattern_))
         continue;
 
       if (GetColumnValue(i) < min_column_value_)
@@ -103,25 +328,8 @@ class ResourceChangeObserver : public TaskManagerModelObserver {
     return match_count;
   }
 
-  size_t GetColumnValue(int index) {
-    size_t value = 0;
-    bool success = false;
-    switch (column_specifier_) {
-      case COLUMN_NONE:
-        break;
-      case V8_MEMORY:
-        success = model_->GetV8Memory(index, &value);
-        break;
-      case V8_MEMORY_USED:
-        success = model_->GetV8MemoryUsed(index, &value);
-        break;
-      case SQLITE_MEMORY_USED:
-        success = model_->GetSqliteMemoryUsedBytes(index, &value);
-        break;
-    }
-    if (!success)
-      return 0;
-    return value;
+  int64_t GetColumnValue(int index) {
+    return model_->GetColumnValue(column_specifier_, index);
   }
 
   const char* GetColumnName() {
@@ -156,10 +364,9 @@ class ResourceChangeObserver : public TaskManagerModelObserver {
     task_manager_state_dump << "\nCurrently there are " << CountMatches()
                             << " matches.";
     task_manager_state_dump << "\nCurrent Task Manager Model is:";
-    for (int i = 0; i < model_->ResourceCount(); i++) {
-      task_manager_state_dump
-          << "\n  > " << std::setw(40) << std::left
-          << base::UTF16ToASCII(model_->GetResourceTitle(i));
+    for (int i = 0; i < model_->GetRowCount(); i++) {
+      task_manager_state_dump << "\n  > " << std::setw(40) << std::left
+                              << base::UTF16ToASCII(model_->GetRowTitle(i));
       if (min_column_value_ > 0) {
         task_manager_state_dump << " [" << GetColumnName()
                                 << " == " << GetColumnValue(i) << "]";
@@ -168,45 +375,41 @@ class ResourceChangeObserver : public TaskManagerModelObserver {
     return task_manager_state_dump;
   }
 
-  const TaskManagerModel* model_;
+  std::unique_ptr<TaskManagerTester> model_;
   const int required_count_;
   const base::string16 title_pattern_;
   const ColumnSpecifier column_specifier_;
-  const size_t min_column_value_;
+  const int64_t min_column_value_;
   base::RunLoop run_loop_;
   base::OneShotTimer timer_;
 };
 
 }  // namespace
 
-void EnableOldTaskManager() {
-  base::CommandLine::ForCurrentProcess()->AppendSwitch(
-      switches::kDisableNewTaskManager);
+std::unique_ptr<TaskManagerTester> GetTaskManagerTester() {
+  if (switches::NewTaskManagerEnabled()) {
+    return base::WrapUnique(

[afakhry, 2016/04/28 00:03:13]

Are we not allowed to use std::make_unique()?

[ncarter (slow), 2016/05/02 18:33:28]

make_unique is not in C++11, it's C++14. Also, WrapUnique and make_unique
operate very differently. WrapUnique takes a T*, whereas make_unique takes a set
of args for a T ctor, and calls "operator new" for you.

[afakhry, 2016/05/02 18:58:17]

Yes, I know, but thanks for the explanation anyways! :)
I was just curious. Should have just checked http://chromium-cpp.appspot.com/.

+        new task_management::TaskManagerTesterImpl(base::Closure()));
+  } else {
+    return base::WrapUnique(new LegacyTaskManagerTesterImpl(base::Closure()));
+  }
 }
 
 void WaitForTaskManagerRows(int required_count,
                             const base::string16& title_pattern) {
-  TaskManagerModel* model = TaskManager::GetInstance()->model();
-
   const int column_value_dont_care = 0;
-  ResourceChangeObserver observer(model, required_count, title_pattern,
-                                  COLUMN_NONE, column_value_dont_care);
-  model->AddObserver(&observer);
+  ResourceChangeObserver observer(required_count, title_pattern, COLUMN_NONE,
+                                  column_value_dont_care);
   observer.RunUntilSatisfied();
-  model->RemoveObserver(&observer);
 }
 
 void WaitForTaskManagerStatToExceed(const base::string16& title_pattern,
                                     ColumnSpecifier column_getter,
                                     size_t min_column_value) {
-  TaskManagerModel* model = TaskManager::GetInstance()->model();
-
   const int wait_for_one_match = 1;
-  ResourceChangeObserver observer(model, wait_for_one_match, title_pattern,
+  ResourceChangeObserver observer(wait_for_one_match, title_pattern,
                                   column_getter, min_column_value);
-  model->AddObserver(&observer);
   observer.RunUntilSatisfied();
-  model->RemoveObserver(&observer);
 }
 
 base::string16 MatchTab(const char* title) {
@@ -214,45 +417,58 @@ base::string16 MatchTab(const char* title) {
                                     base::ASCIIToUTF16(title));
 }
 
-base::string16 MatchAnyTab() { return MatchTab("*"); }
+base::string16 MatchAnyTab() {
+  return MatchTab("*");
+}
 
-base::string16 MatchAboutBlankTab() { return MatchTab("about:blank"); }
+base::string16 MatchAboutBlankTab() {
+  return MatchTab("about:blank");
+}
 
 base::string16 MatchExtension(const char* title) {
   return l10n_util::GetStringFUTF16(IDS_TASK_MANAGER_EXTENSION_PREFIX,
                                     base::ASCIIToUTF16(title));
 }
 
-base::string16 MatchAnyExtension() { return MatchExtension("*"); }
+base::string16 MatchAnyExtension() {
+  return MatchExtension("*");
+}
 
 base::string16 MatchApp(const char* title) {
   return l10n_util::GetStringFUTF16(IDS_TASK_MANAGER_APP_PREFIX,
                                     base::ASCIIToUTF16(title));
 }
 
-base::string16 MatchAnyApp() { return MatchApp("*"); }
+base::string16 MatchAnyApp() {
+  return MatchApp("*");
+}
 
 base::string16 MatchWebView(const char* title) {
   return l10n_util::GetStringFUTF16(
-      IDS_EXTENSION_TASK_MANAGER_WEBVIEW_TAG_PREFIX,
-      base::ASCIIToUTF16(title));
+      IDS_EXTENSION_TASK_MANAGER_WEBVIEW_TAG_PREFIX, base::ASCIIToUTF16(title));
 }
 
-base::string16 MatchAnyWebView() { return MatchWebView("*"); }
+base::string16 MatchAnyWebView() {
+  return MatchWebView("*");
+}
 
 base::string16 MatchBackground(const char* title) {
   return l10n_util::GetStringFUTF16(IDS_TASK_MANAGER_BACKGROUND_PREFIX,
                                     base::ASCIIToUTF16(title));
 }
 
-base::string16 MatchAnyBackground() { return MatchBackground("*"); }
+base::string16 MatchAnyBackground() {
+  return MatchBackground("*");
+}
 
 base::string16 MatchPrint(const char* title) {
   return l10n_util::GetStringFUTF16(IDS_TASK_MANAGER_PRINT_PREFIX,
                                     base::ASCIIToUTF16(title));
 }
 
-base::string16 MatchAnyPrint() { return MatchPrint("*"); }
+base::string16 MatchAnyPrint() {
+  return MatchPrint("*");
+}
 
 base::string16 MatchSubframe(const char* title) {
   return l10n_util::GetStringFUTF16(IDS_TASK_MANAGER_SUBFRAME_PREFIX,