New Task Manager - Phase 1.3.2.B: Implement Tab Contents Task Providing (Prerender)

chrome/browser/prerender/prerender_browsertest.cc

Index: chrome/browser/prerender/prerender_browsertest.cc
diff --git a/chrome/browser/prerender/prerender_browsertest.cc b/chrome/browser/prerender/prerender_browsertest.cc
index 7b742f03bd6e354df0f5faaac73475686f9834d7..54029db4705c715755651edce3714491eaf05671 100644
--- a/chrome/browser/prerender/prerender_browsertest.cc
+++ b/chrome/browser/prerender/prerender_browsertest.cc
@@ -48,6 +48,9 @@
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/safe_browsing/safe_browsing_util.h"
 #include "chrome/browser/safe_browsing/test_database_manager.h"
+#include "chrome/browser/task_management/providers/task_provider_observer.h"
+#include "chrome/browser/task_management/providers/web_contents/web_contents_tags_manager.h"
+#include "chrome/browser/task_management/providers/web_contents/web_contents_task_provider.h"
 #include "chrome/browser/task_manager/task_manager.h"
 #include "chrome/browser/task_manager/task_manager_browsertest_util.h"
 #include "chrome/browser/ui/browser.h"
@@ -4080,4 +4083,106 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTestWithNaCl,
 }
 #endif  // !defined(DISABLE_NACL)
 
+#if defined(ENABLE_TASK_MANAGER)
+
+namespace {
+
+const char kPrerenderPage[] = "files/prerender/prerender_page.html";
+
+// Defines a test class for testing that will act as a mock task manager.
+class MockTaskManager : public task_management::TaskProviderObserver {
+ public:
+  MockTaskManager() {}
+  ~MockTaskManager() override {}
+
+  // task_management::Task_providerObserver:
+  void TaskAdded(task_management::Task* task) override {
+    CHECK(task);

[gavinp, 2015/06/22 18:38:04]

Remove this check, crashing in unit tests is bad form.

Is this check really a part of testing the prerender code, what could cause this
to be null? If it's some other system with its own tests that could cause this,
just remove the check, and we'll crash later when we dereference it.

If it is part of the test for this system, then we have to be a bit silly to
avoid the crash, something like:

if (!task) {
  ADD_FAILURE() << "task was nullptr.";
  return;
}

Should do the trick?

[afakhry, 2015/06/22 21:56:17]

That CHECK was actually not needed, so removed.

+    ASSERT_FALSE(provided_tasks_.count(task));

[gavinp, 2015/06/22 18:38:03]

ASSERT_FALSE won't work as many expect here; on failure it adds a failure to the
test run, which is expected, but then it just returns from this function and
continues execution. The test will continue.

See
https://code.google.com/p/googletest/wiki/AdvancedGuide#Propagating_Fatal_Fai...
in the Google Test manual for details.

Any reason this needs to be fatal? I suggest making it EXPECT_FALSE...; of
course the world gets confusing fast, but that's OK?

[afakhry, 2015/06/22 21:56:17]

Modified to EXPECT_FALSE()

+    provided_tasks_.insert(task);
+  }
+
+  void TaskRemoved(task_management::Task* task) override {
+    CHECK(task);
+    ASSERT_TRUE(provided_tasks_.count(task));
+    provided_tasks_.erase(task);
+  }
+
+  base::string16 GetPrerenderTitlePrefix() const {
+    return l10n_util::GetStringFUTF16(IDS_TASK_MANAGER_PRERENDER_PREFIX,
+                                      base::string16());
+  }
+
+  const std::set<task_management::WebContentsTag*>& tracked_tags() const {
+    return task_management::WebContentsTagsManager::GetInstance()->
+        tracked_tags();
+  }
+
+  const std::set<task_management::Task*>& provided_tasks() const {
+    return provided_tasks_;
+  }
+
+ private:
+  std::set<task_management::Task*> provided_tasks_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockTaskManager);
+};
+
+}  // namespace
+
+// Tests the correct recording of tags for the prerender WebContents.
+IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, TaskManagementTagsBasic) {
+  MockTaskManager task_manager;
+  EXPECT_TRUE(task_manager.tracked_tags().empty());
+
+  // Start prerendering a page and make sure it's correctly tagged.
+  PrerenderTestURL(kPrerenderPage, FINAL_STATUS_USED, 1);
+  EXPECT_FALSE(task_manager.tracked_tags().empty());
+
+  // TODO(afakhry): Once we start tagging the tab contents the below tests

[gavinp, 2015/06/22 18:38:03]

Nit: (optional) Remove the extra indent on the second line, wrap comment at 80
columns.

[afakhry, 2015/06/22 21:56:17]

Done.

+  //                must be changed.
+  EXPECT_EQ(1U, task_manager.tracked_tags().size());
+
+  // Swap in the prerendered content and make sure its tag is removed.
+  NavigateToDestURL();
+  EXPECT_TRUE(task_manager.tracked_tags().empty());
+}
+
+// Tests that the task manager will be provided by tasks that correspond to
+// prerendered WebContents.
+IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, TaskManagementTasksProvided) {
+  MockTaskManager task_manager;
+  EXPECT_TRUE(task_manager.provided_tasks().empty());

[gavinp, 2015/06/22 18:38:03]

It seems like a bit of overkill to assert this every time we create a
MockTaskManager(). It has some documentary value, but it's not huge.

Is there any reason not to just do these EXPECT-it's-empty calls in the
constructor of the MockTaskManager?

[afakhry, 2015/06/22 21:56:17]

Yes it is actually an overkill, and hence removed.

+  EXPECT_TRUE(task_manager.tracked_tags().empty());
+
+  task_management::WebContentsTaskProvider provider;
+  provider.SetObserver(&task_manager);
+
+  // Still empty, no pre-existing tasks.
+  EXPECT_TRUE(task_manager.provided_tasks().empty());
+
+  // Start prerendering a page.
+  PrerenderTestURL(kPrerenderPage, FINAL_STATUS_USED, 1);
+  EXPECT_FALSE(task_manager.tracked_tags().empty());
+
+  // TODO(afakhry): The below may not be true after we support more tags.
+  EXPECT_EQ(1U, task_manager.tracked_tags().size());
+  ASSERT_FALSE(task_manager.provided_tasks().empty());
+  ASSERT_EQ(1U, task_manager.provided_tasks().size());
+
+  const task_management::Task* task = *task_manager.provided_tasks().begin();
+  EXPECT_EQ(task_management::Task::RENDERER, task->GetType());
+  const base::string16 title = task->title();
+  const base::string16 expected_prefix = task_manager.GetPrerenderTitlePrefix();
+  EXPECT_TRUE(base::StartsWith(title,
+                               expected_prefix,
+                               base::CompareCase::INSENSITIVE_ASCII));
+
+  NavigateToDestURL();
+  // TODO(afakhry): The below may not be true after we support more tags.
+  EXPECT_TRUE(task_manager.provided_tasks().empty());
+}
+
+#endif  // defined(ENABLE_TASK_MANAGER)
+
 }  // namespace prerender