[Chromoting] Introducing refcount-based life time management of the message loops in the service (d…

remoting/base/auto_thread_task_runner.h

+// Copyright (c) 2012 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 REMOTING_BASE_AUTO_THREAD_TASK_RUNNER_H_
+#define REMOTING_BASE_AUTO_THREAD_TASK_RUNNER_H_
+
+#include "base/basictypes.h"
+#include "base/callback.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop.h"
+#include "base/single_thread_task_runner.h"
+
+class MessageLoop;
+
+namespace remoting {
+
+// This class provides automatic life time management for a single thread task

[Wez, 2012/08/30 18:20:12]

nit: ... for a SingleThreadTaskRunner.

[alexeypa (please no reviews), 2012/08/30 22:08:07]

Done.

+// runner. The task runner is stopped when the last reference to
+// |AutoThreadTaskRunner| is dropped.

[Wez, 2012/08/30 18:20:12]

Looking at the implementation, I can't see anything that actually posts a Stop()
task for the thread to the parent; presumably that's something the caller has to
arrange via a suitable |stop_callback|?

What is the purpose of AutoThreadTaskRunner with no |stop_callback| or |parent|?

[alexeypa (please no reviews), 2012/08/30 22:08:07]

This is documented in ChromotingHostContext.

[Wez, 2012/08/30 23:56:43]

I can see the comment regarding wrapping the MessageLoopProxys to get CHECKs but
nothing answering the questions above. In any case, explanations of how to use
this class should be in the class header, not in calling code.

[alexeypa (please no reviews), 2012/08/31 19:57:36]

I adopted a modified version of the comment describing |AutoThreadTaskRunner|
that you proposed in a later comment.

+class AutoThreadTaskRunner : public base::SingleThreadTaskRunner {
+ public:
+  // Constructs an instance of |AutoThreadTaskRunner| wrapping |task_runner|.
+  // |stop_callback| is called (on arbitraty thread) when the last reference to
+  // the object is dropped.
+  explicit AutoThreadTaskRunner(
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner);
+  AutoThreadTaskRunner(scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+                       const base::Closure& stop_callback);
+
+  // TODO(alexeypa): Remove the |parent| reference and contructors taking it

[Wez, 2012/08/30 18:20:12]

typo: constructors

[alexeypa (please no reviews), 2012/08/30 22:08:07]

I don't see any typo. I rephrased the comment just in case.

[Wez, 2012/08/30 23:56:43]

You'd just missed the 's' in constructors. The new wording reads better anyway,
IMHO. :)

[alexeypa (please no reviews), 2012/08/31 19:57:36]

I didn't. There are two constructors that take |parent|.

+  // once Thread class supports stopping the thread's message loop and joining
+  // the thread separately.
+  //
+  // Background: currently the only legitimate way of stopping a thread is to
+  // call Thread::Stop() from the same thread that started it. Thread::Stop()
+  // will stop the thread message loop by posting a private task and join
+  // the thread. Thread::Stop() verifies that it is called from the right thread
+  // and that the message loop has been stopped by the private task mentioned
+  // above.
+  //
+  // Until Thread::Stop() is split in two separate operations, it should not be
+  // called while any objects use it. It is achieved by keeping a reference to

[Wez, 2012/08/30 18:20:12]

nit: It -> This

[alexeypa (please no reviews), 2012/08/30 22:08:07]

Done.

+  // the parent loop. Since Thread::Stop() is called after exiting the parent

[Wez, 2012/08/30 18:20:12]

nit: ... parent loop. Since Thread ... -> ... parent thread's task runner, and
posting a task to Stop() this Thread when no more references to it remain.
|parent| should itself be an AutoThreadTaskRunner managed thread, to guarantee
that it remains alive long enough to perform the Stop().

[alexeypa (please no reviews), 2012/08/30 22:08:07]

Done.
The previous paragraph explains that Thread will not allow us to post anything
to stop the thread.
Done.

+  // loop, keeping a reference to it gives enough guaratee.
+  AutoThreadTaskRunner(scoped_refptr<base::SingleThreadTaskRunner> parent,
+                       scoped_refptr<base::SingleThreadTaskRunner> task_runner);

[Wez, 2012/08/30 18:20:12]

nit: I think it makes more sense for |parent| to come after the other
parameters, so that the first TaskRunner passed to the ctor is always the
thread's |task_runner|, even if you're using a |parent| form.

[alexeypa (please no reviews), 2012/08/30 22:08:07]

Done.

+  AutoThreadTaskRunner(scoped_refptr<base::SingleThreadTaskRunner> parent,
+                       scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+                       const base::Closure& stop_callback);
+
+  // SingleThreadTaskRunner implementation
+  virtual bool PostDelayedTask(const tracked_objects::Location& from_here,
+                               const base::Closure& task,
+                               base::TimeDelta delay) OVERRIDE;
+  virtual bool PostNonNestableDelayedTask(
+      const tracked_objects::Location& from_here,
+      const base::Closure& task,
+      base::TimeDelta delay) OVERRIDE;
+  virtual bool RunsTasksOnCurrentThread() const OVERRIDE;
+
+ private:
+  friend class base::DeleteHelper<AutoThreadTaskRunner>;
+
+  virtual ~AutoThreadTaskRunner();
+
+  // An reference to the parent message loop to keep it alive while
+  // |task_runner_| is running.

[Wez, 2012/08/30 18:20:12]

nit: ... is running. -> ... is running, so that a thread Stop() task can be
posted to it.

[alexeypa (please no reviews), 2012/08/30 22:08:07]

Stop() cannot be posted to it.

[Wez, 2012/08/30 23:56:43]

Because of the plugin shutdown case... it's a PITA that we have to make this
class confusing to code around that.

+  scoped_refptr<base::SingleThreadTaskRunner> parent_;
+
+  // This callback quits |task_runner_|. It can be run on any thread.
+  base::Closure stop_callback_;
+
+  // The wrapped task runner.
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(AutoThreadTaskRunner);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_BASE_AUTO_THREAD_TASK_RUNNER_H_