[sync] Divorce DataTypeManager from NotificationService notifications by creating a new DataTypeMa…

[sync] 	Divorce DataTypeManager from NotificationService notifications by creating a new DataTypeManagerOserver interface

As of today, DataTypeManager sends out notifications for various
configure events via NotificationService notifications. This is bad,
because other services that depend on these events need to have
knowledge of DataTypeManager, which is internal to the sync
implementation.

This patch does the following:

1) Creates a new DatTypeManagerObserver interface through which
   observers are made aware of configure events.
2) Has ProfileSyncService implement the interface.
3) Gets rid of unnecessary sync configure * notification types.
4) Has PSS send out notifications for SYNC_CONFIGURE_START and
   SYNC_CONFIGURE_DONE.
5) Allows ForeignSessionHandler to listen to SYNC_CONFIGURE_DONE
   notifications from the PSS, instead of from all sources, while
   waiting to populate the "other devices" section of the NTP.
6) Updates various mock and test classes and fixes unit tests.

TBR=estade@chromium.org, thakis@chromium.org
BUG=142984, 124717, 111676, 142550
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=152170

[Raghu Simha, 2012-08-16 04:34:57]

Nicolas / Tim, could one of you review? Thanks!

[tim (not reviewing), 2012-08-16 18:25:20]

http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/glue/da...
File chrome/browser/sync/glue/data_type_manager.h (right):

http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/glue/da...
chrome/browser/sync/glue/data_type_manager.h:53: typedef
browser_sync::DataTypeManagerObserver Observer;
I'm tempted to suggest defining the interface as a nested class rather than
typedef / separate file, similar to how we've done it in sync service and
SyncManager.  But I won't.  I N C E P T I O N

http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/glue/da...
chrome/browser/sync/glue/data_type_manager.h:122: virtual const ConfigureResult&
last_configure_result() const = 0;
As you brought up offline, you can pass this to the observer and don't need to
cache.

http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/glue/da...
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/glue/da...
chrome/browser/sync/glue/data_type_manager_impl.cc:174: last_configure_result_ =
ConfigureResult(DataTypeManager::RETRY);
Was there a reason you chose to move retry to its own callback?  That works, but
if we do that we probably dont need DataTypeManager::RETRY as a ConfigureResult
status any more.  We should make sure the delineation between statuses and
dedicated callbacks is clear.  

i.e. 
OnSyncConfigureDone(OK, ABORTED, PARTIAL_SUCCESS)
OnSyncConfigureStart()
OnSyncConfigureBlocked()
OnSyncConfigureRetry()?

http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:496: if
(data_type_manager_->HasObserver(this))
I think if data_type_manager_.get(), then this if could be a DCHECK?

http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:1008:
content::NotificationService::current()->Notify(
I just realized, another nice part about this new design is that the fact that
the ProfileSyncService gets the notification _first_ is explicit, whereas before
it was based on registration order.

We ran into trouble with backend migrator here before, note that it has a
special "OnConfigureDone handler". I'd like to avoid having multiple ways to do
the same thing, and avoid other classes (even within sync) being tempted to add
themselves as an observer of the DTM and potentially throw the wheels out of
alignment.  I think we can do that by passing in a single
DataTypeManagerObserver on construction and removing all the observer_list
maintenance since there'd only ever be one.  I think, (even in tests?).

http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:298: void UpdateConfigureTimes();
What's this for?

[Raghu Simha, 2012-08-17 01:56:42]

Tim, PTAL.

+dubroy for the change to
chrome/browser/ui/webui/ntp/foreign_session_handler.cc.

http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/glue/da...
File chrome/browser/sync/glue/data_type_manager.h (right):

http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/glue/da...
chrome/browser/sync/glue/data_type_manager.h:53: typedef
browser_sync::DataTypeManagerObserver Observer;
On 2012/08/16 18:25:20, timsteele wrote:
> I'm tempted to suggest defining the interface as a nested class rather than
> typedef / separate file, similar to how we've done it in sync service and
> SyncManager.  But I won't.  I N C E P T I O N

:)

How about you review patch set 2 and let me know if you still feel strongly
about the nested class?

http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/glue/da...
chrome/browser/sync/glue/data_type_manager.h:122: virtual const ConfigureResult&
last_configure_result() const = 0;
On 2012/08/16 18:25:20, timsteele wrote:
> As you brought up offline, you can pass this to the observer and don't need to
> cache.

Done.

http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/glue/da...
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/glue/da...
chrome/browser/sync/glue/data_type_manager_impl.cc:174: last_configure_result_ =
ConfigureResult(DataTypeManager::RETRY);
On 2012/08/16 18:25:20, timsteele wrote:
> Was there a reason you chose to move retry to its own callback?

