Query NaCl processes' GDB debug port on the correct thread.

chrome/browser/task_manager/sampling/task_group_unittest.cc

+// Copyright 2016 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_manager/sampling/task_group.h"
+
+#include <memory>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "base/strings/string16.h"
+#include "chrome/browser/task_manager/sampling/shared_sampler.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "gpu/ipc/common/memory_stats.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace task_manager {
+
+namespace {
+
+class FakeTask : public Task {
+ public:
+  FakeTask(base::ProcessId process_id,
+           Type type)
+      : Task(base::string16(),
+             "FakeTask",
+             nullptr,
+             base::kNullProcessHandle,
+             process_id),
+        type_(type) {
+  }
+
+  Type GetType() const override { return type_; }
+
+  int GetChildProcessUniqueID() const override { return 0; }
+
+  const Task* GetParentTask() const override { return nullptr; }
+
+  int GetTabId() const override { return 0; }
+
+ private:
+  Type type_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeTask);
+};
+
+} // namespace
+
+class TaskGroupTest : public testing::Test {
+ public:
+  TaskGroupTest()
+      : io_task_runner_(content::BrowserThread::GetTaskRunnerForThread(
+            content::BrowserThread::IO)),
+        run_loop_(base::MakeUnique<base::RunLoop>()),
+        task_group_(process_handle(),
+                    process_id(),

[afakhry, 2017/02/10 20:08:52]

I think you should make the above two accessors static functions or
anonymous-namespace functions. I find it weird that we're calling member
functions in the constructor before the object is fully constructed. Besides
they don't really access any member variable, which makes them good candidates
for getting outside the class.

[Wez, 2017/02/13 01:22:19]

Removed them in favour of calling the base::Process APIs directly, which seemed
more readable, if anything.

[afakhry, 2017/02/15 02:58:19]

Acknowledged.

+                    base::Bind(&TaskGroupTest::OnBackgroundCalculationsDone,
+                               base::Unretained(this)),
+                    new SharedSampler(io_task_runner_),
+                    io_task_runner_) {}
+
+ protected:
+  void OnBackgroundCalculationsDone() {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    background_refresh_complete_ = true;
+    run_loop_->QuitWhenIdle();
+  }
+
+  base::ProcessHandle process_handle() const {
+    return base::Process::Current().Handle();
+  }
+  base::ProcessId process_id() const { return base::Process::Current().Pid(); }
+
+  content::TestBrowserThreadBundle browser_threads_;
+  scoped_refptr<base::SequencedTaskRunner> io_task_runner_;
+  std::unique_ptr<base::RunLoop> run_loop_;
+  TaskGroup task_group_;
+  bool background_refresh_complete_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(TaskGroupTest);
+};
+
+TEST_F(TaskGroupTest, NullRefresh) {

[afakhry, 2017/02/10 20:08:52]

It would be really nice to add a comment above each test to simply explain what
it's testing. Could you please do that?

[Wez, 2017/02/13 01:22:19]

Done.

+  task_group_.Refresh(gpu::VideoMemoryUsageStats(), base::TimeDelta(), 0);
+  EXPECT_TRUE(task_group_.AreBackgroundCalculationsDone());
+  EXPECT_FALSE(background_refresh_complete_);
+}
+
+TEST_F(TaskGroupTest, SyncRefresh) {
+  task_group_.Refresh(gpu::VideoMemoryUsageStats(), base::TimeDelta(),
+                      REFRESH_TYPE_NETWORK_USAGE);
+  EXPECT_TRUE(task_group_.AreBackgroundCalculationsDone());
+  EXPECT_FALSE(background_refresh_complete_);
+}
+
+TEST_F(TaskGroupTest, AsyncRefresh) {
+  // Memory is measured asynchronously, but not via SharedSampler (see below).
+  task_group_.Refresh(gpu::VideoMemoryUsageStats(), base::TimeDelta(),
+                      REFRESH_TYPE_MEMORY);
+  EXPECT_FALSE(task_group_.AreBackgroundCalculationsDone());
+
+  ASSERT_FALSE(background_refresh_complete_);
+  run_loop_->Run();
+
+  EXPECT_TRUE(task_group_.AreBackgroundCalculationsDone());
+  EXPECT_TRUE(background_refresh_complete_);
+}
+
+TEST_F(TaskGroupTest, SharedAsyncRefresh) {
+  // Idle wakeups are measured via SharedSampler under Windows.
+  task_group_.Refresh(gpu::VideoMemoryUsageStats(), base::TimeDelta(),
+                      REFRESH_TYPE_IDLE_WAKEUPS);

[afakhry, 2017/02/10 20:08:52]

IDLE_WAKEUPS is a background-calculated value and that what matters here. Done
via the SharedSampler on Windows and the |worker_thread_sampler_| on other
platforms.

The name of the test as well as the comment are misleading.

[Wez, 2017/02/13 01:22:19]

I've reworked the comment; I'd prefer to keep the test name as-is since the
intention is specifically to test a value which will use the SharedSampler on
platforms which use that.

Ideally I'd like to have parameterized tests which run multiple times, each
verifying a different set of flags, but this approach felt cleaner to get some
basic tests running.

[afakhry, 2017/02/15 02:58:19]

Acknowledged.

+  EXPECT_FALSE(task_group_.AreBackgroundCalculationsDone());
+
+  ASSERT_FALSE(background_refresh_complete_);
+  run_loop_->Run();
+
+  EXPECT_TRUE(background_refresh_complete_);
+
+  EXPECT_TRUE(task_group_.AreBackgroundCalculationsDone());
+}
+
+TEST_F(TaskGroupTest, NaclRefreshZeroTasks) {
+  task_group_.Refresh(gpu::VideoMemoryUsageStats(), base::TimeDelta(),
+                      REFRESH_TYPE_NACL);
+  EXPECT_TRUE(task_group_.AreBackgroundCalculationsDone());
+  EXPECT_FALSE(background_refresh_complete_);
+}
+
+TEST_F(TaskGroupTest, NaclRefreshWithTask) {
+  FakeTask fake_task(process_id(), Task::NACL);
+  task_group_.AddTask(&fake_task);
+
+  task_group_.Refresh(gpu::VideoMemoryUsageStats(), base::TimeDelta(),
+                      REFRESH_TYPE_NACL);
+#if !defined(DISABLE_NACL)
+  EXPECT_FALSE(task_group_.AreBackgroundCalculationsDone());
+
+  ASSERT_FALSE(background_refresh_complete_);
+  run_loop_->Run();
+
+  EXPECT_TRUE(background_refresh_complete_);
+#endif // !defined(DISABLE_NACL)
+
+  EXPECT_TRUE(task_group_.AreBackgroundCalculationsDone());
+}
+
+}  // namespace task_manager