Chromium plumbing for Device Orientation.

Chromium plumbing for Device Orientation.

Add the plumbing needed for communicating with the Device Orientation code in WebKit.

RenderView provides an implementation of WebKit::WebDeviceOrientationClient: DeviceOrientationDispatcher. This communicates with the browser-side class device_orientation::DispatcherHost.

device_orientation::Provider, responsible for providing the orientation data, is just an empty shell for now.

BUG=44654
TEST=browser_tests --gtest_filter=DeviceOrientationBrowserTest.BasicTest

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

[hans, 2010-07-06 15:59:54]

This is the first in a series of patches to implement Device Orientation in
Chrome. It's broken up into pieces to make review easier.

To make the code live, one needs the WebKit patch from
https://bugs.webkit.org/show_bug.cgi?id=41616, as well as enable the
ENABLE_DEVICE_ORIENTATION switch in build/features_override.gypi.

Submitted early as this is my first larger patch and it might need some polish;
all comments welcome!

[bulach, 2010-07-06 17:01:59]

good stuff Hans!

once the WebKit side is in, would it be possible to write some browser tests to
exercise the bindings / plumbing? if at all possible, it'd be great to have them
in this patch.

a few comments and one design question below that I hope it'll simplify the
implementation:

http://codereview.chromium.org/2858049/diff/1/2
File chrome/browser/device_orientation/arbitrator.h (right):

