Refactors DeviceOrientation to make it more extensible

content/browser/device_orientation/provider_impl.cc

Index: content/browser/device_orientation/provider_impl.cc
diff --git a/content/browser/device_orientation/provider_impl.cc b/content/browser/device_orientation/provider_impl.cc
index fab78a11a42733926495eaa1902bc52d879a21bb..531d6b226e2c423ea282de83f54d054c11e3a5b3 100644
--- a/content/browser/device_orientation/provider_impl.cc
+++ b/content/browser/device_orientation/provider_impl.cc
@@ -2,7 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <cmath>
 #include <set>
 #include <vector>
 
@@ -11,7 +10,6 @@
 #include "base/message_loop.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
-#include "content/browser/device_orientation/orientation.h"
 #include "content/browser/device_orientation/provider_impl.h"
 
 namespace device_orientation {
@@ -29,11 +27,24 @@ ProviderImpl::~ProviderImpl() {
 void ProviderImpl::AddObserver(Observer* observer) {
   DCHECK(MessageLoop::current() == creator_loop_);
 
+  DeviceData::Type device_data_type = observer->device_data_type();

[hans, 2012/07/18 17:21:02]

maybe move this line down until when device_data_type is first used below,
also i'd be fine with calling this just 'type' or 'data_type' here to hopefully
make some lines below fit within 80 columns

[aousterh, 2012/07/20 11:18:52]

Done.

+
   observers_.insert(observer);
   if (observers_.size() == 1)
     Start();
-  else
-    observer->OnOrientationUpdate(last_notification_);
+
+  ScheduleDoAddObserverType(device_data_type);
+
+  if (!data_fetcher_.get()) {

[hans, 2012/07/18 17:21:02]

just to double check: the idea is that there would be a single data_fetcher_,
and we would call GetDeviceData() on it, passing in which type of data we're
asking for?

[hans, 2012/07/18 17:21:02]

i'm not sure why this is being moved out of Start(). If the provider was already
started, the data_fetcher_ should have been setup, otherwise, wouldn't it make
sense to do it when Start() gets called?

It seems bad to do this start-up stuff in two different places.

[aousterh, 2012/07/20 11:18:52]

I was thinking that we could end up with observers but no fetcher (if no fetcher
could provide that type of data), and in that case we wouldn't want to bother
with polling yet. But I realize now that if we can't find a suitable fetcher we
delete the observer. So that case would never happen, and all the start stuff
can be done together. Done.

[aousterh, 2012/07/20 11:18:52]

Correct. For now there are multiple data fetchers but only one will work on any
given device. In the future, if we wanted this to apply to a much wider range of
data types, we could have multiple fetchers working simultaneously. But I don't
think that would make sense for orientation and motion.

+    ScheduleInitializePollingThread(device_data_type);

[hans, 2012/07/18 17:21:02]

hmm, it makes me worried that we have to pass in device_data_type here.

This will make us look for a DataFetcher that provides that specific type of
data. But if we're only going to have a single DataFetcher, it feels like we
shouldn't be asking if it can provide a specific type of data, but if it can
provide any data at all?

Alternatively, we could have one DataFetcher for each type of data, but that
might turn out to be more complicated.

[aousterh, 2012/07/20 11:18:52]

The scenario I am trying to handle is that first an observer for data type A is
added, but no fetcher can provide type A. Then later an observer for data type B
is added, and the fetcher can provide that kind of data. We don't really want to
start polling after the A-type observer is added, since that will waste
resources. But we will want to start polling for the B-type data later. So we
can't just look for a fetcher that can provide any data - we have to look for
one that can provide the right type of data in order to know whether to
ScheduleDoPoll or not. [this example would apply on mac if we implemented motion
only for android, where A-type = motion and B-type = orientation]

Can you think of a better way to handle this case?

+    return;
+  }
+
+  // Notify observer of most recent notification if one exists.
+  DeviceData* last_notification = last_notifications_map_.Lookup(
+      device_data_type);
+  if (last_notification)

[hans, 2012/07/18 17:21:02]

if it fits on one line, it would be nice to write this as

if (DeviceData* notification = last_notifications_map_.Lookup(type))
  observer->blabla();



also, the ownership issue comes up here.. who owns this object?
last_notifications_ or the observer? should we return a clone here instead? i
think refcounting the data objects would help here.

[aousterh, 2012/07/20 11:18:52]

This no longer applies since I switched to using a map and made DeviceData
RefCounted.

+    observer->OnDeviceDataUpdate(last_notification, device_data_type);
 }
 
 void ProviderImpl::RemoveObserver(Observer* observer) {
@@ -48,13 +59,16 @@ void ProviderImpl::Start() {
   DCHECK(MessageLoop::current() == creator_loop_);
   DCHECK(!polling_thread_.get());
 
-  polling_thread_.reset(new base::Thread("Device orientation polling thread"));
+  // Reset members used on polling thread.
+  last_device_data_map_.reset(new IDMap<DeviceData, IDMapOwnPointer>);
+  observer_types_.clear();
+
+  polling_thread_.reset(new base::Thread("Device data polling thread"));
   if (!polling_thread_->Start()) {
-    LOG(ERROR) << "Failed to start device orientation polling thread";
+    LOG(ERROR) << "Failed to start device data polling thread";
     polling_thread_.reset();
     return;
   }
-  ScheduleInitializePollingThread();
 }
 
 void ProviderImpl::Stop() {
@@ -67,24 +81,45 @@ void ProviderImpl::Stop() {
   data_fetcher_.reset();
 }
 
+void ProviderImpl::DoAddObserverType(DeviceData::Type device_data_type) {
+  DCHECK(MessageLoop::current() == polling_thread_->message_loop());
+
+  observer_types_.insert(device_data_type);
+}
+
+void ProviderImpl::ScheduleDoAddObserverType(
+    DeviceData::Type device_data_type) {

[hans, 2012/07/18 17:21:02]

would it fit on one line if the parameter was just called 'type'?

[aousterh, 2012/07/20 11:18:52]

Done.

+  DCHECK(MessageLoop::current() == creator_loop_);
+
+  MessageLoop* polling_loop = polling_thread_->message_loop();
+  polling_loop->PostTask(FROM_HERE,
+                         base::Bind(&ProviderImpl::DoAddObserverType,
+                                    this,
+                                    device_data_type));
+}
+
 void ProviderImpl::DoInitializePollingThread(
-    const std::vector<DataFetcherFactory>& factories) {
+    const std::vector<DataFetcherFactory>& factories,
+    DeviceData::Type device_data_type) {
   DCHECK(MessageLoop::current() == polling_thread_->message_loop());
 
   typedef std::vector<DataFetcherFactory>::const_iterator Iterator;
   for (Iterator i = factories_.begin(), e = factories_.end(); i != e; ++i) {
     DataFetcherFactory factory = *i;
     scoped_ptr<DataFetcher> fetcher(factory());
-    Orientation orientation;
 
-    if (fetcher.get() && fetcher->GetOrientation(&orientation)) {
+    if (!fetcher.get())
+      continue;
+
+    scoped_ptr<DeviceData> device_data(fetcher->GetDeviceData(
+        device_data_type));
+    if (device_data != NULL) {
       // Pass ownership of fetcher to provider_.
       data_fetcher_.swap(fetcher);
-      last_orientation_ = orientation;
+      last_device_data_map_->AddWithID(device_data->Clone(), device_data_type);
 
       // Notify observers.
-      if (!orientation.is_empty())
-        ScheduleDoNotify(orientation);
+      ScheduleDoNotify(device_data.release(), device_data_type);
 
       // Start polling.
       ScheduleDoPoll();
@@ -92,58 +127,96 @@ void ProviderImpl::DoInitializePollingThread(
     }
   }
 
-  // When no orientation data can be provided.
-  ScheduleDoNotify(Orientation::Empty());
+  // When no device data can be provided.
+  ScheduleDoNotify(NULL, device_data_type);
 }
 
-void ProviderImpl::ScheduleInitializePollingThread() {
+void ProviderImpl::ScheduleInitializePollingThread(
+    DeviceData::Type device_data_type) {
   DCHECK(MessageLoop::current() == creator_loop_);
 
   MessageLoop* polling_loop = polling_thread_->message_loop();
   polling_loop->PostTask(FROM_HERE,
                          base::Bind(&ProviderImpl::DoInitializePollingThread,
                                     this,
-                                    factories_));
+                                    factories_,
+                                    device_data_type));
 }
 
-void ProviderImpl::DoNotify(const Orientation& orientation) {
+void ProviderImpl::DoNotify(const DeviceData* device_data,
+    DeviceData::Type device_data_type) {
   DCHECK(MessageLoop::current() == creator_loop_);
 
-  last_notification_ = orientation;
+  scoped_ptr<const DeviceData> data(device_data);
+
+  // Update last notification of this type.
+  DeviceData* last_notification = last_notifications_map_.Lookup(
+      device_data_type);
+  if (last_notification)
+    last_notifications_map_.Remove(device_data_type);
+  if (data != NULL)
+    last_notifications_map_.AddWithID(data->Clone(), device_data_type);

[hans, 2012/07/18 17:21:02]

i think the if statements above would be simplified if we used a std::map
instead of IDMap.. we could just do something like:
last_notifications_map_[type] = data;

[aousterh, 2012/07/20 11:18:52]

Done.

+
+  // Notify observers of this type of the new data.
+  typedef std::set<Observer*>::const_iterator ConstIterator;
+  for (ConstIterator i = observers_.begin(), e = observers_.end();

[hans, 2012/07/18 17:21:02]

i think "i != observers_.end()" is much more common in chromium than doing the
"i != e" thing.

i happen to know that the original author of the code that does the same thing
on line 107 was just being very stubborn when he wrote it :-p

[aousterh, 2012/07/20 11:18:52]

:-) ok, done. and i changed a couple other instances of the same construction
elsewhere in provider_impl.

+       i != e; ++i) {
+    if ((*i)->device_data_type() == device_data_type)
+      (*i)->OnDeviceDataUpdate(data.get(), device_data_type);
+  }
 
-  typedef std::set<Observer*>::const_iterator Iterator;
-  for (Iterator i = observers_.begin(), e = observers_.end(); i != e; ++i)
-    (*i)->OnOrientationUpdate(orientation);
+  if (data == NULL) {
+    // Notify observers exactly once about failure to provide data.
+    typedef std::set<Observer*>::iterator Iterator;
+    Iterator i = observers_.begin();
+    while (i != observers_.end()) {
+      Iterator current = i++;
+      if ((*current)->device_data_type() == device_data_type)
+        observers_.erase(current);
+    }
 
-  if (orientation.is_empty()) {
-    // Notify observers about failure to provide data exactly once.
-    observers_.clear();
-    Stop();
+    if (observers_.empty())
+      Stop();
   }
 }
 
-void ProviderImpl::ScheduleDoNotify(const Orientation& orientation) {
+void ProviderImpl::ScheduleDoNotify(const DeviceData* device_data,
+    DeviceData::Type device_data_type) {
   DCHECK(MessageLoop::current() == polling_thread_->message_loop());
 
-  creator_loop_->PostTask(
-      FROM_HERE, base::Bind(&ProviderImpl::DoNotify, this, orientation));
+  creator_loop_->PostTask(FROM_HERE,
+                          base::Bind(&ProviderImpl::DoNotify, this,
+                                     device_data, device_data_type));
 }
 
 void ProviderImpl::DoPoll() {
   DCHECK(MessageLoop::current() == polling_thread_->message_loop());
 
-  Orientation orientation;
-  if (!data_fetcher_->GetOrientation(&orientation)) {
-    LOG(ERROR) << "Failed to poll device orientation data fetcher.";
-
-    ScheduleDoNotify(Orientation::Empty());
-    return;
-  }
+  // Poll the fetcher for each type of data

[hans, 2012/07/18 17:21:02]

end comment with period.

[aousterh, 2012/07/20 11:18:52]

Done.

+  typedef std::set<DeviceData::Type>::const_iterator SetIterator;
+  for (SetIterator i = observer_types_.begin(), e = observer_types_.end();
+       i != e; ++i) {
+    DeviceData::Type device_data_type = *i;
+    scoped_ptr<DeviceData> device_data(data_fetcher_->GetDeviceData(
+        device_data_type));
+
+    if (device_data == NULL) {
+      LOG(ERROR) << "Failed to poll device data fetcher.";
+      ScheduleDoNotify(NULL, device_data_type);
+      continue;
+    }
 
-  if (!orientation.is_empty() &&
-      SignificantlyDifferent(orientation, last_orientation_)) {
-    last_orientation_ = orientation;
-    ScheduleDoNotify(orientation);
+    // Update the last device data of this type and notify observers
+    DeviceData* last_device_data = last_device_data_map_->Lookup(
+        device_data_type);
+    if (!last_device_data) {
+      last_device_data_map_->AddWithID(device_data->Clone(), device_data_type);
+      ScheduleDoNotify(device_data.release(), device_data_type);
+    } else if (device_data->SignificantlyDifferentFrom(*last_device_data)) {
+      last_device_data_map_->Remove(device_data_type);
+      last_device_data_map_->AddWithID(device_data->Clone(), device_data_type);
+      ScheduleDoNotify(device_data.release(), device_data_type);
+    }
   }
 
   ScheduleDoPoll();
@@ -159,43 +232,6 @@ void ProviderImpl::ScheduleDoPoll() {
       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) {
-  return IsElementSignificantlyDifferent(o1.can_provide_alpha(),
-                                         o2.can_provide_alpha(),
-                                         o1.alpha(),
-                                         o2.alpha()) ||
-         IsElementSignificantlyDifferent(o1.can_provide_beta(),
-                                         o2.can_provide_beta(),
-                                         o1.beta(),
-                                         o2.beta()) ||
-         IsElementSignificantlyDifferent(o1.can_provide_gamma(),
-                                         o2.can_provide_gamma(),
-                                         o1.gamma(),
-                                         o2.gamma()) ||
-         (o1.can_provide_absolute() != o2.can_provide_absolute() ||
-          o1.absolute() != o2.absolute());
-}
-
 base::TimeDelta ProviderImpl::SamplingInterval() const {
   DCHECK(MessageLoop::current() == polling_thread_->message_loop());
   DCHECK(data_fetcher_.get());