device_orientation::DispatcherHost: one Provider observer per render_view_id.

device_orientation::DispatcherHost: one Provider observer per render_view_id.

Refactor device_orientation::DispatcherHost to create a 1-to-1 relationship
between a RenderView and an observer of the Provider. The intention is
to remove the need for logic from DispatcherHost, and in effect have the
RenderView observe the Provider.

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

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

[jorlow, 2010-08-23 13:30:36]

I like this...seems pretty elegant. Few (double checking) questions:

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

http://codereview.chromium.org/3152043/diff/1/2#newcode77
chrome/browser/device_orientation/dispatcher_host.cc:77: process_id_,
space in once

http://codereview.chromium.org/3152043/diff/1/2#newcode84
chrome/browser/device_orientation/dispatcher_host.cc:84:
observers_map_.erase(render_view_id);
Should we dcheck that there's actually something to erase?

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

http://codereview.chromium.org/3152043/diff/1/3#newcode19
chrome/browser/device_orientation/dispatcher_host.h:19: class DispatcherHost :
public base::RefCountedThreadSafe<DispatcherHost> {
Does this need to be threadsafe?

http://codereview.chromium.org/3152043/diff/1/3#newcode32
chrome/browser/device_orientation/dispatcher_host.h:32: class ObserverHelper :
public Provider::Observer,
I believe ObserverDelegate would be the more typical name for such a thing? 
(Please double check.)

http://codereview.chromium.org/3152043/diff/1/3#newcode33
chrome/browser/device_orientation/dispatcher_host.h:33: public
base::RefCounted<ObserverHelper> {
Does this need to be threadsafe?

[joth, 2010-08-23 13:49:35]

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

http://codereview.chromium.org/3152043/diff/1/2#newcode27
chrome/browser/device_orientation/dispatcher_host.cc:27:
DispatcherHost::ObserverHelper::ObserverHelper(scoped_refptr<Provider> provider,
strictly this should either be const scoped_refptr<>& or just a naked Provider*

I think the later is fine; a bit easier to read and has very little draw back

http://codereview.chromium.org/3152043/diff/1/2#newcode33
chrome/browser/device_orientation/dispatcher_host.cc:33:
provider_->AddObserver(this);
I notice this may synchronously callback into OnOrientationUpdate(). I think
this is fine (as that sends a message, it naturally limits the call stack depth)
but maybe worth noting with a comment.

http://codereview.chromium.org/3152043/diff/1/2#newcode73
chrome/browser/device_orientation/dispatcher_host.cc:73: if (!provider_)
do you need to stash this as a member now, or could you just pass
Provider::GetInstance() in the expression below?

http://codereview.chromium.org/3152043/diff/1/2#newcode75
chrome/browser/device_orientation/dispatcher_host.cc:75: 
is it OK for this method to get called multiple times? maybe
if (observers_map_.count(render_view_id) == 0) ...

or DCHECK_EQ(0, observers_map_.count(render_view_id)) if it's not ok.

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

http://codereview.chromium.org/3152043/diff/1/3#newcode31
chrome/browser/device_orientation/dispatcher_host.h:31: // Helper class that
observes a Provider and forwards updates to a RenderView.
you can probably just declare this here, and define it in the .cc

http://codereview.chromium.org/3152043/diff/1/3#newcode57
chrome/browser/device_orientation/dispatcher_host.h:57: std::map<int,
scoped_refptr<ObserverHelper> > observers_map_;
document what the int is.

[hans, 2010-08-23 14:53:14]

Thanks for the input. Addressing your comments and uploading new patch.

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

http://codereview.chromium.org/3152043/diff/1/2#newcode27
chrome/browser/device_orientation/dispatcher_host.cc:27:
DispatcherHost::ObserverHelper::ObserverHelper(scoped_refptr<Provider> provider,
On 2010/08/23 13:49:35, joth wrote:
> strictly this should either be const scoped_refptr<>& or just a naked
Provider*
> 
> I think the later is fine; a bit easier to read and has very little draw back

Going with the naked pointer.

http://codereview.chromium.org/3152043/diff/1/2#newcode33
chrome/browser/device_orientation/dispatcher_host.cc:33:
provider_->AddObserver(this);
On 2010/08/23 13:49:35, joth wrote:
> I notice this may synchronously callback into OnOrientationUpdate(). I think
> this is fine (as that sends a message, it naturally limits the call stack
depth)
> but maybe worth noting with a comment.

Done. Adding comment in provider.h.

http://codereview.chromium.org/3152043/diff/1/2#newcode73
chrome/browser/device_orientation/dispatcher_host.cc:73: if (!provider_)
On 2010/08/23 13:49:35, joth wrote:
> do you need to stash this as a member now, or could you just pass
> Provider::GetInstance() in the expression below?

I stash it as a member to keep the reference count above zero as long as the
DispatcherHost is alive. This is not strictly necessary, except that it makes
testing easier: if the reference count drops to zero, the Provider is destructed
and then we would potentially loose an injected mock Provider prematurely.

http://codereview.chromium.org/3152043/diff/1/2#newcode75
chrome/browser/device_orientation/dispatcher_host.cc:75: 
On 2010/08/23 13:49:35, joth wrote:
> is it OK for this method to get called multiple times? maybe
> if (observers_map_.count(render_view_id) == 0) ...
> 
> or DCHECK_EQ(0, observers_map_.count(render_view_id)) if it's not ok.
> 

It's OK, and it will work nicely. The old ObserverDelegate is destructed, and
replaced by a new one. This is deliberate.

http://codereview.chromium.org/3152043/diff/1/2#newcode77
chrome/browser/device_orientation/dispatcher_host.cc:77: process_id_,
On 2010/08/23 13:30:36, jorlow wrote:
> space in once

Done.

http://codereview.chromium.org/3152043/diff/1/2#newcode84
chrome/browser/device_orientation/dispatcher_host.cc:84:
observers_map_.erase(render_view_id);
On 2010/08/23 13:30:36, jorlow wrote:
> Should we dcheck that there's actually something to erase?

Calling erase() for a non-existent key works fine. OnStopUpdating may be called
more than once for a RenderView if the web page calls window.removeEventListener
repeatedly.

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

http://codereview.chromium.org/3152043/diff/1/3#newcode19
chrome/browser/device_orientation/dispatcher_host.h:19: class DispatcherHost :
public base::RefCountedThreadSafe<DispatcherHost> {
On 2010/08/23 13:30:36, jorlow wrote:
> Does this need to be threadsafe?

Not sure. I think I just copied that from GeolocationDispatcherHost. Other
ResourceMessageFilter members have it too, e.g. IndexedDBDispatcherHost. I think
ResourceMessageFilter is constructed and destructed on different threads, so
that might be the reason?

http://codereview.chromium.org/3152043/diff/1/3#newcode31
chrome/browser/device_orientation/dispatcher_host.h:31: // Helper class that
observes a Provider and forwards updates to a RenderView.
On 2010/08/23 13:49:35, joth wrote:
> you can probably just declare this here, and define it in the .cc

I tried that, but because of the observers_map_ member, it wouldn't work (the
scoped_refptr needs to see the declaration of the ObserverDelegate destructor, I
think).

http://codereview.chromium.org/3152043/diff/1/3#newcode32
chrome/browser/device_orientation/dispatcher_host.h:32: class ObserverHelper :
public Provider::Observer,
On 2010/08/23 13:30:36, jorlow wrote:
> I believe ObserverDelegate would be the more typical name for such a thing? 
> (Please double check.)

Yes, that sounds better. Renaming.

http://codereview.chromium.org/3152043/diff/1/3#newcode33
chrome/browser/device_orientation/dispatcher_host.h:33: public
base::RefCounted<ObserverHelper> {
On 2010/08/23 13:30:36, jorlow wrote:
> Does this need to be threadsafe?

No. Those objects are only constructed, destructed or otherwise accessed on the
IO thread.

http://codereview.chromium.org/3152043/diff/1/3#newcode57
chrome/browser/device_orientation/dispatcher_host.h:57: std::map<int,
scoped_refptr<ObserverHelper> > observers_map_;
On 2010/08/23 13:49:35, joth wrote:
> document what the int is.

Done.

[jorlow, 2010-08-23 14:59:29]

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

http://codereview.chromium.org/3152043/diff/1/3#newcode19
chrome/browser/device_orientation/dispatcher_host.h:19: class DispatcherHost :
public base::RefCountedThreadSafe<DispatcherHost> {
On 2010/08/23 14:53:15, hans wrote:
> On 2010/08/23 13:30:36, jorlow wrote:
> > Does this need to be threadsafe?
> 
> Not sure. I think I just copied that from GeolocationDispatcherHost. Other
> ResourceMessageFilter members have it too, e.g. IndexedDBDispatcherHost. I
think
> ResourceMessageFilter is constructed and destructed on different threads, so
> that might be the reason?
> 

ResourceMessageFilter is created on the UI thread and then handed off to the IO
thread.  No sync issues here.  If this is only used on the IO thread then it
shouldn't need to be threadsafe.

[hans, 2010-08-23 15:24:32]

Ok. Making it just RefCounted, and sending it for an extra try run :)

[joth, 2010-08-23 16:38:32]

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

http://codereview.chromium.org/3152043/diff/1/3#newcode19
chrome/browser/device_orientation/dispatcher_host.h:19: class DispatcherHost :
public base::RefCountedThreadSafe<DispatcherHost> {
On 2010/08/23 14:59:29, jorlow wrote:
> On 2010/08/23 14:53:15, hans wrote:
> > On 2010/08/23 13:30:36, jorlow wrote:
> > > Does this need to be threadsafe?
> > 
> > Not sure. I think I just copied that from GeolocationDispatcherHost. Other
> > ResourceMessageFilter members have it too, e.g. IndexedDBDispatcherHost. I
> think
> > ResourceMessageFilter is constructed and destructed on different threads, so
> > that might be the reason?
> > 
> 
> ResourceMessageFilter is created on the UI thread and then handed off to the
IO
> thread.  No sync issues here.  If this is only used on the IO thread then it
> shouldn't need to be threadsafe.

Right, it's only needed it you pass task across threads that reference this
object. (GeolocationDispatcherHost does, for the ui prompts).
(Kind of unfortunate that this internal detail percolates through to the public
interface like this)

http://codereview.chromium.org/3152043/diff/1/3#newcode31
chrome/browser/device_orientation/dispatcher_host.h:31: // Helper class that
observes a Provider and forwards updates to a RenderView.
On 2010/08/23 14:53:15, hans wrote:
> On 2010/08/23 13:49:35, joth wrote:
> > you can probably just declare this here, and define it in the .cc
> 
> I tried that, but because of the observers_map_ member, it wouldn't work (the
> scoped_refptr needs to see the declaration of the ObserverDelegate destructor,
I
> think).

Fair enough (although I'm mildly interested to see the error and work out why
:-)

nit: RefCounted... should be the first base class as it's the one that's not
pure interface.

[hans, 2010-08-24 08:47:32]

On 2010/08/23 16:38:32, joth wrote:
> http://codereview.chromium.org/3152043/diff/1/3
> File chrome/browser/device_orientation/dispatcher_host.h (right):
> 
> http://codereview.chromium.org/3152043/diff/1/3#newcode19
> chrome/browser/device_orientation/dispatcher_host.h:19: class DispatcherHost :
> public base::RefCountedThreadSafe<DispatcherHost> {
> On 2010/08/23 14:59:29, jorlow wrote:
> > On 2010/08/23 14:53:15, hans wrote:
> > > On 2010/08/23 13:30:36, jorlow wrote:
> > > > Does this need to be threadsafe?
> > > 
> > > Not sure. I think I just copied that from GeolocationDispatcherHost. Other
> > > ResourceMessageFilter members have it too, e.g. IndexedDBDispatcherHost. I
> > think
> > > ResourceMessageFilter is constructed and destructed on different threads,
so
> > > that might be the reason?
> > > 
> > 
> > ResourceMessageFilter is created on the UI thread and then handed off to the
> IO
> > thread.  No sync issues here.  If this is only used on the IO thread then it
> > shouldn't need to be threadsafe.
> 
> Right, it's only needed it you pass task across threads that reference this
> object. (GeolocationDispatcherHost does, for the ui prompts).
> (Kind of unfortunate that this internal detail percolates through to the
public
> interface like this)
> 
> http://codereview.chromium.org/3152043/diff/1/3#newcode31
> chrome/browser/device_orientation/dispatcher_host.h:31: // Helper class that
> observes a Provider and forwards updates to a RenderView.
> On 2010/08/23 14:53:15, hans wrote:
> > On 2010/08/23 13:49:35, joth wrote:
> > > you can probably just declare this here, and define it in the .cc
> > 
> > I tried that, but because of the observers_map_ member, it wouldn't work
(the
> > scoped_refptr needs to see the declaration of the ObserverDelegate
destructor,
> I
> > think).
> 
> Fair enough (although I'm mildly interested to see the error and work out why
> :-)
Hmm, something must have been different in the class when I first tried it,
because now it works fine. Thanks for pointing this out :)

> 
> nit: RefCounted... should be the first base class as it's the one that's not
> pure interface.
Done.

[hans, 2010-08-24 11:39:32]

Ping? If there are no further comments, I'd like to land this.

[joth, 2010-08-24 11:49:54]

LGTM

Thanks,

On 2010/08/24 11:39:32, hans wrote:
> Ping? If there are no further comments, I'd like to land this.