I did so based on the TODO that used to sit here. See bug crbug.com/111676.

>  That works, but
> if we do that we probably dont need DataTypeManager::RETRY as a
ConfigureResult
> status any more.  We should make sure the delineation between statuses and
> dedicated callbacks is clear.  
> 
> i.e. 
> OnSyncConfigureDone(OK, ABORTED, PARTIAL_SUCCESS)
> OnSyncConfigureStart()
> OnSyncConfigureBlocked()
> OnSyncConfigureRetry()?

Agreed. Changed interface and removed DataTypeManager::RETRY.

http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:496: if
(data_type_manager_->HasObserver(this))
On 2012/08/16 18:25:20, timsteele wrote:
> I think if data_type_manager_.get(), then this if could be a DCHECK?

Moot. We no longer add / remove observers from here.

http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:1008:
content::NotificationService::current()->Notify(
On 2012/08/16 18:25:20, timsteele wrote:
> I just realized, another nice part about this new design is that the fact that
> the ProfileSyncService gets the notification _first_ is explicit, whereas
before
> it was based on registration order.

True. I realised that while debugging and liked the idea.
 
> We ran into trouble with backend migrator here before, note that it has a
> special "OnConfigureDone handler". I'd like to avoid having multiple ways to
do
> the same thing, and avoid other classes (even within sync) being tempted to
add
> themselves as an observer of the DTM and potentially throw the wheels out of
> alignment.  I think we can do that by passing in a single
> DataTypeManagerObserver on construction and removing all the observer_list
> maintenance since there'd only ever be one.  I think, (even in tests?).
> 

I like the idea. Very much. Done. Let me know what you think of patch set 2.

http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:298: void UpdateConfigureTimes();
On 2012/08/16 18:25:20, timsteele wrote:
> What's this for?

I added it so I could share the code that updated the start / stop times and UMA
logs between OnConfigureDone and OnConfigureRetry. Turns out we don't hit that
code path in OnConfigureRetry (makes sense, now that I think of it).

Method removed.

[tim (not reviewing), 2012-08-17 19:53:02]

http://codereview.chromium.org/10834351/diff/8026/chrome/browser/sync/glue/da...
File chrome/browser/sync/glue/data_type_manager_impl.h (right):

http://codereview.chromium.org/10834351/diff/8026/chrome/browser/sync/glue/da...
chrome/browser/sync/glue/data_type_manager_impl.h:116: DataTypeManagerObserver*
observer_;
use const pointer (* const observer_;)

http://codereview.chromium.org/10834351/diff/8026/chrome/browser/sync/glue/da...
File chrome/browser/sync/glue/data_type_manager_impl_unittest.cc (left):

http://codereview.chromium.org/10834351/diff/8026/chrome/browser/sync/glue/da...
chrome/browser/sync/glue/data_type_manager_impl_unittest.cc:591:
SetConfigureDoneExpectation(DataTypeManager::PARTIAL_SUCCESS);
It's still useful to test that the DTM does inform observers properly.  Did you
intend on removing all these expectations or are they captured another way?

[Patrick Dubroy, 2012-08-17 20:29:13]

Changes to ForeignSessionHandler LGTM.

On 2012/08/17 01:56:42, rsimha wrote:
> Tim, PTAL.
> 
> +dubroy for the change to
> chrome/browser/ui/webui/ntp/foreign_session_handler.cc.
> 
>
http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/glue/da...
> File chrome/browser/sync/glue/data_type_manager.h (right):
> 
>
http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/glue/da...
> chrome/browser/sync/glue/data_type_manager.h:53: typedef
> browser_sync::DataTypeManagerObserver Observer;
> On 2012/08/16 18:25:20, timsteele wrote:
> > I'm tempted to suggest defining the interface as a nested class rather than
> > typedef / separate file, similar to how we've done it in sync service and
> > SyncManager.  But I won't.  I N C E P T I O N
> 
> :)
> 
> How about you review patch set 2 and let me know if you still feel strongly
> about the nested class?
> 
>
http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/glue/da...
> chrome/browser/sync/glue/data_type_manager.h:122: virtual const
ConfigureResult&
> last_configure_result() const = 0;
> On 2012/08/16 18:25:20, timsteele wrote:
> > As you brought up offline, you can pass this to the observer and don't need
to
> > cache.
> 
> Done.
> 
>
http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/glue/da...
> File chrome/browser/sync/glue/data_type_manager_impl.cc (right):
> 
>
http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/glue/da...
> chrome/browser/sync/glue/data_type_manager_impl.cc:174: last_configure_result_
=
> ConfigureResult(DataTypeManager::RETRY);
> On 2012/08/16 18:25:20, timsteele wrote:
> > Was there a reason you chose to move retry to its own callback?
> 
> I did so based on the TODO that used to sit here. See bug crbug.com/111676.
> 
> >  That works, but
> > if we do that we probably dont need DataTypeManager::RETRY as a
> ConfigureResult
> > status any more.  We should make sure the delineation between statuses and
> > dedicated callbacks is clear.  
> > 
> > i.e. 
> > OnSyncConfigureDone(OK, ABORTED, PARTIAL_SUCCESS)
> > OnSyncConfigureStart()
> > OnSyncConfigureBlocked()
> > OnSyncConfigureRetry()?
> 
> Agreed. Changed interface and removed DataTypeManager::RETRY.
> 
>
http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/profile...
> File chrome/browser/sync/profile_sync_service.cc (right):
> 
>
http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/profile...
> chrome/browser/sync/profile_sync_service.cc:496: if
> (data_type_manager_->HasObserver(this))
> On 2012/08/16 18:25:20, timsteele wrote:
> > I think if data_type_manager_.get(), then this if could be a DCHECK?
> 
> Moot. We no longer add / remove observers from here.
> 
>
http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/profile...
> chrome/browser/sync/profile_sync_service.cc:1008:
> content::NotificationService::current()->Notify(
> On 2012/08/16 18:25:20, timsteele wrote:
> > I just realized, another nice part about this new design is that the fact
that
> > the ProfileSyncService gets the notification _first_ is explicit, whereas
> before
> > it was based on registration order.
> 
> True. I realised that while debugging and liked the idea.
>  
> > We ran into trouble with backend migrator here before, note that it has a
> > special "OnConfigureDone handler". I'd like to avoid having multiple ways to
> do
> > the same thing, and avoid other classes (even within sync) being tempted to
> add
> > themselves as an observer of the DTM and potentially throw the wheels out of
> > alignment.  I think we can do that by passing in a single
> > DataTypeManagerObserver on construction and removing all the observer_list
> > maintenance since there'd only ever be one.  I think, (even in tests?).
> > 
> 
> I like the idea. Very much. Done. Let me know what you think of patch set 2.
> 
>
http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/profile...
> File chrome/browser/sync/profile_sync_service.h (right):
> 
>
http://codereview.chromium.org/10834351/diff/1040/chrome/browser/sync/profile...
> chrome/browser/sync/profile_sync_service.h:298: void UpdateConfigureTimes();
> On 2012/08/16 18:25:20, timsteele wrote:
> > What's this for?
> 
> I added it so I could share the code that updated the start / stop times and
UMA
> logs between OnConfigureDone and OnConfigureRetry. Turns out we don't hit that
> code path in OnConfigureRetry (makes sense, now that I think of it).
> 
> Method removed.

[Raghu Simha, 2012-08-17 21:06:06]

Tim, PTAL.

http://codereview.chromium.org/10834351/diff/8026/chrome/browser/sync/glue/da...
File chrome/browser/sync/glue/data_type_manager_impl.h (right):

http://codereview.chromium.org/10834351/diff/8026/chrome/browser/sync/glue/da...
chrome/browser/sync/glue/data_type_manager_impl.h:116: DataTypeManagerObserver*
observer_;
On 2012/08/17 19:53:02, timsteele wrote:
> use const pointer (* const observer_;)
> 

Done.

http://codereview.chromium.org/10834351/diff/8026/chrome/browser/sync/glue/da...
File chrome/browser/sync/glue/data_type_manager_impl_unittest.cc (left):

http://codereview.chromium.org/10834351/diff/8026/chrome/browser/sync/glue/da...
chrome/browser/sync/glue/data_type_manager_impl_unittest.cc:591:
SetConfigureDoneExpectation(DataTypeManager::PARTIAL_SUCCESS);
On 2012/08/17 19:53:02, timsteele wrote:
> It's still useful to test that the DTM does inform observers properly.  Did
you
> intend on removing all these expectations or are they captured another way?

Crap. Good catch. I meant to restore these expectations with the
observer-as-member redesign, and change the way they were satisfied. Done.

[tim (not reviewing), 2012-08-17 21:16:21]

LGTM

[Raghu Simha, 2012-08-17 22:30:54]

+OWNERS
estade: chrome/browser/ui/webui/ntp/foreign_session_handler.cc
thakis: chrome/chrome_browser.gypi and chrome/common/chrome_notification_types.h

Thanks!

[Nico, 2012-08-17 23:31:15]

> thakis: chrome/chrome_browser.gypi and
chrome/common/chrome_notification_types.h

These files lgtm.