Refactors DeviceOrientation to make it more extensible

Refactors DeviceOrientation to make it more extensible

This refactors the ProviderImpl in DeviceOrientation to make it more general.
This will make it easy to add other types of device data (such as DeviceMotion)
in the future and to have them use the same Provider/ProviderImpl.

This is an alternative to http://codereview.chromium.org/10689106.

TBR=avi
BUG=none
TEST=browser_tests:DeviceOrientationBrowserTest.BasicTest,
content_unittests --gtest_filter=DeviceOrientation*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149864

[aousterh, 2012-07-09 17:07:53]

This is an attempt to refactor ProviderImpl for DeviceOrientation so that
DeviceMotion (and other types of device data in the future) can be added more
easily. It ended up being more complicated and involving more changes than I had
originally hoped (and makes ProviderImpl kind of complicated). If you think that
this refactoring still generally looks like a good idea, I can submit parts of
the change in separate patches for easier review.

[aousterh, 2012-07-11 20:11:25]

I changed DataFetcher so that GetOrientation now returns a pointer to a
DeviceData. This allows Orientations to be constructed in the fetchers rather
than the ProviderImpl, so that the ProviderImpl can simply pass them along to
the MessageFilters rather than having to create them (this eliminates a lot of
static_casts in the fetchers).

I also changed OnDeviceDataUpdate in Provider:Observer to take a pointer instead
of a reference. This allows the ProviderImpl to pass NULL when it cannot provide
data, instead of having to construct an empty DeviceData of the correct type.
This means that we no longer need the static function EmptyDeviceData in
ProviderImpl, and that the ProviderImpl doesn't need any information about the
DeviceData that it passes along, which is very nice.

https://chromiumcodereview.appspot.com/10755002/diff/3001/content/browser/dev...
File content/browser/device_orientation/orientation.h (right):

https://chromiumcodereview.appspot.com/10755002/diff/3001/content/browser/dev...
content/browser/device_orientation/orientation.h:24: absolute_(false),
Somehow those initializations accidentally got added back in. I'll remove them
in my next patch set for this.

[bulach, 2012-07-12 10:43:26]

oh, this is getting into a nice shape, many thanks!!

a few nits and suggestions below, let me know what you think... the most
important ones:
1. decouple the type from the observer, so that a given observer can listen for
multiple datatypes.

