device_orientation::ProviderImpl to stop polling thread asynchronously

content/browser/device_orientation/provider_impl.cc

Index: content/browser/device_orientation/provider_impl.cc
===================================================================
--- content/browser/device_orientation/provider_impl.cc	(revision 148849)
+++ content/browser/device_orientation/provider_impl.cc	(working copy)
@@ -10,69 +10,88 @@
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/threading/thread.h"
-#include "base/threading/thread_restrictions.h"
+#include "base/threading/worker_pool.h"
 #include "content/browser/device_orientation/orientation.h"
 #include "content/browser/device_orientation/provider_impl.h"
 
-namespace device_orientation {
+namespace {
 
-ProviderImpl::ProviderImpl(const DataFetcherFactory factories[])
-    : creator_loop_(MessageLoop::current()),
-      ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
-  for (const DataFetcherFactory* fp = factories; *fp; ++fp)
-    factories_.push_back(*fp);
+bool IsElementSignificantlyDifferent(bool can_provide_element1,
+                                     bool can_provide_element2,
+                                     double element1,
+                                     double element2) {
+  const double kThreshold = 0.1;
+
+  if (can_provide_element1 != can_provide_element2)
+    return true;
+  if (can_provide_element1 &&
+      std::fabs(element1 - element2) >= kThreshold)
+    return true;
+  return false;
 }
 
-ProviderImpl::~ProviderImpl() {
+void DeleteThread(base::Thread* thread) {
+  delete thread;
 }
 
-void ProviderImpl::AddObserver(Observer* observer) {
-  DCHECK(MessageLoop::current() == creator_loop_);
-
-  observers_.insert(observer);
-  if (observers_.size() == 1)
-    Start();
-  else
-    observer->OnOrientationUpdate(last_notification_);
 }
 
-void ProviderImpl::RemoveObserver(Observer* observer) {
-  DCHECK(MessageLoop::current() == creator_loop_);
+namespace device_orientation {
 
-  observers_.erase(observer);
-  if (observers_.empty())
-    Stop();
-}
+class ProviderImpl::PollingThread : public base::Thread {
+ public:
+  explicit PollingThread(const char* name,

[hans, 2012/07/30 12:56:40]

nit: no need for explicit here, I think

+                         base::WeakPtr<ProviderImpl> provider,
+                         MessageLoop* creator_loop);
+  virtual ~PollingThread() OVERRIDE;
 
-void ProviderImpl::Start() {
-  DCHECK(MessageLoop::current() == creator_loop_);
-  DCHECK(!polling_thread_.get());
+  // Method for finding a suitable DataFetcher and starting the polling.
+  void InitializePollingThread(

[hans, 2012/07/30 12:56:40]

since it's a member of PollingThread, I think just calling this "Initialize"
would be fine?

+      const std::vector<DataFetcherFactory>& factories);
 
-  polling_thread_.reset(new base::Thread("Device orientation polling thread"));
-  if (!polling_thread_->Start()) {
-    LOG(ERROR) << "Failed to start device orientation polling thread";
-    polling_thread_.reset();
-    return;
-  }
-  ScheduleInitializePollingThread();
-}
+ private:
+  // Method for polling a DataFetcher.
+  void DoPoll();
+  void ScheduleDoPoll();
 
-void ProviderImpl::Stop() {
-  DCHECK(MessageLoop::current() == creator_loop_);
+  // Schedule a notification to the |provider_| which lives on a different
+  // thread (|creator_loop_| is its message loop).
+  void ScheduleDoNotify(const Orientation& orientation);
 
-  // TODO(hans): Don't join the thread. See crbug.com/72286.
-  base::ThreadRestrictions::ScopedAllowIO allow_io;
+  static bool SignificantlyDifferent(const Orientation& orientation1,
+                                     const Orientation& orientation2);
 
-  polling_thread_.reset();
-  data_fetcher_.reset();
+  enum { kDesiredSamplingIntervalMs = 100 };
+  base::TimeDelta SamplingInterval() const;
+
+  // The Message Loop on which this object was created.
+  // Typically the I/O loop, but may be something else during testing.
+  MessageLoop* creator_loop_;
+
+  scoped_ptr<DataFetcher> data_fetcher_;
+  Orientation last_orientation_;
+
+  base::WeakPtr<ProviderImpl> provider_;
+};
+
+ProviderImpl::PollingThread::PollingThread(
+    const char* name,
+    base::WeakPtr<ProviderImpl> provider,
+    MessageLoop* creator_loop)
+  : base::Thread(name),

[hans, 2012/07/30 12:56:40]

nit indentation looks off here: there should be four spaces before the :

+    creator_loop_(creator_loop),
+    provider_(provider) {
 }
 
-void ProviderImpl::DoInitializePollingThread(
+ProviderImpl::PollingThread::~PollingThread() {
+}
+
+void ProviderImpl::PollingThread::InitializePollingThread(
     const std::vector<DataFetcherFactory>& factories) {
-  DCHECK(MessageLoop::current() == polling_thread_->message_loop());
+  DCHECK(MessageLoop::current() == message_loop());
 
   typedef std::vector<DataFetcherFactory>::const_iterator Iterator;
-  for (Iterator i = factories_.begin(), e = factories_.end(); i != e; ++i) {
+  for (Iterator i = factories.begin(), e = factories.end(); i != e; ++i) {

[hans, 2012/07/30 12:56:40]

nit: since you're touching this line anyway, feel free to rewrite as "for
(Iterator i = factories.begin(); i != factories.end(); ++i) {"

     DataFetcherFactory factory = *i;
     scoped_ptr<DataFetcher> fetcher(factory());
     Orientation orientation;
@@ -96,41 +115,16 @@
   ScheduleDoNotify(Orientation::Empty());
 }
 
-void ProviderImpl::ScheduleInitializePollingThread() {
-  DCHECK(MessageLoop::current() == creator_loop_);
+void ProviderImpl::PollingThread::ScheduleDoNotify(
+    const Orientation& orientation) {
+  DCHECK(MessageLoop::current() == message_loop());
 
-  MessageLoop* polling_loop = polling_thread_->message_loop();
-  polling_loop->PostTask(FROM_HERE,
-                         base::Bind(&ProviderImpl::DoInitializePollingThread,
-                                    this,
-                                    factories_));
-}
-
-void ProviderImpl::DoNotify(const Orientation& orientation) {
-  DCHECK(MessageLoop::current() == creator_loop_);
-
-  last_notification_ = orientation;
-
-  typedef std::set<Observer*>::const_iterator Iterator;
-  for (Iterator i = observers_.begin(), e = observers_.end(); i != e; ++i)
-    (*i)->OnOrientationUpdate(orientation);
-
-  if (orientation.is_empty()) {
-    // Notify observers about failure to provide data exactly once.
-    observers_.clear();
-    Stop();
-  }
-}
-
-void ProviderImpl::ScheduleDoNotify(const Orientation& orientation) {
-  DCHECK(MessageLoop::current() == polling_thread_->message_loop());
-
   creator_loop_->PostTask(
-      FROM_HERE, base::Bind(&ProviderImpl::DoNotify, this, orientation));
+      FROM_HERE, base::Bind(&ProviderImpl::DoNotify, provider_, orientation));
 }
 
-void ProviderImpl::DoPoll() {
-  DCHECK(MessageLoop::current() == polling_thread_->message_loop());
+void ProviderImpl::PollingThread::DoPoll() {
+  DCHECK(MessageLoop::current() == message_loop());
 
   Orientation orientation;
   if (!data_fetcher_->GetOrientation(&orientation)) {
@@ -149,37 +143,20 @@
   ScheduleDoPoll();
 }
 
-void ProviderImpl::ScheduleDoPoll() {
-  DCHECK(MessageLoop::current() == polling_thread_->message_loop());
+void ProviderImpl::PollingThread::ScheduleDoPoll() {
+  DCHECK(MessageLoop::current() == message_loop());
 
-  MessageLoop* polling_loop = polling_thread_->message_loop();
-  polling_loop->PostDelayedTask(
+  message_loop()->PostDelayedTask(
       FROM_HERE,
-      base::Bind(&ProviderImpl::DoPoll, weak_factory_.GetWeakPtr()),
+      base::Bind(&PollingThread::DoPoll, base::Unretained(this)),
       SamplingInterval());
 }
 
-namespace {
-
-bool IsElementSignificantlyDifferent(bool can_provide_element1,
-                                     bool can_provide_element2,
-                                     double element1,
-                                     double element2) {
-  const double kThreshold = 0.1;
-
-  if (can_provide_element1 != can_provide_element2)
-    return true;
-  if (can_provide_element1 &&
-      std::fabs(element1 - element2) >= kThreshold)
-    return true;
-  return false;
-}
-}  // namespace
-
 // Returns true if two orientations are considered different enough that
 // observers should be notified of the new orientation.
-bool ProviderImpl::SignificantlyDifferent(const Orientation& o1,
-                                          const Orientation& o2) {
+bool ProviderImpl::PollingThread::SignificantlyDifferent(
+    const Orientation& o1,
+    const Orientation& o2) {
   return IsElementSignificantlyDifferent(o1.can_provide_alpha(),
                                          o2.can_provide_alpha(),
                                          o1.alpha(),
@@ -196,8 +173,8 @@
           o1.absolute() != o2.absolute());
 }
 
-base::TimeDelta ProviderImpl::SamplingInterval() const {
-  DCHECK(MessageLoop::current() == polling_thread_->message_loop());
+base::TimeDelta ProviderImpl::PollingThread::SamplingInterval() const {
+  DCHECK(MessageLoop::current() == message_loop());
   DCHECK(data_fetcher_.get());
 
   // TODO(erg): There used to be unused code here, that called a default
@@ -206,4 +183,92 @@
   return base::TimeDelta::FromMilliseconds(kDesiredSamplingIntervalMs);
 }
 
+ProviderImpl::ProviderImpl(const DataFetcherFactory factories[])
+    : creator_loop_(MessageLoop::current()),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
+      polling_thread_(NULL) {
+  for (const DataFetcherFactory* fp = factories; *fp; ++fp)
+    factories_.push_back(*fp);
+}
+
+ProviderImpl::~ProviderImpl() {
+  DCHECK(observers_.empty());
+  DCHECK(!polling_thread_);
+}
+
+void ProviderImpl::AddObserver(Observer* observer) {
+  DCHECK(MessageLoop::current() == creator_loop_);
+
+  observers_.insert(observer);
+  if (observers_.size() == 1)
+    Start();
+  else
+    observer->OnOrientationUpdate(last_notification_);
+}
+
+void ProviderImpl::RemoveObserver(Observer* observer) {
+  DCHECK(MessageLoop::current() == creator_loop_);
+
+  observers_.erase(observer);
+  if (observers_.empty())
+    Stop();
+}
+
+void ProviderImpl::Start() {
+  DCHECK(MessageLoop::current() == creator_loop_);
+  DCHECK(!polling_thread_);
+
+  polling_thread_ = new PollingThread("Device orientation polling thread",
+                                      weak_factory_.GetWeakPtr(),
+                                      creator_loop_);
+  if (!polling_thread_->Start()) {
+    LOG(ERROR) << "Failed to start device orientation polling thread";
+    delete polling_thread_;
+    polling_thread_ = NULL;
+    return;
+  }
+  ScheduleInitializePollingThread();
+}
+
+void ProviderImpl::Stop() {
+  DCHECK(MessageLoop::current() == creator_loop_);
+
+  weak_factory_.InvalidateWeakPtrs();
+  if (polling_thread_) {
+    polling_thread_->StopSoon();
+    bool posted = base::WorkerPool::PostTask(
+        FROM_HERE,
+        base::Bind(&DeleteThread, base::Unretained(polling_thread_)),
+        true /* task is slow */);
+    DCHECK(posted);
+    polling_thread_ = NULL;
+  }
+}
+
+void ProviderImpl::ScheduleInitializePollingThread() {
+  DCHECK(MessageLoop::current() == creator_loop_);
+
+  MessageLoop* polling_loop = polling_thread_->message_loop();
+  polling_loop->PostTask(FROM_HERE,
+                         base::Bind(&PollingThread::InitializePollingThread,
+                                    base::Unretained(polling_thread_),
+                                    factories_));
+}
+
+void ProviderImpl::DoNotify(const Orientation& orientation) {
+  DCHECK(MessageLoop::current() == creator_loop_);
+
+  last_notification_ = orientation;
+
+  typedef std::set<Observer*>::const_iterator Iterator;
+  for (Iterator i = observers_.begin(), e = observers_.end(); i != e; ++i)

[hans, 2012/07/30 12:56:40]

nit: since you're touching this line anyway, feel free to rewrite as "for
(Iterator i = observers_.begin(); i != observers_.end(); ++i)"

+    (*i)->OnOrientationUpdate(orientation);
+
+  if (orientation.is_empty()) {
+    // Notify observers about failure to provide data exactly once.
+    observers_.clear();
+    Stop();
+  }
+}
+
 }  // namespace device_orientation