http://codereview.chromium.org/2858049/diff/1/2#newcode17
chrome/browser/device_orientation/arbitrator.h:17: class Arbitrator : public
base::RefCounted<Arbitrator> {
does any of the platforms would implement multiple providers?
perhaps we could simplify and have just a single provider and avoid arbitrating?

in geolocation, we have GPS and Network provider with different degrees of
accuracy and power consumption, and the javascript API exposes a way to choose
low / high accuracy.
would this be the case for orientation as well?

http://codereview.chromium.org/2858049/diff/1/2#newcode31
chrome/browser/device_orientation/arbitrator.h:31: static Arbitrator*
GetInstance() {
I think GetInstance() is normally associated with singleton pattern.
specifically in this case, the caller could just call the ctor?

http://codereview.chromium.org/2858049/diff/1/4
File chrome/browser/device_orientation/dispatcher_host.h (right):

http://codereview.chromium.org/2858049/diff/1/4#newcode21
chrome/browser/device_orientation/dispatcher_host.h:21: static DispatcherHost*
New(int resource_message_filter_process_id);
as above, if the factory method is not doing anything fancy, it may be simpler
to just let the caller use the regular ctor..

[hans, 2010-07-06 17:16:11]

On 2010/07/06 17:01:59, bulach wrote:
> good stuff Hans!
Thanks for the review :)

> 
> once the WebKit side is in, would it be possible to write some browser tests
to
> exercise the bindings / plumbing? if at all possible, it'd be great to have
them
> in this patch.
Hmm, yes such tests would certainly be desirable. My plan was to do a browser
test that goes end-to-end, including the Arbitrator (which will come in the next
patch). But perhaps one could write a test already at this stage that uses a
mock Arbitrator. I will work on this.

> 
> a few comments and one design question below that I hope it'll simplify the
> implementation:
> 
> http://codereview.chromium.org/2858049/diff/1/2
> File chrome/browser/device_orientation/arbitrator.h (right):
> 
> http://codereview.chromium.org/2858049/diff/1/2#newcode17
> chrome/browser/device_orientation/arbitrator.h:17: class Arbitrator : public
> base::RefCounted<Arbitrator> {
> does any of the platforms would implement multiple providers?
> perhaps we could simplify and have just a single provider and avoid
arbitrating?
> 
> in geolocation, we have GPS and Network provider with different degrees of
> accuracy and power consumption, and the javascript API exposes a way to choose
> low / high accuracy.
> would this be the case for orientation as well?
The name 'arbitrator' may be poorly chosen. It does not do the same thing as in
geolocation; there will never be more than one provider used at the same time.
The arbitrator's job in this case is merely to choose one that works and poll it
on background thread. Suggestions for better name is very welcome :)
ProviderManager?

> 
> http://codereview.chromium.org/2858049/diff/1/2#newcode31
> chrome/browser/device_orientation/arbitrator.h:31: static Arbitrator*
> GetInstance() {
> I think GetInstance() is normally associated with singleton pattern.
> specifically in this case, the caller could just call the ctor?
Yes, the real arbitrator implementation will be a singleton. Right now, I just
call new in there so that it compiles.

> 
> http://codereview.chromium.org/2858049/diff/1/4
> File chrome/browser/device_orientation/dispatcher_host.h (right):
> 
> http://codereview.chromium.org/2858049/diff/1/4#newcode21
> chrome/browser/device_orientation/dispatcher_host.h:21: static DispatcherHost*
> New(int resource_message_filter_process_id);
> as above, if the factory method is not doing anything fancy, it may be simpler
> to just let the caller use the regular ctor..
As above, this is needed since the arbitrator implementation will be a
singleton.

[bulach, 2010-07-06 17:48:50]

On Tue, Jul 6, 2010 at 6:16 PM,  <hans@chromium.org> wrote:
> On 2010/07/06 17:01:59, bulach wrote:
>>
>> good stuff Hans!
>
> Thanks for the review :)
>
>
>> once the WebKit side is in, would it be possible to write some browser
>> tests
>
> to
>>
>> exercise the bindings / plumbing? if at all possible, it'd be great to
>> have
>
> them
>>
>> in this patch.
>
> Hmm, yes such tests would certainly be desirable. My plan was to do a
> browser
> test that goes end-to-end, including the Arbitrator (which will come in the
> next
> patch). But perhaps one could write a test already at this stage that uses a
> mock Arbitrator. I will work on this.

yeah, it'd be great if you could have a simple test now, and then add
more / make it go end-to-end as you flesh out the remaining bits.

>
>
>> a few comments and one design question below that I hope it'll simplify
>> the
>> implementation:
>
>> http://codereview.chromium.org/2858049/diff/1/2
>> File chrome/browser/device_orientation/arbitrator.h (right):
>
>> http://codereview.chromium.org/2858049/diff/1/2#newcode17
>> chrome/browser/device_orientation/arbitrator.h:17: class Arbitrator :
>> public
>> base::RefCounted<Arbitrator> {
>> does any of the platforms would implement multiple providers?
>> perhaps we could simplify and have just a single provider and avoid
>
> arbitrating?
>
>> in geolocation, we have GPS and Network provider with different degrees of
>> accuracy and power consumption, and the javascript API exposes a way to
>> choose
>> low / high accuracy.
>> would this be the case for orientation as well?
>
> The name 'arbitrator' may be poorly chosen. It does not do the same thing as
> in
> geolocation; there will never be more than one provider used at the same
> time.
> The arbitrator's job in this case is merely to choose one that works and
> poll it
> on background thread. Suggestions for better name is very welcome :)
> ProviderManager?

how about just an abstract "Provider" that would be implement by
platform / test?
perhaps a ProviderFactory so you can inject on test.
OrientationFetcher may be a good name for the concrete class that
polls the platform Provider.
or you could have a ProviderBase that does the polling, and let
platforms derive from there.
DeviceDataProvider seems to be relatively similar, perhaps we could
even refactor from there?


>
>
>> http://codereview.chromium.org/2858049/diff/1/2#newcode31
>> chrome/browser/device_orientation/arbitrator.h:31: static Arbitrator*
>> GetInstance() {
>> I think GetInstance() is normally associated with singleton pattern.
>> specifically in this case, the caller could just call the ctor?
>
> Yes, the real arbitrator implementation will be a singleton. Right now, I
> just
> call new in there so that it compiles.

I see, thanks.

>
>
>> http://codereview.chromium.org/2858049/diff/1/4
>> File chrome/browser/device_orientation/dispatcher_host.h (right):
>
>> http://codereview.chromium.org/2858049/diff/1/4#newcode21
>> chrome/browser/device_orientation/dispatcher_host.h:21: static
>> DispatcherHost*
>> New(int resource_message_filter_process_id);
>> as above, if the factory method is not doing anything fancy, it may be
>> simpler
>> to just let the caller use the regular ctor..
>
> As above, this is needed since the arbitrator implementation will be a
> singleton.

not sure I follow this bit though. shouldn't it be possible to just
"new DispatcherHost"?

>
>
>
> http://codereview.chromium.org/2858049/show
>

[andreip3000, 2010-07-06 18:15:02]

http://codereview.chromium.org/2858049/diff/1/2
File chrome/browser/device_orientation/arbitrator.h (right):

http://codereview.chromium.org/2858049/diff/1/2#newcode12
chrome/browser/device_orientation/arbitrator.h:12: namespace device_orientation
{
does chrome code use namespaces? I thought it doesn't normally use them.

http://codereview.chromium.org/2858049/diff/1/3
File chrome/browser/device_orientation/dispatcher_host.cc (right):

http://codereview.chromium.org/2858049/diff/1/3#newcode67
chrome/browser/device_orientation/dispatcher_host.cc:67: arbitrator_ =
Arbitrator::GetInstance();
Hmm, but if GetInstance() always returns a new Arbitrator, then they will always
have a single observer, right? I think the intent is to have a singleton
arbitrator?

http://codereview.chromium.org/2858049/diff/1/10
File chrome/renderer/device_orientation_dispatcher.cc (right):

http://codereview.chromium.org/2858049/diff/1/10#newcode50
chrome/renderer/device_orientation_dispatcher.cc:50: typedef
std::set<WebKit::WebDeviceOrientationServiceBridge*>::iterator It;
it

http://codereview.chromium.org/2858049/diff/1/11
File chrome/renderer/device_orientation_dispatcher.h (right):

http://codereview.chromium.org/2858049/diff/1/11#newcode32
chrome/renderer/device_orientation_dispatcher.h:32:
std::set<WebKit::WebDeviceOrientationServiceBridge*> bridges_;
Could this use the Client objects directly instead? Maybe I am missing something
but I am unsure what the bridge object is buying us...

[hans, 2010-07-07 09:20:01]

On 2010/07/06 17:48:50, bulach wrote:
> > The name 'arbitrator' may be poorly chosen. It does not do the same thing as
> > in
> > geolocation; there will never be more than one provider used at the same
> > time.
> > The arbitrator's job in this case is merely to choose one that works and
> > poll it
> > on background thread. Suggestions for better name is very welcome :)
> > ProviderManager?
> 
> how about just an abstract "Provider" that would be implement by
> platform / test?
> perhaps a ProviderFactory so you can inject on test.
> OrientationFetcher may be a good name for the concrete class that
> polls the platform Provider.
> or you could have a ProviderBase that does the polling, and let
> platforms derive from there.
> DeviceDataProvider seems to be relatively similar, perhaps we could
> even refactor from there?
My plan was to have a set of concrete classes for different platforms that
implement a Provider interface, are created by ProviderFactory objects, and
managed (created and polled) by the Arbitrator. I think this makes good sense
except for the name of the Arbitrator and that ProviderManager may be a better
name.

I don't want the polling to be done in a base class that the various providers
inherit from, as it doesn't feel like proper "is-a" inheritance.

> >
> >
> >> http://codereview.chromium.org/2858049/diff/1/4
> >> File chrome/browser/device_orientation/dispatcher_host.h (right):
> >
> >> http://codereview.chromium.org/2858049/diff/1/4#newcode21
> >> chrome/browser/device_orientation/dispatcher_host.h:21: static
> >> DispatcherHost*
> >> New(int resource_message_filter_process_id);
> >> as above, if the factory method is not doing anything fancy, it may be
> >> simpler
> >> to just let the caller use the regular ctor..
> >
> > As above, this is needed since the arbitrator implementation will be a
> > singleton.
> 
> not sure I follow this bit though. shouldn't it be possible to just
> "new DispatcherHost"?
Sorry, I completely misread your comment. Yes, DispatcherHost could very well be
created by new DispatcherHost(). The New method is there because I copied the
design from GeolocationDispatcherHost. But I guess there's really no point in
having it, so I'll remove it.

[hans, 2010-07-07 09:39:29]

On 2010/07/06 18:15:02, andreip3000 wrote:
> http://codereview.chromium.org/2858049/diff/1/2
> File chrome/browser/device_orientation/arbitrator.h (right):
> 
> http://codereview.chromium.org/2858049/diff/1/2#newcode12
> chrome/browser/device_orientation/arbitrator.h:12: namespace
device_orientation
> {
> does chrome code use namespaces? I thought it doesn't normally use them.
I don't know. Some code seems to use it (e.g. printing), but most doesn't. I
felt it would be useful to avoid repeating long identifiers inside the device
orientation code though. Perhaps Jeremy could enlighten us about the use of
namespaces in Chromium?

> http://codereview.chromium.org/2858049/diff/1/3
> File chrome/browser/device_orientation/dispatcher_host.cc (right):
> 
> http://codereview.chromium.org/2858049/diff/1/3#newcode67
> chrome/browser/device_orientation/dispatcher_host.cc:67: arbitrator_ =
> Arbitrator::GetInstance();
> Hmm, but if GetInstance() always returns a new Arbitrator, then they will
always
> have a single observer, right? I think the intent is to have a singleton
> arbitrator?
Yes, but the arbitrator implementation is coming in the next patch (see reply to
Marcus's comment). Perhaps I should make it a singleton here as well though, as
this keeps coming up.

> http://codereview.chromium.org/2858049/diff/1/10
> File chrome/renderer/device_orientation_dispatcher.cc (right):
> 
> http://codereview.chromium.org/2858049/diff/1/10#newcode50
> chrome/renderer/device_orientation_dispatcher.cc:50: typedef
> std::set<WebKit::WebDeviceOrientationServiceBridge*>::iterator It;
> it
Grepping through the code, I find typedefs like this named both starting with
capital letters and without. The use of capital letters seems most common
though, and it coincides with the Google C++ guidelines that say "Type names
start with a capital letter..." So I think it's right?

> http://codereview.chromium.org/2858049/diff/1/11
> File chrome/renderer/device_orientation_dispatcher.h (right):
> 
> http://codereview.chromium.org/2858049/diff/1/11#newcode32
> chrome/renderer/device_orientation_dispatcher.h:32:
> std::set<WebKit::WebDeviceOrientationServiceBridge*> bridges_;
> Could this use the Client objects directly instead? Maybe I am missing
something
> but I am unsure what the bridge object is buying us...
No, the DeviceOrientationClient class is not visible from Chromium, and as I
understand it can't be made visible as it relies on WebCore code.

[joth, 2010-07-07 11:19:51]

On 7 July 2010 10:20, <hans@chromium.org> wrote:

> On 2010/07/06 17:48:50, bulach wrote:
>
>> > The name 'arbitrator' may be poorly chosen. It does not do the same
>> thing as
>> > in
>> > geolocation; there will never be more than one provider used at the same
>> > time.
>> > The arbitrator's job in this case is merely to choose one that works and
>> > poll it
>> > on background thread. Suggestions for better name is very welcome :)
>> > ProviderManager?
>>
>
>  how about just an abstract "Provider" that would be implement by
>> platform / test?
>> perhaps a ProviderFactory so you can inject on test.
>> OrientationFetcher may be a good name for the concrete class that
>> polls the platform Provider.
>> or you could have a ProviderBase that does the polling, and let
>> platforms derive from there.
>> DeviceDataProvider seems to be relatively similar, perhaps we could
>> even refactor from there?
>>
> My plan was to have a set of concrete classes for different platforms that
> implement a Provider interface, are created by ProviderFactory objects, and
> managed (created and polled) by the Arbitrator. I think this makes good
> sense
> except for the name of the Arbitrator and that ProviderManager may be a
> better
> name.
>
>
I was steered away from LocationManager on the common argument that
'manager' classes are bad (end up being a dumping ground for everything).
(This doesn't change the fact that the location arbitrator has
responsibilities far beyond arbitration... :-/ )


> I don't want the polling to be done in a base class that the various
> providers
> inherit from, as it doesn't feel like proper "is-a" inheritance.
>
> In practice this can be a lot lot less of a problem if the polling
'convenience' functionality is put into an optional intermediate base
class. Agree it's not good if it's part of the base interface.


>  >
>> >
>> >> http://codereview.chromium.org/2858049/diff/1/4
>> >> File chrome/browser/device_orientation/dispatcher_host.h (right):
>> >
>> >> http://codereview.chromium.org/2858049/diff/1/4#newcode21
>> >> chrome/browser/device_orientation/dispatcher_host.h:21: static
>> >> DispatcherHost*
>> >> New(int resource_message_filter_process_id);
>> >> as above, if the factory method is not doing anything fancy, it may be
>> >> simpler
>> >> to just let the caller use the regular ctor..
>> >
>> > As above, this is needed since the arbitrator implementation will be a
>> > singleton.
>>
>
>  not sure I follow this bit though. shouldn't it be possible to just
>> "new DispatcherHost"?
>>
> Sorry, I completely misread your comment. Yes, DispatcherHost could very
> well be
> created by new DispatcherHost(). The New method is there because I copied
> the
> design from GeolocationDispatcherHost. But I guess there's really no point
> in
> having it, so I'll remove it.
>
> http://codereview.chromium.org/2858049/show
>


> <http://codereview.chromium.org/2858049/diff/1/10#newcode50>

 http://codereview.chromium.org/2858049/diff/1/10#newcode50

 chrome/renderer/device_orientation_dispatcher.cc:50: typedef

 std::set<WebKit::WebDeviceOrientationServiceBridge*>::iterator It;

 it

Grepping through the code, I find typedefs like this named both starting
with

capital letters and without. The use of capital letters seems most common

though, and it coincides with the Google C++ guidelines that say "Type names

start with a capital letter..." So I think it's right?



I think the more common pattern is to typedef the container, and then you
can use WebDeviceOrienteationServiceBridgePointerSet::iterator i = ...
In this case, that's still rather a mouthful though.. :-/

[hans, 2010-07-22 16:25:58]

Sorry for the long wait; here is the next revision of this patch. Thanks for all
your comments so far.

I hope I have managed to fix everything we've mentioned so far, but since it's
been some time I may have missed something. Please yell at me if that's the
case.

The WebKit side is still in https://bugs.webkit.org/show_bug.cgi?id=41616.

[bulach, 2010-07-26 18:41:57]

LGTM

nice stuff! a few nits and suggestions below, and overall looks fine! 

sorry about the delay..

http://codereview.chromium.org/2858049/diff/14001/15001
File chrome/browser/device_orientation/device_orientation_browsertest.cc
(right):

http://codereview.chromium.org/2858049/diff/14001/15001#newcode18
chrome/browser/device_orientation/device_orientation_browsertest.cc:18: public:
need to initialize the members:

MockProvider() : added_observer_(false), removed_observer_(false);

http://codereview.chromium.org/2858049/diff/14001/15001#newcode21
chrome/browser/device_orientation/device_orientation_browsertest.cc:21:
observer->OnOrientationUpdate(true, 1, true, 2, true, 3);
see below: perhaps use a struct, then define a const member here that you can
use to reference in the test comment below.

http://codereview.chromium.org/2858049/diff/14001/15001#newcode27
chrome/browser/device_orientation/device_orientation_browsertest.cc:27: bool
removed_observer;
"_" suffix on both.

http://codereview.chromium.org/2858049/diff/14001/15001#newcode48
chrome/browser/device_orientation/device_orientation_browsertest.cc:48: //
expects to get an event with the magic orientation (1, 2, 3),
as above, instead of "magic" :) perhaps "expects to get an event with
kMockOrientationValue" or something...

(the 'magic' bit got me a little distracted trying to figure out where it was
being done.. ;)

http://codereview.chromium.org/2858049/diff/14001/15003
File chrome/browser/device_orientation/dispatcher_host.h (right):

http://codereview.chromium.org/2858049/diff/14001/15003#newcode37
chrome/browser/device_orientation/dispatcher_host.h:37: friend class
base::RefCountedThreadSafe<DispatcherHost>;
I think the friend decl goes right before the dtor above

http://codereview.chromium.org/2858049/diff/14001/15004
File chrome/browser/device_orientation/provider.cc (right):

http://codereview.chromium.org/2858049/diff/14001/15004#newcode9
chrome/browser/device_orientation/provider.cc:9: Provider* Provider::instance_;
 = NULL;

http://codereview.chromium.org/2858049/diff/14001/15005
File chrome/browser/device_orientation/provider.h (right):

http://codereview.chromium.org/2858049/diff/14001/15005#newcode22
chrome/browser/device_orientation/provider.h:22: bool can_provider_gamma, double
gamma) = 0;
perhaps define a struct instead of individual params?

http://codereview.chromium.org/2858049/diff/14001/15005#newcode34
chrome/browser/device_orientation/provider.h:34: static void
SetInstance(Provider* provider) {
perhaps SetMockInstance, or SetInstanceForTests?

http://codereview.chromium.org/2858049/diff/14001/15005#newcode44
chrome/browser/device_orientation/provider.h:44: virtual ~Provider() { instance_
= NULL; }
DCHECK(instance_ == this);

http://codereview.chromium.org/2858049/diff/14001/15005#newcode48
chrome/browser/device_orientation/provider.h:48: static Provider* instance_;
\n

http://codereview.chromium.org/2858049/diff/14001/15013
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/2858049/diff/14001/15013#newcode312
chrome/renderer/render_view.h:312: // Device Orientation
--------------------------------------------------------
\n

http://codereview.chromium.org/2858049/diff/14001/15014
File chrome/test/data/device_orientation/device_orientation_test.html (right):

http://codereview.chromium.org/2858049/diff/14001/15014#newcode7
chrome/test/data/device_orientation/device_orientation_test.html:7: function
eventOk(event) {
perhaps: checkOrientationEvent

[hans, 2010-07-27 10:56:59]

Thanks for the review, Marcus.

Will land this when the WebKit part is in.

In the meantime, comments on the device_orientation::Orientation struct are
welcome, as that has been added since the review.

http://codereview.chromium.org/2858049/diff/14001/15001
File chrome/browser/device_orientation/device_orientation_browsertest.cc
(right):

http://codereview.chromium.org/2858049/diff/14001/15001#newcode18
chrome/browser/device_orientation/device_orientation_browsertest.cc:18: public:
On 2010/07/26 18:41:57, bulach wrote:
> need to initialize the members:
> 
> MockProvider() : added_observer_(false), removed_observer_(false);

Done. I was relying on the implicit default constructor to do the
initialization, but looking at the Style Guide, I see that's discouraged.

http://codereview.chromium.org/2858049/diff/14001/15001#newcode21
chrome/browser/device_orientation/device_orientation_browsertest.cc:21:
observer->OnOrientationUpdate(true, 1, true, 2, true, 3);
On 2010/07/26 18:41:57, bulach wrote:
> see below: perhaps use a struct, then define a const member here that you can
> use to reference in the test comment below.

Done. Introducing struct device_orientation::Orientation to cut down on the
number of parameters to pass around.

http://codereview.chromium.org/2858049/diff/14001/15001#newcode27
chrome/browser/device_orientation/device_orientation_browsertest.cc:27: bool
removed_observer;
On 2010/07/26 18:41:57, bulach wrote:
> "_" suffix on both.

Done.

http://codereview.chromium.org/2858049/diff/14001/15001#newcode48
chrome/browser/device_orientation/device_orientation_browsertest.cc:48: //
expects to get an event with the magic orientation (1, 2, 3),
On 2010/07/26 18:41:57, bulach wrote:
> as above, instead of "magic" :) perhaps "expects to get an event with
> kMockOrientationValue" or something...
> 
> (the 'magic' bit got me a little distracted trying to figure out where it was
> being done.. ;)

Done.

http://codereview.chromium.org/2858049/diff/14001/15003
File chrome/browser/device_orientation/dispatcher_host.h (right):

http://codereview.chromium.org/2858049/diff/14001/15003#newcode37
chrome/browser/device_orientation/dispatcher_host.h:37: friend class
base::RefCountedThreadSafe<DispatcherHost>;
On 2010/07/26 18:41:57, bulach wrote:
> I think the friend decl goes right before the dtor above

Done.

http://codereview.chromium.org/2858049/diff/14001/15004
File chrome/browser/device_orientation/provider.cc (right):

http://codereview.chromium.org/2858049/diff/14001/15004#newcode9
chrome/browser/device_orientation/provider.cc:9: Provider* Provider::instance_;
On 2010/07/26 18:41:57, bulach wrote:
>  = NULL;

Done.

http://codereview.chromium.org/2858049/diff/14001/15005
File chrome/browser/device_orientation/provider.h (right):

http://codereview.chromium.org/2858049/diff/14001/15005#newcode22
chrome/browser/device_orientation/provider.h:22: bool can_provider_gamma, double
gamma) = 0;
On 2010/07/26 18:41:57, bulach wrote:
> perhaps define a struct instead of individual params?
Done. Defined struct device_orientation::Orientation.

http://codereview.chromium.org/2858049/diff/14001/15005#newcode34
chrome/browser/device_orientation/provider.h:34: static void
SetInstance(Provider* provider) {
On 2010/07/26 18:41:57, bulach wrote:
> perhaps SetMockInstance, or SetInstanceForTests?

Done.

http://codereview.chromium.org/2858049/diff/14001/15005#newcode44
chrome/browser/device_orientation/provider.h:44: virtual ~Provider() { instance_
= NULL; }
On 2010/07/26 18:41:57, bulach wrote:
> DCHECK(instance_ == this);

Done.

http://codereview.chromium.org/2858049/diff/14001/15005#newcode48
chrome/browser/device_orientation/provider.h:48: static Provider* instance_;
On 2010/07/26 18:41:57, bulach wrote:
> \n

Done.

http://codereview.chromium.org/2858049/diff/14001/15013
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/2858049/diff/14001/15013#newcode312
chrome/renderer/render_view.h:312: // Device Orientation
--------------------------------------------------------
On 2010/07/26 18:41:57, bulach wrote:
> \n

Done.

http://codereview.chromium.org/2858049/diff/14001/15014
File chrome/test/data/device_orientation/device_orientation_test.html (right):

http://codereview.chromium.org/2858049/diff/14001/15014#newcode7
chrome/test/data/device_orientation/device_orientation_test.html:7: function
eventOk(event) {
On 2010/07/26 18:41:57, bulach wrote:
> perhaps: checkOrientationEvent

Done.

[andreip3000, 2010-07-27 11:55:31]

Looks good, a couple of questions.

http://codereview.chromium.org/2858049/diff/25001/26004
File chrome/browser/device_orientation/orientation.h (right):

http://codereview.chromium.org/2858049/diff/25001/26004#newcode21
chrome/browser/device_orientation/orientation.h:21: Orientation() {}
Should this not initialize the members to false / 0?

http://codereview.chromium.org/2858049/diff/25001/26004#newcode25
chrome/browser/device_orientation/orientation.h:25: }
Is this needed if you have the default ctor initialize the members to 0 / false?
I also can't find any call sites for this method.

http://codereview.chromium.org/2858049/diff/25001/26004#newcode27
chrome/browser/device_orientation/orientation.h:27: bool isEmpty() {
Is this called anywhere? I can't find any call site.

[jorlow, 2010-07-27 13:00:54]

Some comments from a while ago that I forgot to publish.

http://codereview.chromium.org/2858049/diff/1/4
File chrome/browser/device_orientation/dispatcher_host.h (right):

http://codereview.chromium.org/2858049/diff/1/4#newcode21
chrome/browser/device_orientation/dispatcher_host.h:21: static DispatcherHost*
New(int resource_message_filter_process_id);
On 2010/07/06 17:02:00, bulach wrote:
> as above, if the factory method is not doing anything fancy, it may be simpler
> to just let the caller use the regular ctor..

Yeah..normally we don't use factories in chromium

http://codereview.chromium.org/2858049/diff/1/5
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/2858049/diff/1/5#newcode349
chrome/browser/renderer_host/resource_message_filter.cc:349: this->id()))) {
could go on the prev line

http://codereview.chromium.org/2858049/diff/1/10
File chrome/renderer/device_orientation_dispatcher.cc (right):

http://codereview.chromium.org/2858049/diff/1/10#newcode16
chrome/renderer/device_orientation_dispatcher.cc:16: if (!bridges_.empty())
{}'s if the if clause is more than one line

http://codereview.chromium.org/2858049/diff/1/10#newcode34
chrome/renderer/device_orientation_dispatcher.cc:34: if (bridges_.size() == 1)
ditto

http://codereview.chromium.org/2858049/diff/1/10#newcode42
chrome/renderer/device_orientation_dispatcher.cc:42: if (bridges_.empty())
ditto

[jorlow, 2010-07-27 13:22:32]

and more comments

http://codereview.chromium.org/2858049/diff/25001/26002
File chrome/browser/device_orientation/dispatcher_host.cc (right):

http://codereview.chromium.org/2858049/diff/25001/26002#newcode32
chrome/browser/device_orientation/dispatcher_host.cc:32: if
(!command_line.HasSwitch(switches::kEnableDeviceOrientation))
I guess I don't see the point of changing it to a CommandLine& rather than just
keeping it a *...but it doesn't really matter.

http://codereview.chromium.org/2858049/diff/25001/26002#newcode36
chrome/browser/device_orientation/dispatcher_host.cc:36: *msg_was_ok = true;
Not sure, but I think this might be a bug (it could have been set intentionally
but had handled be false...in which case this would reset it).  At best it's not
needed.

http://codereview.chromium.org/2858049/diff/25001/26002#newcode58
chrome/browser/device_orientation/dispatcher_host.cc:58: for (Iterator i =
render_view_ids_.begin(), e = render_view_ids_.end();
We normally don't bother putting the end in a variable.  I doubt it affects perf
and the compiler hopefully can handle this for us.

http://codereview.chromium.org/2858049/diff/25001/26002#newcode60
chrome/browser/device_orientation/dispatcher_host.cc:60: IPC::Message* message =
new ViewMsg_DeviceOrientation_OrientationUpdated(
Maybe put everything after the = on the same line?

http://codereview.chromium.org/2858049/diff/25001/26002#newcode69
chrome/browser/device_orientation/dispatcher_host.cc:69: if
(render_view_ids_.find(render_view_id) != render_view_ids_.end()) {
Maybe just do a DCHECK?  What happens if you insert again?  Nothing, right?

http://codereview.chromium.org/2858049/diff/25001/26002#newcode77
chrome/browser/device_orientation/dispatcher_host.cc:77:
provider_->AddObserver(this);
Is it easy/efficient to ask the provider if we're already added?  If so, I'd
prefer that.  Maintaining state in 2 places should be avoided when possible.

http://codereview.chromium.org/2858049/diff/25001/26002#newcode83
chrome/browser/device_orientation/dispatcher_host.cc:83: if
(render_view_ids_.find(render_view_id) == render_view_ids_.end()) {
Same thing.

http://codereview.chromium.org/2858049/diff/25001/26002#newcode90
chrome/browser/device_orientation/dispatcher_host.cc:90: provider_ = NULL;
Is there any reason to store this in a member variable?

http://codereview.chromium.org/2858049/diff/25001/26004
File chrome/browser/device_orientation/orientation.h (right):

http://codereview.chromium.org/2858049/diff/25001/26004#newcode19
chrome/browser/device_orientation/orientation.h:19:
can_provide_gamma_(can_provide_gamma) {}
} on next line

http://codereview.chromium.org/2858049/diff/25001/26006
File chrome/browser/device_orientation/provider.h (right):

http://codereview.chromium.org/2858049/diff/25001/26006#newcode27
chrome/browser/device_orientation/provider.h:27: static Provider* GetInstance()
{
Hmmm....singletons are generally bad for testing reasons.  Looking at this code,
it seems that you're assuming that it'll _always_ be mocked out during tests?  I
guess I can probably buy that assumption.  But what about destruction?  It'll
just be leaked?

Lets try to find an owner for this rather than doing it this way.

http://codereview.chromium.org/2858049/diff/25001/26006#newcode28
chrome/browser/device_orientation/provider.h:28: if (instance_ == NULL)
if (!instance_)

http://codereview.chromium.org/2858049/diff/25001/26006#newcode35
chrome/browser/device_orientation/provider.h:35: DCHECK(NULL == instance_);
!

http://codereview.chromium.org/2858049/diff/25001/26006#newcode44
chrome/browser/device_orientation/provider.h:44: virtual ~Provider() {
Who would ever call this?

http://codereview.chromium.org/2858049/diff/25001/26010
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/2858049/diff/25001/26010#newcode907
chrome/chrome_tests.gypi:907: 'browser/dock_info_unittest.cc',
tab problem.

http://codereview.chromium.org/2858049/diff/25001/26010#newcode1523
chrome/chrome_tests.gypi:1523:
'browser/device_orientation/device_orientation_browsertest.cc',
alpha order

http://codereview.chromium.org/2858049/diff/25001/26011
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/2858049/diff/25001/26011#newcode826
chrome/common/render_messages.h:826: bool can_provide_alpha;
All variables should be commented.

And put above the enum.

http://codereview.chromium.org/2858049/diff/25001/26013
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/2858049/diff/25001/26013#newcode5225
chrome/renderer/render_view.cc:5225: if (webview()) {
Can webView ever not be set?  My guess is that in this case, the answer is no. 
Please research.

http://codereview.chromium.org/2858049/diff/25001/26014
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/2858049/diff/25001/26014#newcode311
chrome/renderer/render_view.h:311: const
ViewMsg_DeviceOrientation_OrientationUpdated_Params& p);
ViewMsg_DeviceOrientationUpdated_Params

[hans, 2010-07-27 16:23:36]

Thanks for the input!

http://codereview.chromium.org/2858049/diff/25001/26002
File chrome/browser/device_orientation/dispatcher_host.cc (right):

http://codereview.chromium.org/2858049/diff/25001/26002#newcode32
chrome/browser/device_orientation/dispatcher_host.cc:32: if
(!command_line.HasSwitch(switches::kEnableDeviceOrientation))
On 2010/07/27 13:22:32, Jeremy Orlow wrote:
> I guess I don't see the point of changing it to a CommandLine& rather than
just
> keeping it a *...but it doesn't really matter.

Done.

http://codereview.chromium.org/2858049/diff/25001/26002#newcode36
chrome/browser/device_orientation/dispatcher_host.cc:36: *msg_was_ok = true;
On 2010/07/27 13:22:32, Jeremy Orlow wrote:
> Not sure, but I think this might be a bug (it could have been set
intentionally
> but had handled be false...in which case this would reset it).  At best it's
not
> needed.

Hmm, this occurs in several places in the code, e.g. in
geolocation_dispatcher_host.cc (from where I copied it),
database_dispatcher_host.cc, resource_dispatcher_host.cc,
audio_renderer_host.cc, etc. But there are several that don't have it, e.g. in
dom_storage_dispatcher_host.cc.

I am not familiar with this code, but as I understand, msg_was_ok should only be
false after a handler has tried and failed to deserialize the message, in which
case handled should also be true and our OnMessageReceived function should not
be called.

If that's the case, then it should always be true on entry to OnMessageReceived,
and the assignment is redundant. I'll remove it.

http://codereview.chromium.org/2858049/diff/25001/26002#newcode58
chrome/browser/device_orientation/dispatcher_host.cc:58: for (Iterator i =
render_view_ids_.begin(), e = render_view_ids_.end();
On 2010/07/27 13:22:32, Jeremy Orlow wrote:
> We normally don't bother putting the end in a variable.  I doubt it affects
perf
> and the compiler hopefully can handle this for us.

You're probably right that it doesn't affect performance in this case, but I do
tend to write it like this by default (for the reasons here:
http://llvm.org/docs/CodingStandards.html#ll_end). If you think this conflicts
with Chromium style, I'm happy to change it.

(Changing the iterator to const_iterator, though.)

http://codereview.chromium.org/2858049/diff/25001/26002#newcode60
chrome/browser/device_orientation/dispatcher_host.cc:60: IPC::Message* message =
new ViewMsg_DeviceOrientation_OrientationUpdated(
On 2010/07/27 13:22:32, Jeremy Orlow wrote:
> Maybe put everything after the = on the same line?

Done.

http://codereview.chromium.org/2858049/diff/25001/26002#newcode69
chrome/browser/device_orientation/dispatcher_host.cc:69: if
(render_view_ids_.find(render_view_id) != render_view_ids_.end()) {
On 2010/07/27 13:22:32, Jeremy Orlow wrote:
> Maybe just do a DCHECK?  What happens if you insert again?  Nothing, right?

Done.

http://codereview.chromium.org/2858049/diff/25001/26002#newcode77
chrome/browser/device_orientation/dispatcher_host.cc:77:
provider_->AddObserver(this);
On 2010/07/27 13:22:32, Jeremy Orlow wrote:
> Is it easy/efficient to ask the provider if we're already added?  If so, I'd
> prefer that.  Maintaining state in 2 places should be avoided when possible.

Well, we could add a member function in Provider to ask if we're already
observing, but I'm not convinced it would buy us anything.

We would still need to look at render_view_ids_.size() (I assume that's the
state you refer to?) in OnStopUpdating to see if the render_view_id is the last
to be removed. It feels natural to me to do the same thing in OnStartUpdating:
if it's the first id to be added, start observing.

Please correct me if I'm misunderstanding.

http://codereview.chromium.org/2858049/diff/25001/26002#newcode83
chrome/browser/device_orientation/dispatcher_host.cc:83: if
(render_view_ids_.find(render_view_id) == render_view_ids_.end()) {
On 2010/07/27 13:22:32, Jeremy Orlow wrote:
> Same thing.

Done.

http://codereview.chromium.org/2858049/diff/25001/26002#newcode90
chrome/browser/device_orientation/dispatcher_host.cc:90: provider_ = NULL;
On 2010/07/27 13:22:32, Jeremy Orlow wrote:
> Is there any reason to store this in a member variable?

Yes. Provider is reference counted, so we need to hold a reference to it
(provider_ is a scoped_refptr) to keep it around.

http://codereview.chromium.org/2858049/diff/25001/26004
File chrome/browser/device_orientation/orientation.h (right):

http://codereview.chromium.org/2858049/diff/25001/26004#newcode19
chrome/browser/device_orientation/orientation.h:19:
can_provide_gamma_(can_provide_gamma) {}
On 2010/07/27 13:22:32, Jeremy Orlow wrote:
> } on next line

Done.

http://codereview.chromium.org/2858049/diff/25001/26004#newcode21
chrome/browser/device_orientation/orientation.h:21: Orientation() {}
On 2010/07/27 11:55:31, andreip3000 wrote:
> Should this not initialize the members to false / 0?

Done.

http://codereview.chromium.org/2858049/diff/25001/26004#newcode25
chrome/browser/device_orientation/orientation.h:25: }
On 2010/07/27 11:55:31, andreip3000 wrote:
> Is this needed if you have the default ctor initialize the members to 0 /
false?
> I also can't find any call sites for this method.

It's not called because it is used by the Provider implementation, which isn't
part of this patch, so I'm removing it.

I liked having a special method to get this special "Empty" (should probably be
called something else) orientation, that is used for signalling that no
orientation data can be provided, so I might put it back when implementing
Provider.

http://codereview.chromium.org/2858049/diff/25001/26004#newcode27
chrome/browser/device_orientation/orientation.h:27: bool isEmpty() {
On 2010/07/27 11:55:31, andreip3000 wrote:
> Is this called anywhere? I can't find any call site.

Removing. It's used by the Provider implementation from which I cherry-picked
this class.

http://codereview.chromium.org/2858049/diff/25001/26006
File chrome/browser/device_orientation/provider.h (right):

http://codereview.chromium.org/2858049/diff/25001/26006#newcode27
chrome/browser/device_orientation/provider.h:27: static Provider* GetInstance()
{
On 2010/07/27 13:22:32, Jeremy Orlow wrote:
> Hmmm....singletons are generally bad for testing reasons.  Looking at this
code,
> it seems that you're assuming that it'll _always_ be mocked out during tests? 
I
> guess I can probably buy that assumption.  But what about destruction?  It'll
> just be leaked?
> 
> Lets try to find an owner for this rather than doing it this way.

I made it a singleton to make sure there would only be a single instance of the
object that talks to the underlying resources that provide orientation data. I'm
not sure that's necessary, but it felt kind of natural.

Since the class is reference counted, it is destructed when the last reference
to it is dropped, so it will not be leaked.

http://codereview.chromium.org/2858049/diff/25001/26006#newcode28
chrome/browser/device_orientation/provider.h:28: if (instance_ == NULL)
On 2010/07/27 13:22:32, Jeremy Orlow wrote:
> if (!instance_)

Done.

http://codereview.chromium.org/2858049/diff/25001/26006#newcode35
chrome/browser/device_orientation/provider.h:35: DCHECK(NULL == instance_);
On 2010/07/27 13:22:32, Jeremy Orlow wrote:
> !

Done. (Assuming you mean "make it DCHECK(!instance_)").

http://codereview.chromium.org/2858049/diff/25001/26006#newcode44
chrome/browser/device_orientation/provider.h:44: virtual ~Provider() {
On 2010/07/27 13:22:32, Jeremy Orlow wrote:
> Who would ever call this?

base::RefCountedThreadSafe<Provider>::Release().

http://codereview.chromium.org/2858049/diff/25001/26010
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/2858049/diff/25001/26010#newcode907
chrome/chrome_tests.gypi:907: 'browser/dock_info_unittest.cc',
On 2010/07/27 13:22:32, Jeremy Orlow wrote:
> tab problem.

Done.
Also removing arbitrator_unittest.cc which should not be there.

http://codereview.chromium.org/2858049/diff/25001/26010#newcode1523
chrome/chrome_tests.gypi:1523:
'browser/device_orientation/device_orientation_browsertest.cc',
On 2010/07/27 13:22:32, Jeremy Orlow wrote:
> alpha order

Done.

http://codereview.chromium.org/2858049/diff/25001/26011
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/2858049/diff/25001/26011#newcode826
chrome/common/render_messages.h:826: bool can_provide_alpha;
On 2010/07/27 13:22:32, Jeremy Orlow wrote:
> All variables should be commented.
> 
> And put above the enum.

Done.

http://codereview.chromium.org/2858049/diff/25001/26013
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/2858049/diff/25001/26013#newcode5225
chrome/renderer/render_view.cc:5225: if (webview()) {
On 2010/07/27 13:22:32, Jeremy Orlow wrote:
> Can webView ever not be set?  My guess is that in this case, the answer is no.

> Please research.

Yes, the comment in render_view.h says "May return NULL when the view is
closing.".

http://codereview.chromium.org/2858049/diff/25001/26014
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/2858049/diff/25001/26014#newcode311
chrome/renderer/render_view.h:311: const
ViewMsg_DeviceOrientation_OrientationUpdated_Params& p);
On 2010/07/27 13:22:32, Jeremy Orlow wrote:
> ViewMsg_DeviceOrientationUpdated_Params

Done.
Renamed ViewMsg_DeviceOrientation_OrientationUpdated to
ViewMsg_DeviceOrientationUpdated to match.

[jorlow, 2010-07-27 16:29:57]

http://codereview.chromium.org/2858049/diff/25001/26002
File chrome/browser/device_orientation/dispatcher_host.cc (right):

http://codereview.chromium.org/2858049/diff/25001/26002#newcode58
chrome/browser/device_orientation/dispatcher_host.cc:58: for (Iterator i =
render_view_ids_.begin(), e = render_view_ids_.end();
On 2010/07/27 16:23:37, hans wrote:
> On 2010/07/27 13:22:32, Jeremy Orlow wrote:
> > We normally don't bother putting the end in a variable.  I doubt it affects
> perf
> > and the compiler hopefully can handle this for us.
> 
> You're probably right that it doesn't affect performance in this case, but I
do
> tend to write it like this by default (for the reasons here:
> http://llvm.org/docs/CodingStandards.html#ll_end). If you think this conflicts
> with Chromium style, I'm happy to change it.
> 
> (Changing the iterator to const_iterator, though.)

I wouldn't say it conflicts, but it's slightly less clear.  Your choice.

http://codereview.chromium.org/2858049/diff/25001/26006
File chrome/browser/device_orientation/provider.h (right):

http://codereview.chromium.org/2858049/diff/25001/26006#newcode27
chrome/browser/device_orientation/provider.h:27: static Provider* GetInstance()
{
On 2010/07/27 16:23:37, hans wrote:
> On 2010/07/27 13:22:32, Jeremy Orlow wrote:
> > Hmmm....singletons are generally bad for testing reasons.  Looking at this
> code,
> > it seems that you're assuming that it'll _always_ be mocked out during
tests? 
> I
> > guess I can probably buy that assumption.  But what about destruction? 
It'll
> > just be leaked?
> > 
> > Lets try to find an owner for this rather than doing it this way.
> 
> I made it a singleton to make sure there would only be a single instance of
the
> object that talks to the underlying resources that provide orientation data.
I'm
> not sure that's necessary, but it felt kind of natural.
> 
> Since the class is reference counted, it is destructed when the last reference
> to it is dropped, so it will not be leaked.

Oh, I see.  So instance_ is essentially just a weak pointer to it.  Ok...maybe
add a comment about all of this though....maybe in the header file above
GetInstance's declaration?

[hans, 2010-07-27 17:14:43]

On 2010/07/27 16:29:57, Jeremy Orlow wrote:
> Oh, I see.  So instance_ is essentially just a weak pointer to it.  Ok...maybe
> add a comment about all of this though....maybe in the header file above
> GetInstance's declaration?

Uploaded patch with hopefully better comments.

[jorlow, 2010-07-28 11:14:22]

I think that addresses my concerns.  Marcus/Andrei?

[andreip3000, 2010-07-28 11:23:04]

LGTM

[bulach, 2010-07-28 11:35:32]

LGTM

ultra minor nits:

http://codereview.chromium.org/2858049/diff/38001/14004
File chrome/browser/device_orientation/dispatcher_host.cc (right):

http://codereview.chromium.org/2858049/diff/38001/14004#newcode47
chrome/browser/device_orientation/dispatcher_host.cc:47: void
DispatcherHost::OnOrientationUpdate(const Orientation& o) {
nit: s/o/orientation/

http://codereview.chromium.org/2858049/diff/38001/14008
File chrome/browser/device_orientation/provider.h (right):

http://codereview.chromium.org/2858049/diff/38001/14008#newcode22
chrome/browser/device_orientation/provider.h:22: virtual void
OnOrientationUpdate(const Orientation&) = 0;
const Orientation& orientation

[hans, 2010-07-28 13:08:41]

Thanks for the comments; ultra nits fixed.

http://codereview.chromium.org/2858049/diff/38001/14004
File chrome/browser/device_orientation/dispatcher_host.cc (right):

http://codereview.chromium.org/2858049/diff/38001/14004#newcode47
chrome/browser/device_orientation/dispatcher_host.cc:47: void
DispatcherHost::OnOrientationUpdate(const Orientation& o) {
On 2010/07/28 11:35:32, bulach wrote:
> nit: s/o/orientation/

Done.

http://codereview.chromium.org/2858049/diff/38001/14008
File chrome/browser/device_orientation/provider.h (right):

http://codereview.chromium.org/2858049/diff/38001/14008#newcode22
chrome/browser/device_orientation/provider.h:22: virtual void
OnOrientationUpdate(const Orientation&) = 0;
On 2010/07/28 11:35:32, bulach wrote:
> const Orientation& orientation

Done.

[jorlow, 2010-08-03 13:54:19]

http://codereview.chromium.org/2858049/diff/41001/42002
File chrome/browser/device_orientation/dispatcher_host.cc (right):

http://codereview.chromium.org/2858049/diff/41001/42002#newcode26
chrome/browser/device_orientation/dispatcher_host.cc:26:
provider_->RemoveObserver(this);
maybe stupid question: would it be easy to make this happen just on destruction

http://codereview.chromium.org/2858049/diff/41001/42002#newcode32
chrome/browser/device_orientation/dispatcher_host.cc:32: if
(!command_line->HasSwitch(switches::kEnableDeviceOrientation))
You don't want to be doing stuff like this on every single message.  It'll
likely cause a noticeable perf drop.

If you really want to, add a DCHECK in the body of these handlers.  But I don't
think it's necessary.

http://codereview.chromium.org/2858049/diff/41001/42002#newcode67
chrome/browser/device_orientation/dispatcher_host.cc:67:
DCHECK(render_view_ids_.find(render_view_id) == render_view_ids_.end());
What happens if there is a duplicate?

http://codereview.chromium.org/2858049/diff/41001/42004
File chrome/browser/device_orientation/orientation.h (right):

http://codereview.chromium.org/2858049/diff/41001/42004#newcode34
chrome/browser/device_orientation/orientation.h:34: bool can_provide_alpha_ : 1;
We don't do this anywhere else and I'm not really convinced it's worth the
optimization.  If you really want to, maybe find out why we don't other places
to be sure it's OK.

[hans, 2010-08-10 13:12:53]

Uploaded new version of the patch to match the WebKit plumbing (landed in
http://trac.webkit.org/changeset/65063).

The difference is that communication is no longer made by methods of RenderView,
but by a new class: DeviceOrientationDispatcher.

http://codereview.chromium.org/2858049/diff/41001/42002
File chrome/browser/device_orientation/dispatcher_host.cc (right):

http://codereview.chromium.org/2858049/diff/41001/42002#newcode26
chrome/browser/device_orientation/dispatcher_host.cc:26:
provider_->RemoveObserver(this);
On 2010/08/03 13:54:19, Jeremy Orlow wrote:
> maybe stupid question: would it be easy to make this happen just on
destruction

Not sure I'm following, but we really want to stop observing the Provider if all
render views unsubscribe from updates, as the Provider may then stop its polling
thread.

http://codereview.chromium.org/2858049/diff/41001/42002#newcode32
chrome/browser/device_orientation/dispatcher_host.cc:32: if
(!command_line->HasSwitch(switches::kEnableDeviceOrientation))
On 2010/08/03 13:54:19, Jeremy Orlow wrote:
> You don't want to be doing stuff like this on every single message.  It'll
> likely cause a noticeable perf drop.
> 
> If you really want to, add a DCHECK in the body of these handlers.  But I
don't
> think it's necessary.

Done.

http://codereview.chromium.org/2858049/diff/41001/42002#newcode67
chrome/browser/device_orientation/dispatcher_host.cc:67:
DCHECK(render_view_ids_.find(render_view_id) == render_view_ids_.end());
On 2010/08/03 13:54:19, Jeremy Orlow wrote:
> What happens if there is a duplicate?

Nothing really. Removing.

http://codereview.chromium.org/2858049/diff/41001/42004
File chrome/browser/device_orientation/orientation.h (right):

http://codereview.chromium.org/2858049/diff/41001/42004#newcode34
chrome/browser/device_orientation/orientation.h:34: bool can_provide_alpha_ : 1;
On 2010/08/03 13:54:19, Jeremy Orlow wrote:
> We don't do this anywhere else and I'm not really convinced it's worth the
> optimization.  If you really want to, maybe find out why we don't other places
> to be sure it's OK.
Removed.

[jorlow, 2010-08-10 13:39:15]

Did one last skim through.  LGTM

http://codereview.chromium.org/2858049/diff/41001/42002
File chrome/browser/device_orientation/dispatcher_host.cc (right):

http://codereview.chromium.org/2858049/diff/41001/42002#newcode67
chrome/browser/device_orientation/dispatcher_host.cc:67:
DCHECK(render_view_ids_.find(render_view_id) == render_view_ids_.end());
On 2010/08/10 13:12:53, hans wrote:
> On 2010/08/03 13:54:19, Jeremy Orlow wrote:
> > What happens if there is a duplicate?
> 
> Nothing really. Removing.

Even if the answer is nothing, it's fine to leave in.  I just wanted to make
sure nothing bad happened.

http://codereview.chromium.org/2858049/diff/50001/51003
File chrome/browser/device_orientation/dispatcher_host.h (right):

http://codereview.chromium.org/2858049/diff/50001/51003#newcode22
chrome/browser/device_orientation/dispatcher_host.h:22: explicit
DispatcherHost(int resource_message_filter_process_id);
I don't see why you need resource_message_filter_ in this name.

http://codereview.chromium.org/2858049/diff/50001/51003#newcode26
chrome/browser/device_orientation/dispatcher_host.h:26: virtual void
OnOrientationUpdate(const Orientation& o);
o is a bad variable name

http://codereview.chromium.org/2858049/diff/50001/51015
File chrome/renderer/device_orientation_dispatcher.h (right):

http://codereview.chromium.org/2858049/diff/50001/51015#newcode8
chrome/renderer/device_orientation_dispatcher.h:8: #include
"third_party/WebKit/WebKit/chromium/public/WebDeviceOrientationClient.h"
It's OK this is >80 cols.

[joth, 2010-08-10 14:25:05]

LG

late but minor comments...!

http://codereview.chromium.org/2858049/diff/1/10
File chrome/renderer/device_orientation_dispatcher.cc (right):

http://codereview.chromium.org/2858049/diff/1/10#newcode50
chrome/renderer/device_orientation_dispatcher.cc:50: typedef
std::set<WebKit::WebDeviceOrientationServiceBridge*>::iterator It;
On 2010/07/06 18:15:02, andreip3000 wrote:
> it

I think the more common pattern is to typedef the container, and then you can
use
WebDeviceOrienteationServiceBridgePointerSet::iterator i = ...

In this case, that's still rather a mouthful though..

http://codereview.chromium.org/2858049/diff/50001/51004
File chrome/browser/device_orientation/orientation.h (right):

http://codereview.chromium.org/2858049/diff/50001/51004#newcode10
chrome/browser/device_orientation/orientation.h:10: struct Orientation {
maybe document what this is, what alpha beta gamma refer to. (Or link to the
class/place that docs it)

http://codereview.chromium.org/2858049/diff/50001/51006
File chrome/browser/device_orientation/provider.h (right):

http://codereview.chromium.org/2858049/diff/50001/51006#newcode44
chrome/browser/device_orientation/provider.h:44: virtual void
RemoveObserver(Observer* observer) {}
how can these be usefully overridden, given GetInstance() calls the base class
constructor.
Maybe add a TODO explaining what needs to happen (or at least hint that it's not
finished?)

http://codereview.chromium.org/2858049/diff/50001/51006#newcode51
chrome/browser/device_orientation/provider.h:51: }
as you have a .cc file, probably as well to move function bodies there.

http://codereview.chromium.org/2858049/diff/50001/51012
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/2858049/diff/50001/51012#newcode821
chrome/common/render_messages.h:821: // variable.
found it :-)

Maybe move this doc to the Orientation struct, and just link to that here?
("these fields have the same meaning as device_orientation::Orientation")

[hans, 2010-08-11 08:37:38]

Thanks for the review guys.

Uploading new patch addressing the comments below. Will land this as after
taking it for a spin on the try bots.

http://codereview.chromium.org/2858049/diff/50001/51003
File chrome/browser/device_orientation/dispatcher_host.h (right):

http://codereview.chromium.org/2858049/diff/50001/51003#newcode22
chrome/browser/device_orientation/dispatcher_host.h:22: explicit
DispatcherHost(int resource_message_filter_process_id);
On 2010/08/10 13:39:15, Jeremy Orlow wrote:
> I don't see why you need resource_message_filter_ in this name.

Done. Renamed to just process_id.

http://codereview.chromium.org/2858049/diff/50001/51003#newcode26
chrome/browser/device_orientation/dispatcher_host.h:26: virtual void
OnOrientationUpdate(const Orientation& o);
On 2010/08/10 13:39:15, Jeremy Orlow wrote:
> o is a bad variable name

Done.

http://codereview.chromium.org/2858049/diff/50001/51004
File chrome/browser/device_orientation/orientation.h (right):

http://codereview.chromium.org/2858049/diff/50001/51004#newcode10
chrome/browser/device_orientation/orientation.h:10: struct Orientation {
On 2010/08/10 14:25:05, joth wrote:
> maybe document what this is, what alpha beta gamma refer to. (Or link to the
> class/place that docs it)

Done. Moved the comment from render_messages.h as suggested below.

http://codereview.chromium.org/2858049/diff/50001/51006
File chrome/browser/device_orientation/provider.h (right):

http://codereview.chromium.org/2858049/diff/50001/51006#newcode44
chrome/browser/device_orientation/provider.h:44: virtual void
RemoveObserver(Observer* observer) {}
On 2010/08/10 14:25:05, joth wrote:
> how can these be usefully overridden, given GetInstance() calls the base class
> constructor.
> Maybe add a TODO explaining what needs to happen (or at least hint that it's
not
> finished?)
> 

Done. Adding a TODO.

http://codereview.chromium.org/2858049/diff/50001/51006#newcode51
chrome/browser/device_orientation/provider.h:51: }
On 2010/08/10 14:25:05, joth wrote:
> as you have a .cc file, probably as well to move function bodies there.

Done.

http://codereview.chromium.org/2858049/diff/50001/51012
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/2858049/diff/50001/51012#newcode821
chrome/common/render_messages.h:821: // variable.
On 2010/08/10 14:25:05, joth wrote:
> found it :-)
> 
> Maybe move this doc to the Orientation struct, and just link to that here?
> ("these fields have the same meaning as device_orientation::Orientation")
> 

Done.

http://codereview.chromium.org/2858049/diff/50001/51015
File chrome/renderer/device_orientation_dispatcher.h (right):

http://codereview.chromium.org/2858049/diff/50001/51015#newcode8
chrome/renderer/device_orientation_dispatcher.h:8: #include
"third_party/WebKit/WebKit/chromium/public/WebDeviceOrientationClient.h"
On 2010/08/10 13:39:15, Jeremy Orlow wrote:
> It's OK this is >80 cols.

ok.