Use MessageLoopProxy instead of MessageLoop to dispatch notifications in ObserverListThreadsafe.

Use MessageLoopProxy instead of MessageLoop to dispatch notifications in ObserverListThreadsafe.

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

[darin (slow to review), 2011-08-08 18:18:26]

it seems like a possibly simpler approach would be to continue using
MessageLoop::current() as the key, but store the MessageLoopProxy to use in the
ObserversListMap?

http://codereview.chromium.org/7584016/diff/3001/base/observer_list_threadsafe.h
File base/observer_list_threadsafe.h (right):

http://codereview.chromium.org/7584016/diff/3001/base/observer_list_threadsaf...
base/observer_list_threadsafe.h:272: base::MessageLoopProxy*
GetExistingMessageLoopProxyUnlocked() {
The "Unlocked" suffix is a little confusing to me.  It sounds like the function
is called when we do not have the lock held, but that's actually the opposite of
how it should be used.  We often use a suffix of "_Locked" to indicate when a
method should be called with the lock already held.

[adamk, 2011-08-08 19:23:38]

I'd initially gone with my original approach as a "simpler" option, but it looks
like your suggestion does lead to a smaller delta from the original code, and
saves a bit of memory.  I've created a wrapper class for the proxy/list pair, to
avoid nested "it->first->second" accesses of std::pairs.  Let me know what you
think.

[darin (slow to review), 2011-08-08 19:33:21]

LGTM

http://codereview.chromium.org/7584016/diff/5001/base/observer_list_threadsafe.h
File base/observer_list_threadsafe.h (right):

http://codereview.chromium.org/7584016/diff/5001/base/observer_list_threadsaf...
base/observer_list_threadsafe.h:99: typename ObserversListMap::iterator it =
observer_lists_.find(loop);
nit: maybe it is not worth it, but you could just do the insert without the
find.  to avoid allocating an ObseverListContext each time, you could only set
that field if need be.

  std::pair<typename ObserversListMap::iterator, bool> result =
      observer_lists_.insert(std::make_pair(loop, NULL));
  if (result.second)
    result.first->second = new ObserverListContext();

http://codereview.chromium.org/7584016/diff/5001/base/observer_list_threadsaf...
base/observer_list_threadsafe.h:197: base::MessageLoopProxy& loop() { return
*message_loop_proxy_; }
perhaps there is no point in these member variables being private?

[adamk, 2011-08-08 19:50:03]

http://codereview.chromium.org/7584016/diff/5001/base/observer_list_threadsafe.h
File base/observer_list_threadsafe.h (right):

http://codereview.chromium.org/7584016/diff/5001/base/observer_list_threadsaf...
base/observer_list_threadsafe.h:99: typename ObserversListMap::iterator it =
observer_lists_.find(loop);
On 2011/08/08 19:33:21, darin wrote:
> nit: maybe it is not worth it, but you could just do the insert without the
> find.  to avoid allocating an ObseverListContext each time, you could only set
> that field if need be.
> 
>   std::pair<typename ObserversListMap::iterator, bool> result =
>       observer_lists_.insert(std::make_pair(loop, NULL));
>   if (result.second)
>     result.first->second = new ObserverListContext();

I've made the code do this.  Certainly not a big deal on performance, but easy
enough to do.

http://codereview.chromium.org/7584016/diff/5001/base/observer_list_threadsaf...
base/observer_list_threadsafe.h:197: base::MessageLoopProxy& loop() { return
*message_loop_proxy_; }
On 2011/08/08 19:33:21, darin wrote:
> perhaps there is no point in these member variables being private?

I suppose not.  Made this a struct instead (though left it non-copyable).

[darin (slow to review), 2011-08-08 20:23:59]

LGTM

[adamk, 2011-08-08 22:21:53]

FYI, trybots found a bug in my change (wasn't passing NotificationType down to
the child ObserverLists).  I've now fixed the bug and added a proper unittest
for that case.  Feel free to take another look if you so desire.

[darin (slow to review), 2011-08-08 22:38:41]

LGTM