2. create / serialize the IPC message in the concrete datatype itself so that
avoids the static_cast higher up.

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
File content/browser/device_orientation/accelerometer_mac.cc (right):

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/accelerometer_mac.cc:29: switch
(device_data_type()) {
nit: since this is not expected to grow, it'd be clearer as:

if (device_data_type() != DeviceData::kDeviceOrientationData)
  return NULL;
return GetOrientation();

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
File content/browser/device_orientation/data_fetcher_impl_android.cc (right):

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/data_fetcher_impl_android.cc:59: switch
(device_data_type) {
nit: as above..

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
File content/browser/device_orientation/device_data.h (right):

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/device_data.h:8: #include <cmath>
nit: best to move this (and the method implementation) to the cc file..

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/device_data.h:10: namespace
device_orientation {
nit: add a \n here..

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/device_data.h:13: enum DeviceDataType {
nit: since it's already scoped to DeviceData, it'd probably be clearer to name
this just Type..

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/device_data.h:14: kDeviceOrientationData,
nit: also, the enum values are normally named as k${ENUM_NAME}Value..

so these would be either kDeviceDataTypeOrientation, or if you agree with the
above, just kTypeOrientation.

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/device_data.h:23: static bool
IsElementSignificantlyDifferent(bool can_provide_element1,
nit: as above, probably best to move this to the .cc file to simplify the API..

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
File content/browser/device_orientation/orientation.cc (right):

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/orientation.cc:10: Orientation* orientation =
new Orientation();
nit: odd indentation, remove one space from 10-19

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/orientation.cc:24: && !can_provide_absolute_;
nit: I think the && needs to be in the previous line (or break right after the
other one..)

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/orientation.cc:46: (can_provide_absolute_ !=
other_orientation.can_provide_absolute() ||
nit: no need for this parens.

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/orientation.cc:50: };  // namespace
device_orientation
nit: remove the ;

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
File content/browser/device_orientation/orientation.h (right):

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/orientation.h:11: namespace
device_orientation {
nit: add a \n here, and the following line needs to be unindented..

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/orientation.h:31: static Orientation Empty()
{ return Orientation(); }
nit: I think we could remove this and just use Orientation() directly where
needed..

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
File content/browser/device_orientation/orientation_message_filter.cc (right):

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/orientation_message_filter.cc:52: :
provider_(provider),
nit: I think the initializer list needs to be indented by 4..

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/orientation_message_filter.cc:67: const
Orientation* orientation = static_cast<const Orientation*>(device_data);
hmm... I'm not a 100% of this suggestion, but here we go.. :)
what if we had a virtual Message* CreateIPCMessage() const; in DeviceData
itself?
then this layer would be completely transparent to it...

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
File content/browser/device_orientation/provider.h (right):

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/provider.h:26: // Each Observer observes
exactly one type of DeviceData
nit: members must be private...
please, add a constructor that takes and sets this param. however, why is that
restriction? a single observer should be able to listen to multiple data types,
right?
perhaps it should be a param to "AddObserver" rather than in the observer
itself?

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
File content/browser/device_orientation/provider_impl.cc (right):

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/provider_impl.cc:208: return;
hmm, I think now that we're traversing all the different datatypes we should not
return (otherwise, a failing datatype would prevent all other providers from
updating..)

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/provider_impl.cc:218: else if
(device_data->SignificantlyDifferentFrom(*last_device_data)) {
nit: else on the line above with }

[aousterh, 2012-07-12 17:13:57]

Thanks for the feedback! I made the changes you suggested, except for the one
described below:

> 1. decouple the type from the observer, so that a given observer can listen
for
> multiple datatypes.

I think it is best if each observer listens for one datatype. Then, we can have
one message filter per data type, with its own set of observers of that type.
This is convenient because it is easy to manage things like StartUpdating and
StopUpdating, which apply to all observers of one datatype. In this way, the
observers demultiplex updates based on datatype, and we don't have to do it in
the message filter. This also simplifies the ProviderImpl. To make it easier to
have multiple different message filters, I made a base class MessageFilter.

I also rebased.

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
File content/browser/device_orientation/accelerometer_mac.cc (right):

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/accelerometer_mac.cc:29: switch
(device_data_type()) {
On 2012/07/12 10:43:27, bulach wrote:
> nit: since this is not expected to grow, it'd be clearer as:
> 
> if (device_data_type() != DeviceData::kDeviceOrientationData)
>   return NULL;
> return GetOrientation();

Done.

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
File content/browser/device_orientation/data_fetcher_impl_android.cc (right):

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/data_fetcher_impl_android.cc:59: switch
(device_data_type) {
On 2012/07/12 10:43:27, bulach wrote:
> nit: as above..

Actually for this one, I will (hopefully) be adding DeviceMotion soon. I can
change it to the if-else way for now if you'd prefer, but in the long run it
probably makes more sense as a switch-case statement.

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
File content/browser/device_orientation/device_data.h (right):

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/device_data.h:8: #include <cmath>
On 2012/07/12 10:43:27, bulach wrote:
> nit: best to move this (and the method implementation) to the cc file..

Done.

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/device_data.h:10: namespace
device_orientation {
On 2012/07/12 10:43:27, bulach wrote:
> nit: add a \n here..

Done.

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/device_data.h:13: enum DeviceDataType {
On 2012/07/12 10:43:27, bulach wrote:
> nit: since it's already scoped to DeviceData, it'd probably be clearer to name
> this just Type..

Done.

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/device_data.h:14: kDeviceOrientationData,
On 2012/07/12 10:43:27, bulach wrote:
> nit: also, the enum values are normally named as k${ENUM_NAME}Value..
> 
> so these would be either kDeviceDataTypeOrientation, or if you agree with the
> above, just kTypeOrientation.

Done.

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/device_data.h:23: static bool
IsElementSignificantlyDifferent(bool can_provide_element1,
On 2012/07/12 10:43:27, bulach wrote:
> nit: as above, probably best to move this to the .cc file to simplify the
API..

Done.

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
File content/browser/device_orientation/orientation.cc (right):

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/orientation.cc:10: Orientation* orientation =
new Orientation();
On 2012/07/12 10:43:27, bulach wrote:
> nit: odd indentation, remove one space from 10-19

Done.

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/orientation.cc:24: && !can_provide_absolute_;
On 2012/07/12 10:43:27, bulach wrote:
> nit: I think the && needs to be in the previous line (or break right after the
> other one..)

Done.

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/orientation.cc:46: (can_provide_absolute_ !=
other_orientation.can_provide_absolute() ||
On 2012/07/12 10:43:27, bulach wrote:
> nit: no need for this parens.

Done.

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/orientation.cc:50: };  // namespace
device_orientation
On 2012/07/12 10:43:27, bulach wrote:
> nit: remove the ;

Done.

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
File content/browser/device_orientation/orientation.h (right):

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/orientation.h:11: namespace
device_orientation {
On 2012/07/12 10:43:27, bulach wrote:
> nit: add a \n here, and the following line needs to be unindented..

Done.

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/orientation.h:24: absolute_(false),
On 2012/07/11 20:11:25, aousterh wrote:
> Somehow those initializations accidentally got added back in. I'll remove them
> in my next patch set for this.

Done.

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/orientation.h:31: static Orientation Empty()
{ return Orientation(); }
On 2012/07/12 10:43:27, bulach wrote:
> nit: I think we could remove this and just use Orientation() directly where
> needed..

Done.

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
File content/browser/device_orientation/orientation_message_filter.cc (right):

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/orientation_message_filter.cc:52: :
provider_(provider),
On 2012/07/12 10:43:27, bulach wrote:
> nit: I think the initializer list needs to be indented by 4..

Done.

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/orientation_message_filter.cc:67: const
Orientation* orientation = static_cast<const Orientation*>(device_data);
On 2012/07/12 10:43:27, bulach wrote:
> hmm... I'm not a 100% of this suggestion, but here we go.. :)
> what if we had a virtual Message* CreateIPCMessage() const; in DeviceData
> itself?
> then this layer would be completely transparent to it...

Done.

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
File content/browser/device_orientation/provider.h (right):

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/provider.h:26: // Each Observer observes
exactly one type of DeviceData
On 2012/07/12 10:43:27, bulach wrote:
> nit: members must be private...
> please, add a constructor that takes and sets this param. however, why is that
> restriction? a single observer should be able to listen to multiple data
types,
> right?
> perhaps it should be a param to "AddObserver" rather than in the observer
> itself?

Done (regarding the private members).

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
File content/browser/device_orientation/provider_impl.cc (right):

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/provider_impl.cc:208: return;
On 2012/07/12 10:43:27, bulach wrote:
> hmm, I think now that we're traversing all the different datatypes we should
not
> return (otherwise, a failing datatype would prevent all other providers from
> updating..)

Good point. Done.

http://codereview.chromium.org/10755002/diff/3001/content/browser/device_orie...
content/browser/device_orientation/provider_impl.cc:218: else if
(device_data->SignificantlyDifferentFrom(*last_device_data)) {
On 2012/07/12 10:43:27, bulach wrote:
> nit: else on the line above with }

Done.

[aousterh, 2012-07-18 09:42:16]

I fixed a small bug in this patch in observer_delegate.cc which occurred when
NULL device_data was returned. This bug was not uncovered by the browser test or
provider unittests, which makes me think that perhaps there should be more
device orientation tests somewhere.

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

a bunch of comments below..

also, one important thing to bear in mind is that there's a difference between
the DeviceOrientation and DeviceMotion events, except for the type of data: the
orientation events shall fire whenever the orientation changes above some
threshold, whereas the motion events should fire at a regular interval.

We can probably achieve this with the current approach, though, by making
isSignificantlyDifferent always return true for the device motion data.. that
way, we will fire new events at the polling interval.

Alternatively, we can change the logic in the provider to look at the type of
the data, and call isSignificantlyDifferent or just always fire the event.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
File content/browser/device_orientation/data_fetcher_impl_android.cc (right):

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/data_fetcher_impl_android.cc:58:
DeviceData::Type* device_data_type) {
feel free to just call the parameter 'type' here instead if that makes it fit on
one line

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/data_fetcher_impl_android.cc:59: switch
(device_data_type) {
i'd just use an if-else.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
File content/browser/device_orientation/device_data.h (right):

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/device_data.h:8: #include "ipc/ipc_message.h"
i guess this is for IPC::Message?
You could just forward-declare it like this:

namespace IPC {
class Message;
}

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/device_data.h:12: class DeviceData {
Should this class be RefCounted? (RefCountedThreadSafe if we're accessing it
from different threads) Now that we're going to be passing around pointers to
DeviceData where we were previously passing and storing Orientation objects by
value, the ownership situation seems to become a bit unclear..

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/device_data.h:19: virtual DeviceData* Clone()
const = 0;
if we make it ref counted, we might not need to Clone() it..

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/device_data.h:24: static bool
IsElementSignificantlyDifferent(bool can_provide_element1,
hmm, we might actually want to implement the notion of being significantly
different differently for different types of data. Maybe keep this in the
derived class for now.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
File content/browser/device_orientation/device_orientation_browsertest.cc
(right):

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/device_orientation_browsertest.cc:23:
explicit MockProvider(const Orientation* orientation,
no need for explicit here

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
File content/browser/device_orientation/observer_delegate.cc (right):

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/observer_delegate.cc:43: DeviceData*
ObserverDelegate::EmptyDeviceData(DeviceData::Type type) {
maybe we should just DCHECK that type is kTypeOrientation here? It's annoying to
have to have code for the case where this function returns NULL. I think it
should be written so that the caller can assume it doesn't return NULL.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
File content/browser/device_orientation/observer_delegate.h (right):

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/observer_delegate.h:16: // Create
ObserverDelegate that observes provider and forwards updates to
this and the lines below are indented too much

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
File content/browser/device_orientation/orientation_message_filter.cc (right):

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/orientation_message_filter.cc:16: : 
MessageFilter(DeviceData::kTypeOrientation) {
i think this should be indented more

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
File content/browser/device_orientation/provider_impl.cc (right):

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:30: DeviceData::Type
device_data_type = observer->device_data_type();
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

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:38: if
(!data_fetcher_.get()) {
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?

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:38: if
(!data_fetcher_.get()) {
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.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:39:
ScheduleInitializePollingThread(device_data_type);
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.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:46: if (last_notification)
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.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:91: DeviceData::Type
device_data_type) {
would it fit on one line if the parameter was just called 'type'?

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:158:
last_notifications_map_.AddWithID(data->Clone(), device_data_type);
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;

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:162: for (ConstIterator i =
observers_.begin(), e = observers_.end();
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

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:195: // Poll the fetcher for
each type of data
end comment with period.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
File content/browser/device_orientation/provider_impl.h (right):

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.h:47: // Method for adding a
type of observer to poll. Runs on the polling_thread_.
we're not really polling the observers, though? Maybe something like "Add
another type of data to poll for. Runs on the polling_thread_."

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.h:79: IDMap<DeviceData,
IDMapOwnPointer> last_notifications_map_;
I'm not sure IDMap is the best for this. Could we just use

std::map<DeviceData::Type, scoped_ptr<DeviceData> > instead?

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.h:84:
scoped_ptr<IDMap<DeviceData, IDMapOwnPointer> > last_device_data_map_;
same here

[aousterh, 2012-07-20 11:18:31]

On 2012/07/18 17:21:02, hans wrote:
> a bunch of comments below..
> 
> also, one important thing to bear in mind is that there's a difference between
> the DeviceOrientation and DeviceMotion events, except for the type of data:
the
> orientation events shall fire whenever the orientation changes above some
> threshold, whereas the motion events should fire at a regular interval.
> 
> We can probably achieve this with the current approach, though, by making
> isSignificantlyDifferent always return true for the device motion data.. that
> way, we will fire new events at the polling interval.
> 

As long as the fetching interval is at least as short as the polling interval, I
think this approach will work well.

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

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
File content/browser/device_orientation/data_fetcher.h (right):

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/data_fetcher.h:19: virtual DeviceData*
GetDeviceData(DeviceData::Type device_data_type) = 0;
I changed this to const DeviceData*, since it shouldn't be modified by the
provider, message_filter, etc.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
File content/browser/device_orientation/data_fetcher_impl_android.cc (right):

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/data_fetcher_impl_android.cc:58:
DeviceData::Type* device_data_type) {
On 2012/07/18 17:21:02, hans wrote:
> feel free to just call the parameter 'type' here instead if that makes it fit
on
> one line

Done.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/data_fetcher_impl_android.cc:59: switch
(device_data_type) {
On 2012/07/18 17:21:02, hans wrote:
> i'd just use an if-else.

Done.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
File content/browser/device_orientation/device_data.h (right):

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/device_data.h:8: #include "ipc/ipc_message.h"
On 2012/07/18 17:21:02, hans wrote:
> i guess this is for IPC::Message?
> You could just forward-declare it like this:
> 
> namespace IPC {
> class Message;
> }

Done.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/device_data.h:12: class DeviceData {
On 2012/07/18 17:21:02, hans wrote:
> Should this class be RefCounted? (RefCountedThreadSafe if we're accessing it
> from different threads) Now that we're going to be passing around pointers to
> DeviceData where we were previously passing and storing Orientation objects by
> value, the ownership situation seems to become a bit unclear..

Done.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/device_data.h:19: virtual DeviceData* Clone()
const = 0;
On 2012/07/18 17:21:02, hans wrote:
> if we make it ref counted, we might not need to Clone() it..

Done.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/device_data.h:24: static bool
IsElementSignificantlyDifferent(bool can_provide_element1,
On 2012/07/18 17:21:02, hans wrote:
> hmm, we might actually want to implement the notion of being significantly
> different differently for different types of data. Maybe keep this in the
> derived class for now.

ok, i moved IsElementSignificantlyDifferent to the derived class (Orientation),
and replaced SignificantlyDifferentFrom with ShouldFireEvent in the base class,
to determine from the old device data whether an event should be fired. I think
the name is slightly confusing though - it's not clear why the function is
called on one DeviceData with another as an argument, so if you have any
suggestions, please let me know!

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
File content/browser/device_orientation/device_orientation_browsertest.cc
(right):

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/device_orientation_browsertest.cc:23:
explicit MockProvider(const Orientation* orientation,
On 2012/07/18 17:21:02, hans wrote:
> no need for explicit here

Done. Also changed orientation->device data so that the MockProvider can be used
for other types of data in the future.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/device_orientation_browsertest.cc:39:
scoped_ptr<const Orientation> orientation_;
Changing this to device_data_ instead of orientation_ so that it can be used in
other future tests.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
File content/browser/device_orientation/observer_delegate.cc (right):

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/observer_delegate.cc:43: DeviceData*
ObserverDelegate::EmptyDeviceData(DeviceData::Type type) {
On 2012/07/18 17:21:02, hans wrote:
> maybe we should just DCHECK that type is kTypeOrientation here? It's annoying
to
> have to have code for the case where this function returns NULL. I think it
> should be written so that the caller can assume it doesn't return NULL.

Done.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
File content/browser/device_orientation/observer_delegate.h (right):

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/observer_delegate.h:16: // Create
ObserverDelegate that observes provider and forwards updates to
On 2012/07/18 17:21:02, hans wrote:
> this and the lines below are indented too much

Done.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
File content/browser/device_orientation/orientation_message_filter.cc (right):

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/orientation_message_filter.cc:16: : 
MessageFilter(DeviceData::kTypeOrientation) {
On 2012/07/18 17:21:02, hans wrote:
> i think this should be indented more

Done.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
File content/browser/device_orientation/provider_impl.cc (right):

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:30: DeviceData::Type
device_data_type = observer->device_data_type();
On 2012/07/18 17:21:02, hans wrote:
> 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

Done.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:38: if
(!data_fetcher_.get()) {
On 2012/07/18 17:21:02, hans wrote:
> 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.

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.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:38: if
(!data_fetcher_.get()) {
On 2012/07/18 17:21:02, hans wrote:
> 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?

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.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:39:
ScheduleInitializePollingThread(device_data_type);
On 2012/07/18 17:21:02, hans wrote:
> 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.

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?

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:46: if (last_notification)
On 2012/07/18 17:21:02, hans wrote:
> 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.

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

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:91: DeviceData::Type
device_data_type) {
On 2012/07/18 17:21:02, hans wrote:
> would it fit on one line if the parameter was just called 'type'?

Done.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:158:
last_notifications_map_.AddWithID(data->Clone(), device_data_type);
On 2012/07/18 17:21:02, hans wrote:
> 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;

Done.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:162: for (ConstIterator i =
observers_.begin(), e = observers_.end();
On 2012/07/18 17:21:02, hans wrote:
> 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

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

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:195: // Poll the fetcher for
each type of data
On 2012/07/18 17:21:02, hans wrote:
> end comment with period.

Done.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
File content/browser/device_orientation/provider_impl.h (right):

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.h:47: // Method for adding a
type of observer to poll. Runs on the polling_thread_.
On 2012/07/18 17:21:02, hans wrote:
> we're not really polling the observers, though? Maybe something like "Add
> another type of data to poll for. Runs on the polling_thread_."

Done. I also renamed the function and member observer_types_.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.h:79: IDMap<DeviceData,
IDMapOwnPointer> last_notifications_map_;
On 2012/07/18 17:21:02, hans wrote:
> I'm not sure IDMap is the best for this. Could we just use
> 
> std::map<DeviceData::Type, scoped_ptr<DeviceData> > instead?

I used IDMap because it's not possible to use std::map with scoped_ptrs, as far
as I can tell. However, now that I made DeviceData RefCounted, this can be a
std::map<DeviceData::Type, scoped_refptr<DeviceData> >. Done.

http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.h:84:
scoped_ptr<IDMap<DeviceData, IDMapOwnPointer> > last_device_data_map_;
On 2012/07/18 17:21:02, hans wrote:
> same here

Done.

[aousterh, 2012-07-20 13:00:27]

I realized that the function IsEmpty in DeviceData and Orientation is now
unused, so I removed it.

[hans, 2012-07-30 16:12:52]

Apologies for the slow reply.

On 2012/07/20 11:18:52, aousterh wrote:
>
http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
> content/browser/device_orientation/device_data.h:24: static bool
> IsElementSignificantlyDifferent(bool can_provide_element1,
> On 2012/07/18 17:21:02, hans wrote:
> > hmm, we might actually want to implement the notion of being significantly
> > different differently for different types of data. Maybe keep this in the
> > derived class for now.
> 
> ok, i moved IsElementSignificantlyDifferent to the derived class
(Orientation),
> and replaced SignificantlyDifferentFrom with ShouldFireEvent in the base
class,
> to determine from the old device data whether an event should be fired. I
think
> the name is slightly confusing though - it's not clear why the function is
> called on one DeviceData with another as an argument, so if you have any
> suggestions, please let me know!
> 

I have no great suggestions at the moment :/



>
http://codereview.chromium.org/10755002/diff/20001/content/browser/device_ori...
> content/browser/device_orientation/provider_impl.cc:39:
> ScheduleInitializePollingThread(device_data_type);
> On 2012/07/18 17:21:02, hans wrote:
> > 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.
> 
> 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?


Hmm, I guess this works. It just doesn't feel entirely obvious when reading the
code... we call Start(data_type), and go looking for a data fetcher that
provides that type. It's not made entirely clear that that fetcher is also
expected to provide the other types as well. Maybe this can be fixed by better
commenting or naming or something.

We should also have a unit test for the scenario you described.

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
File content/browser/device_orientation/data_fetcher_impl_android.cc (right):

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
content/browser/device_orientation/data_fetcher_impl_android.cc:58:
DeviceData::Type* type) {
hmm, should "Type*" be "Type"?

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
File content/browser/device_orientation/device_data.h (right):

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
content/browser/device_orientation/device_data.h:26: DeviceData() {}
i'm not sure this constructor is necessary?

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
File content/browser/device_orientation/observer_delegate.h (right):

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
content/browser/device_orientation/observer_delegate.h:21: // render_view_id in
process_id.
the "in process_id" part of the comment doesn't seem to apply anymore

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
File content/browser/device_orientation/provider.h (right):

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
content/browser/device_orientation/provider.h:32: // Each Observer observes
exactly one type of DeviceData
nit: period after end of sentence.

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
File content/browser/device_orientation/provider_impl.cc (right):

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:154: if (data != NULL)
do we need this if statement?

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:209: if (iter !=
last_device_data_map_->end() &&
maybe a comment here would help?

or maybe we don't actually need to worry about inserting NULL device data
pointers in that map, maybe we could just do

scoped_refptr<const DeviceData> last_data = last_device_data_map[type];
if (!device_data->ShouldFireEvent(last_data))
  continue;

it would reduce the complexity a bit if instead of doing the .find() and !=
end() thing everywhere, we could just check for last_device_data_map_[type]
being null.

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:213: // Update the last
device data of this type and notify observers
nit: period.

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:214: if
(device_data->ShouldFireEvent((*iter).second))
i'm confused by this and the two lines below..
don't we just want something like 

last_device_data_map[data_type] = device_data;

[aousterh, 2012-08-03 11:08:14]

I added the unit test you recommended, which involved making a lot of changes to
provider_unittest in order to make it general enough to handle multiple types of
device_data.

I also rebased, so there are several changes from that (mostly due to
https://chromiumcodereview.appspot.com/10835030/ and
https://chromiumcodereview.appspot.com/10837055/).

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
File content/browser/device_orientation/data_fetcher_impl_android.cc (right):

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
content/browser/device_orientation/data_fetcher_impl_android.cc:58:
DeviceData::Type* type) {
On 2012/07/30 16:12:52, hans wrote:
> hmm, should "Type*" be "Type"?

Oops, thanks for catching that. Done.

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
File content/browser/device_orientation/device_data.h (right):

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
content/browser/device_orientation/device_data.h:26: DeviceData() {}
On 2012/07/30 16:12:52, hans wrote:
> i'm not sure this constructor is necessary?

I believe it is necessary because any constructor in a derived class needs to
call a constructor of the base class. 

If I remove it, I get the compiler error "constructor for
'device_orientation::Orientation' must explicitly initialize the base class
'device_orientation::DeviceData' which does not have a default constructor'.

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
File content/browser/device_orientation/observer_delegate.h (right):

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
content/browser/device_orientation/observer_delegate.h:21: // render_view_id in
process_id.
On 2012/07/30 16:12:52, hans wrote:
> the "in process_id" part of the comment doesn't seem to apply anymore

Done.

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
File content/browser/device_orientation/provider.h (right):

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
content/browser/device_orientation/provider.h:32: // Each Observer observes
exactly one type of DeviceData
On 2012/07/30 16:12:52, hans wrote:
> nit: period after end of sentence.

Done.

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
File content/browser/device_orientation/provider_impl.cc (right):

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:154: if (data != NULL)
On 2012/07/30 16:12:52, hans wrote:
> do we need this if statement?

Done.

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:209: if (iter !=
last_device_data_map_->end() &&
On 2012/07/30 16:12:52, hans wrote:
> maybe a comment here would help?
> 
> or maybe we don't actually need to worry about inserting NULL device data
> pointers in that map, maybe we could just do
> 
> scoped_refptr<const DeviceData> last_data = last_device_data_map[type];
> if (!device_data->ShouldFireEvent(last_data))
>   continue;
> 
> it would reduce the complexity a bit if instead of doing the .find() and !=
> end() thing everywhere, we could just check for last_device_data_map_[type]
> being null.

I did it that way because last_device_data_map_ was a scoped_ptr to a map, so
that the map could be reset whenever the polling thread was restarted. Now that
PollingThread is a separate class that isn't a problem anymore, and
last_device_data_map_ can simply be a member of PollingThread instead of a
scoped_ptr in ProviderImpl, and we can easily use the [] operator. Done.

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:213: // Update the last
device data of this type and notify observers
On 2012/07/30 16:12:52, hans wrote:
> nit: period.

Done.

http://codereview.chromium.org/10755002/diff/37001/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:214: if
(device_data->ShouldFireEvent((*iter).second))
On 2012/07/30 16:12:52, hans wrote:
> i'm confused by this and the two lines below..
> don't we just want something like 
> 
> last_device_data_map[data_type] = device_data;

Done.

[hans, 2012-08-03 13:05:41]

just few minor nits, otherwise LGTM

http://codereview.chromium.org/10755002/diff/46002/content/browser/device_ori...
File content/browser/device_orientation/data_fetcher_impl_android.cc (right):

http://codereview.chromium.org/10755002/diff/46002/content/browser/device_ori...
content/browser/device_orientation/data_fetcher_impl_android.cc:75: return
orientation;
i think we can simplify this to something like:

if (!current_orientation_.get())
  return new Orientation();
return current_orientation_.get();

http://codereview.chromium.org/10755002/diff/46002/content/browser/device_ori...
File content/browser/device_orientation/data_fetcher_impl_android.h (right):

http://codereview.chromium.org/10755002/diff/46002/content/browser/device_ori...
content/browser/device_orientation/data_fetcher_impl_android.h:54:
scoped_ptr<Orientation> current_orientation_;
should this be a scoped_refptr?

http://codereview.chromium.org/10755002/diff/46002/content/browser/device_ori...
File content/browser/device_orientation/message_filter.h (right):

http://codereview.chromium.org/10755002/diff/46002/content/browser/device_ori...
content/browser/device_orientation/message_filter.h:17: class Provider;
i'd add an extra newline before this, to make it clearer that the comment about
ObserverDelegate doesn't apply to Provider

http://codereview.chromium.org/10755002/diff/46002/content/browser/device_ori...
File content/browser/device_orientation/provider_impl.cc (right):

http://codereview.chromium.org/10755002/diff/46002/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:295: 
ultra nit: a blank line too much here

http://codereview.chromium.org/10755002/diff/46002/content/browser/device_ori...
File content/browser/device_orientation/provider_unittest.cc (right):

http://codereview.chromium.org/10755002/diff/46002/content/browser/device_ori...
content/browser/device_orientation/provider_unittest.cc:29: return NULL;
maybe add a CHECK or DCHECK here, just to make sure it never gets executed?

[aousterh, 2012-08-03 13:42:18]

Thanks for the comments! Does this look ok?

http://codereview.chromium.org/10755002/diff/46002/content/browser/device_ori...
File content/browser/device_orientation/data_fetcher_impl_android.cc (right):

http://codereview.chromium.org/10755002/diff/46002/content/browser/device_ori...
content/browser/device_orientation/data_fetcher_impl_android.cc:75: return
orientation;
On 2012/08/03 13:05:41, hans wrote:
> i think we can simplify this to something like:
> 
> if (!current_orientation_.get())
>   return new Orientation();
> return current_orientation_.get();

Done.

http://codereview.chromium.org/10755002/diff/46002/content/browser/device_ori...
File content/browser/device_orientation/data_fetcher_impl_android.h (right):

http://codereview.chromium.org/10755002/diff/46002/content/browser/device_ori...
content/browser/device_orientation/data_fetcher_impl_android.h:54:
scoped_ptr<Orientation> current_orientation_;
On 2012/08/03 13:05:41, hans wrote:
> should this be a scoped_refptr?

Done.

http://codereview.chromium.org/10755002/diff/46002/content/browser/device_ori...
File content/browser/device_orientation/message_filter.h (right):

http://codereview.chromium.org/10755002/diff/46002/content/browser/device_ori...
content/browser/device_orientation/message_filter.h:17: class Provider;
On 2012/08/03 13:05:41, hans wrote:
> i'd add an extra newline before this, to make it clearer that the comment
about
> ObserverDelegate doesn't apply to Provider

Done.

http://codereview.chromium.org/10755002/diff/46002/content/browser/device_ori...
File content/browser/device_orientation/provider_impl.cc (right):

http://codereview.chromium.org/10755002/diff/46002/content/browser/device_ori...
content/browser/device_orientation/provider_impl.cc:295: 
On 2012/08/03 13:05:41, hans wrote:
> ultra nit: a blank line too much here

Done.

http://codereview.chromium.org/10755002/diff/46002/content/browser/device_ori...
File content/browser/device_orientation/provider_unittest.cc (right):

http://codereview.chromium.org/10755002/diff/46002/content/browser/device_ori...
content/browser/device_orientation/provider_unittest.cc:29: return NULL;
On 2012/08/03 13:05:41, hans wrote:
> maybe add a CHECK or DCHECK here, just to make sure it never gets executed?

Done.

[commit-bot: I haz the power, 2012-08-03 13:53:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aousterh@chromium.org/10755002/52024

[commit-bot: I haz the power, 2012-08-03 13:53:16]

Presubmit check for 10755002-52024 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content

Presubmit checks took 2.1s to calculate.

[hans, 2012-08-03 13:55:36]

On 2012/08/03 13:53:16, I haz the power (commit-bot) wrote:
> Presubmit check for 10755002-52024 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     content

since it's just a .gypi change, i think we can TBR the owner

[commit-bot: I haz the power, 2012-08-03 13:56:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aousterh@chromium.org/10755002/52024

[commit-bot: I haz the power, 2012-08-03 14:12:49]

Try job failure for 10755002-52024 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-08-03 15:52:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aousterh@chromium.org/10755002/39006

[commit-bot: I haz the power, 2012-08-03 17:09:57]

Change committed as 149864

[Steve Block, 2013-05-27 01:36:03]

Just noticed that I have a very old draft comment for this, but can we delete
the issue?

https://codereview.chromium.org/10755002/diff/1/content/browser/device_orient...
File content/browser/device_orientation/device_data.h (right):

https://codereview.chromium.org/10755002/diff/1/content/browser/device_orient...
content/browser/device_orientation/device_data.h:28: if (can_provide_element1 !=
can_provide_element2)
I'm not sure it makes sense for this to be in the base class. I think this kind
of logic depends on the data type (or even on the provider). For example, for
orientation, if two pieces of data are identical except the newer data can't
provide a certain property, the provider probably shouldn't update its listeners
with this.

[hans, 2013-05-28 08:53:55]

On 2013/05/27 01:36:03, Steve Block wrote:
> Just noticed that I have a very old draft comment for this, but can we delete
> the issue?

The issue is already closed and the patch was landed. Is it still showing up on
your dashboard as active in some way?