[Sync] Replace uses of ObserverListThreadSafe with WeakHandles

[Sync] Replace uses of ObserverListThreadSafe with WeakHandles

ObserverListThreadSafe was overkill since there was only two threads
involved, and only one observer.

Add MessageLoop member variables to various test classes that need it
(ObserverListThreadSafe had a hack that made it unnecessary before.)

BUG=103732
TEST=Start with a clean profile and set up sync.  about:profiler shouldn't
show ObserverListThreadSafe::Notify calls with sync threads anymore.


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

[akalin, 2011-11-17 02:27:47]

+zea for review

[Nicolas Zea, 2011-11-17 21:35:45]

http://codereview.chromium.org/8586014/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncer_proto_util_unittest.cc (right):

http://codereview.chromium.org/8586014/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncer_proto_util_unittest.cc:141: MessageLoop
message_loop_;
What is this used by?

http://codereview.chromium.org/8586014/diff/1/chrome/browser/sync/internal_ap...
File chrome/browser/sync/internal_api/debug_info_event_listener.h (right):

http://codereview.chromium.org/8586014/diff/1/chrome/browser/sync/internal_ap...
chrome/browser/sync/internal_api/debug_info_event_listener.h:9: #include <queue>
q before s

http://codereview.chromium.org/8586014/diff/1/chrome/browser/sync/notifier/no...
File chrome/browser/sync/notifier/non_blocking_invalidation_notifier.cc (right):

http://codereview.chromium.org/8586014/diff/1/chrome/browser/sync/notifier/no...
chrome/browser/sync/notifier/non_blocking_invalidation_notifier.cc:124:
delegate_observer_.Call(FROM_HERE,
I wonder, would it be possible to keep the observer_list in the core, have each
observer actually be a weakhandle and then have something like:

FOR_EACH_OBSERVER(SyncNotifierObserver, observers_, Call(FROM_HERE, ..., ...)).

Might need to make a special weak handle observerlist macro. It's seems
unnecessary to have to do this extra plumbing of core -> notifier -> observer
just because you might have multiple observers.

WDYT?

http://codereview.chromium.org/8586014/diff/1/chrome/browser/sync/syncable/sy...
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/8586014/diff/1/chrome/browser/sync/syncable/sy...
chrome/browser/sync/syncable/syncable.cc:359: new Kernel(FilePath(), name,
KernelLoadInfo(),
does this not fit on previous line?

[akalin, 2011-11-18 00:04:32]

Addressed all comments, ptal.

http://codereview.chromium.org/8586014/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncer_proto_util_unittest.cc (right):

http://codereview.chromium.org/8586014/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncer_proto_util_unittest.cc:141: MessageLoop
message_loop_;
On 2011/11/17 21:35:45, nzea wrote:
> What is this used by?

Any test that opens a Transaction uses a WeakHandle, which needs to have been
constructed on a thread with a loop.  We didn't have this problem when we used
OLTS because it had a hack to work around this. (This is mentioned in the CL
description.)

http://codereview.chromium.org/8586014/diff/1/chrome/browser/sync/internal_ap...
File chrome/browser/sync/internal_api/debug_info_event_listener.h (right):

http://codereview.chromium.org/8586014/diff/1/chrome/browser/sync/internal_ap...
chrome/browser/sync/internal_api/debug_info_event_listener.h:9: #include <queue>
On 2011/11/17 21:35:45, nzea wrote:
> q before s

Done.

http://codereview.chromium.org/8586014/diff/1/chrome/browser/sync/notifier/no...
File chrome/browser/sync/notifier/non_blocking_invalidation_notifier.cc (right):

http://codereview.chromium.org/8586014/diff/1/chrome/browser/sync/notifier/no...
chrome/browser/sync/notifier/non_blocking_invalidation_notifier.cc:124:
delegate_observer_.Call(FROM_HERE,
On 2011/11/17 21:35:45, nzea wrote:
> I wonder, would it be possible to keep the observer_list in the core, have
each
> observer actually be a weakhandle and then have something like:
> 
> FOR_EACH_OBSERVER(SyncNotifierObserver, observers_, Call(FROM_HERE, ...,
...)).
> 
> Might need to make a special weak handle observerlist macro. It's seems
> unnecessary to have to do this extra plumbing of core -> notifier -> observer
> just because you might have multiple observers.
> 
> WDYT?

I looked into this and basically it's much more complicated than the current
solution.

The first problem is that ObserverLists store pointers, so it would have to
store WeakHandle<T>*, which is weird.

However, it's technically possible to have a WeakHandle to the ObserverList
itself, since it implements SupportsWeakPtr.  I'd have to make a new function,
say, ObserverList<T>::Notify(), or NotifyObservers(..., ObserverList<T>*) and
add support for binding to functions/callbacks to WeakHandle.  However, the
major weakness of this is that the first argument to NotifyObservers (the
function to apply to each observer) would be a callback, but then it would have
to be a zero-arg member function, since there's no support for binding just a
member function arguments (as opposed to the object variable).  Even if we had
that, it seems less clear, IMO.

Another possibility would be a special WeakHandleObserverList<T> which would
store WeakHandle<T>s.  The major problem with that, though, is that it's in
general impossible to remove an observer even if you're on the same thread
(i.e., you're able to get the WeakPtr), since the WeakPtr may already be
invalid, and thus the pointer is NULL, so you don't know which one to remove. 
Although then I guess it doesn't really matter.  But in this case it would
require the observer type itself to expose WeakPtrs, since the public interface
still takes a pointer.

So yeah, I think this is the best solution. :/

http://codereview.chromium.org/8586014/diff/1/chrome/browser/sync/syncable/sy...
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/8586014/diff/1/chrome/browser/sync/syncable/sy...
chrome/browser/sync/syncable/syncable.cc:359: new Kernel(FilePath(), name,
KernelLoadInfo(),
On 2011/11/17 21:35:45, nzea wrote:
> does this not fit on previous line?

Sure.  Fixed.

[Nicolas Zea, 2011-11-18 00:33:34]

lgtm

[commit-bot: I haz the power, 2011-11-19 03:55:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/8586014/17001

[commit-bot: I haz the power, 2011-11-19 04:50:51]

Try job failure for 8586014-17001 (retry) on linux_rel for step "ui_tests".
It's a second try, previously, steps "check_deps, ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-11-19 08:22:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/8586014/17001

[commit-bot: I haz the power, 2011-11-19 09:31:34]

Change committed as 110836