Replace changes_channel with an observer list.

Replace changes_channel with an observer list.

BUG=none
TEST=Run unit tests

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

[chron, 2010-05-18 21:25:08]

As discussed, we should encapsulate a bit of the logic. Looks good though.

On Tue, May 18, 2010 at 1:51 PM, <zork@chromium.org> wrote:

> Reviewers: chron_chromium.org,
>
> Description:
> Replace changes_channel with an observer list.
>
> BUG=none
> TEST=Run unit tests
>
> Please review this at http://codereview.chromium.org/2075012/show
>
> Affected files:
>  M chrome/browser/sync/engine/syncapi.cc
>  M chrome/browser/sync/syncable/syncable.h
>  M chrome/browser/sync/syncable/syncable.cc
>
>
>


-- 
Now With Calcium and Vitamin D

[Zachary Kuznia, 2010-05-20 00:03:25]

Encapsulated.  Please have another look.

[chron_chromium.org, 2010-05-20 01:05:29]

http://codereview.chromium.org/2075012/diff/11001/12001
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/2075012/diff/11001/12001#newcode833
chrome/browser/sync/engine/syncapi.cc:833: virtual void HandleChannelEvent(const
syncable::DirectoryChangeEvent& event);
doesn't this mean the class can only have one event channel?

http://codereview.chromium.org/2075012/diff/11001/12004
File chrome/browser/sync/util/channel.h (right):

http://codereview.chromium.org/2075012/diff/11001/12004#newcode16
chrome/browser/sync/util/channel.h:16: virtual void HandleChannelEvent(
Can a class handle more than one type of event? It seems like this would prevent
it from doing so if it bakes it in.

[Zachary Kuznia, 2010-05-20 15:39:52]

http://codereview.chromium.org/2075012/diff/11001/12001
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/2075012/diff/11001/12001#newcode833
chrome/browser/sync/engine/syncapi.cc:833: virtual void HandleChannelEvent(const
syncable::DirectoryChangeEvent& event);
No, the argument is based on the channel type.  But, it can only have one event
channel per event type.

http://codereview.chromium.org/2075012/diff/11001/12004
File chrome/browser/sync/util/channel.h (right):

http://codereview.chromium.org/2075012/diff/11001/12004#newcode16
chrome/browser/sync/util/channel.h:16: virtual void HandleChannelEvent(
As long as the EventType is different, it's fine.

[chron_chromium.org, 2010-05-20 17:49:04]

http://codereview.chromium.org/2075012/diff/20001/17004
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/2075012/diff/20001/17004#newcode843
chrome/browser/sync/engine/syncapi.cc:843: void HandleChannelEvent(const
SyncerEvent& event);
do you mean virtual

http://codereview.chromium.org/2075012/diff/20001/17017
File chrome/browser/sync/util/channel.h (right):

http://codereview.chromium.org/2075012/diff/20001/17017#newcode11
chrome/browser/sync/util/channel.h:11: namespace browser_sync {
Please add usage notes, examples, and some comments to this file.

[chron_chromium.org, 2010-05-20 17:58:48]

http://codereview.chromium.org/2075012/diff/20001/17017
File chrome/browser/sync/util/channel.h (right):

http://codereview.chromium.org/2075012/diff/20001/17017#newcode19
chrome/browser/sync/util/channel.h:19: virtual void HandleChannelEvent(
is this line wrap necessary

http://codereview.chromium.org/2075012/diff/20001/17017#newcode52
chrome/browser/sync/util/channel.h:52: bool need_lock = (locking_thread_ !=
PlatformThread::CurrentId());
If locking_thread is only set in notify, is the point of this check exclusively
so you can remove observers in response to a notification? Is this needed? Add
comment.

Add a test for this, and related behavior.

http://codereview.chromium.org/2075012/diff/20001/17017#newcode53
chrome/browser/sync/util/channel.h:53: if (need_lock)
If it's the case that need_lock is only to test for being locked in notify,
please assertacquired the actual lock.

http://codereview.chromium.org/2075012/diff/20001/17017#newcode62
chrome/browser/sync/util/channel.h:62: locking_thread_ =
PlatformThread::CurrentId();
Why is it necessary only to set locking thread here?

http://codereview.chromium.org/2075012/diff/20001/17017#newcode66
chrome/browser/sync/util/channel.h:66: locking_thread_ = 0;
Is zero guaranteed to be invalid? is there a constant for this?

[Zachary Kuznia, 2010-05-31 10:14:17]

http://codereview.chromium.org/2075012/diff/20001/17004
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/2075012/diff/20001/17004#newcode843
chrome/browser/sync/engine/syncapi.cc:843: void HandleChannelEvent(const
SyncerEvent& event);
On 2010/05/20 17:49:04, chron_chromium.org wrote:
> do you mean virtual

Done.

http://codereview.chromium.org/2075012/diff/20001/17017
File chrome/browser/sync/util/channel.h (right):

http://codereview.chromium.org/2075012/diff/20001/17017#newcode11
chrome/browser/sync/util/channel.h:11: namespace browser_sync {
On 2010/05/20 17:49:04, chron_chromium.org wrote:
> Please add usage notes, examples, and some comments to this file.

Done.

http://codereview.chromium.org/2075012/diff/20001/17017#newcode19
chrome/browser/sync/util/channel.h:19: virtual void HandleChannelEvent(
No.  Removed.

On 2010/05/20 17:58:49, chron_chromium.org wrote:
> is this line wrap necessary

http://codereview.chromium.org/2075012/diff/20001/17017#newcode52
chrome/browser/sync/util/channel.h:52: bool need_lock = (locking_thread_ !=
PlatformThread::CurrentId());
Commented and test added.

On 2010/05/20 17:58:49, chron_chromium.org wrote:
> If locking_thread is only set in notify, is the point of this check
exclusively
> so you can remove observers in response to a notification? Is this needed? Add
> comment.
> 
> Add a test for this, and related behavior.

http://codereview.chromium.org/2075012/diff/20001/17017#newcode53
chrome/browser/sync/util/channel.h:53: if (need_lock)
On 2010/05/20 17:58:49, chron_chromium.org wrote:
> If it's the case that need_lock is only to test for being locked in notify,
> please assertacquired the actual lock.

Done.

http://codereview.chromium.org/2075012/diff/20001/17017#newcode62
chrome/browser/sync/util/channel.h:62: locking_thread_ =
PlatformThread::CurrentId();
AddObserver and RemoveObserver can't cause other code to be called.

On 2010/05/20 17:58:49, chron_chromium.org wrote:
> Why is it necessary only to set locking thread here?

http://codereview.chromium.org/2075012/diff/20001/17017#newcode66
chrome/browser/sync/util/channel.h:66: locking_thread_ = 0;
There is no constant, but 0 is guaranteed to be invalid on all platforms.

On 2010/05/20 17:58:49, chron_chromium.org wrote:
> Is zero guaranteed to be invalid? is there a constant for this?

[chron_chromium.org, 2010-06-01 18:37:05]

LGTM with fixes

http://codereview.chromium.org/2075012/diff/31001/32016
File chrome/browser/sync/util/channel.h (right):

http://codereview.chromium.org/2075012/diff/31001/32016#newcode18
chrome/browser/sync/util/channel.h:18: // TYPICAL USAGE:
In the overview, note the expected add / remove order, destruction order, and
typical behavior. Like, what happens if we try to shut down the channel while
events are still in it, etc? Document some more of the behavior.

http://codereview.chromium.org/2075012/diff/31001/32016#newcode65
chrome/browser/sync/util/channel.h:65: // This class managers a connection to a
channel.  When it is destructed, it
manages

http://codereview.chromium.org/2075012/diff/31001/32016#newcode65
chrome/browser/sync/util/channel.h:65: // This class managers a connection to a
channel.  When it is destructed, it
destroyed?