Fix syncing of sessions. Numerous changes have been made. Currently, the mode...

Fix syncing of sessions. Numerous changes have been made. Currently, the model associator does not have a local model to associate with, but instead contains a buffer of protobuf specifics for foreign sessions which gets completely overwritten everytime an update occurs. This buffer is then used to create a vector of foreign sessions for each foreign session handler. As a result, The model associator is slightly different from other datatypes.

The creation of a persistent unique machine tag needs to be resolved still. Something understandable by the user would be good (home, work, etc.), but for now we use the directory kernel's cache_guid. Unfortunately, this gets reset each time sync is enabled/disabled, resulting in stale client session info that remains visible.

BUG=30519
TEST=unit_test


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

[Nicolas Zea, 2010-10-14 23:24:14]

UI changes can be found here: http://codereview.chromium.org/3767009/show

[tim (not reviewing), 2010-10-15 05:47:41]

This looks like a nice improvement.  Good call on using NotificationService!

http://codereview.chromium.org/3825005/diff/28002/29002
File chrome/browser/dom_ui/foreign_session_handler.cc (right):

http://codereview.chromium.org/3825005/diff/28002/29002#newcode62
chrome/browser/dom_ui/foreign_session_handler.cc:62: return;
This isn't really necessary.

http://codereview.chromium.org/3825005/diff/28002/29009
File chrome/browser/sync/glue/session_change_processor.cc (right):

http://codereview.chromium.org/3825005/diff/28002/29009#newcode68
chrome/browser/sync/glue/session_change_processor.cc:68: // Replaces old
UpdateModel with an all-at-once functionality that ignores
i wouldn't reference UpdateModel here if it doesn't exist anymore.

http://codereview.chromium.org/3825005/diff/28002/29009#newcode75
chrome/browser/sync/glue/session_change_processor.cc:75: // (what has changed,
how many sessions, etc. Not useful for now.)
I get what you're saying... but I think we can skip the "todo: use details if we
need it" todo :)

I would, however, add a source parameter (perhaps just this change processor). 
WIthout it, it likely breaks the multi profile scenario, which would break the
integration tests (if we had some running with this code :) )

http://codereview.chromium.org/3825005/diff/28002/29009#newcode112
chrome/browser/sync/glue/session_change_processor.cc:112:
NotificationType::FOREIGN_SESSION_DISABLED,
I don't think I understand this notification, we call StopObserving in
ApplyModelChanges, for example...

http://codereview.chromium.org/3825005/diff/28002/29012
File chrome/browser/sync/glue/session_model_associator.h (right):

http://codereview.chromium.org/3825005/diff/28002/29012#newcode129
chrome/browser/sync/glue/session_model_associator.h:129: std::vector<const
sync_pb::SessionSpecifics*>& specifics);
nit - indent

http://codereview.chromium.org/3825005/diff/28002/29013
File chrome/browser/sync/profile_sync_service_session_unittest.cc (right):

http://codereview.chromium.org/3825005/diff/28002/29013#newcode140
chrome/browser/sync/profile_sync_service_session_unittest.cc:140: int
notification_type_;
what's this used for?

[Nicolas Zea, 2010-10-15 19:13:09]

I've left the current machine tag solution alone for now, and simply described
the problem in the comments.

http://codereview.chromium.org/3825005/diff/28002/29002
File chrome/browser/dom_ui/foreign_session_handler.cc (right):

http://codereview.chromium.org/3825005/diff/28002/29002#newcode62
chrome/browser/dom_ui/foreign_session_handler.cc:62: return;
On 2010/10/15 05:47:41, timsteele wrote:
> This isn't really necessary.

Done.

http://codereview.chromium.org/3825005/diff/28002/29009
File chrome/browser/sync/glue/session_change_processor.cc (right):

http://codereview.chromium.org/3825005/diff/28002/29009#newcode68
chrome/browser/sync/glue/session_change_processor.cc:68: // Replaces old
UpdateModel with an all-at-once functionality that ignores
On 2010/10/15 05:47:41, timsteele wrote:
> i wouldn't reference UpdateModel here if it doesn't exist anymore.

Done.

http://codereview.chromium.org/3825005/diff/28002/29009#newcode75
chrome/browser/sync/glue/session_change_processor.cc:75: // (what has changed,
how many sessions, etc. Not useful for now.)
On 2010/10/15 05:47:41, timsteele wrote:
> I get what you're saying... but I think we can skip the "todo: use details if
we
> need it" todo :)
> 
> I would, however, add a source parameter (perhaps just this change processor).

> WIthout it, it likely breaks the multi profile scenario, which would break the
> integration tests (if we had some running with this code :) )

Done.

http://codereview.chromium.org/3825005/diff/28002/29009#newcode112
chrome/browser/sync/glue/session_change_processor.cc:112:
NotificationType::FOREIGN_SESSION_DISABLED,
On 2010/10/15 05:47:41, timsteele wrote:
> I don't think I understand this notification, we call StopObserving in
> ApplyModelChanges, for example...

Done.

I think this is more appropriate is |DisassociateModels|

http://codereview.chromium.org/3825005/diff/28002/29012
File chrome/browser/sync/glue/session_model_associator.h (right):

http://codereview.chromium.org/3825005/diff/28002/29012#newcode129
chrome/browser/sync/glue/session_model_associator.h:129: std::vector<const
sync_pb::SessionSpecifics*>& specifics);
On 2010/10/15 05:47:41, timsteele wrote:
> nit - indent

Done.

http://codereview.chromium.org/3825005/diff/28002/29013
File chrome/browser/sync/profile_sync_service_session_unittest.cc (right):

http://codereview.chromium.org/3825005/diff/28002/29013#newcode140
chrome/browser/sync/profile_sync_service_session_unittest.cc:140: int
notification_type_;
On 2010/10/15 05:47:41, timsteele wrote:
> what's this used for?

Done.

[tim (not reviewing), 2010-10-19 18:38:12]

On 2010/10/15 19:13:09, nzea wrote:
> I've left the current machine tag solution alone for now, and simply described
> the problem in the comments.
> 
> http://codereview.chromium.org/3825005/diff/28002/29002
> File chrome/browser/dom_ui/foreign_session_handler.cc (right):
> 
> http://codereview.chromium.org/3825005/diff/28002/29002#newcode62
> chrome/browser/dom_ui/foreign_session_handler.cc:62: return;
> On 2010/10/15 05:47:41, timsteele wrote:
> > This isn't really necessary.
> 
> Done.
> 
> http://codereview.chromium.org/3825005/diff/28002/29009
> File chrome/browser/sync/glue/session_change_processor.cc (right):
> 
> http://codereview.chromium.org/3825005/diff/28002/29009#newcode68
> chrome/browser/sync/glue/session_change_processor.cc:68: // Replaces old
> UpdateModel with an all-at-once functionality that ignores
> On 2010/10/15 05:47:41, timsteele wrote:
> > i wouldn't reference UpdateModel here if it doesn't exist anymore.
> 
> Done.
> 
> http://codereview.chromium.org/3825005/diff/28002/29009#newcode75
> chrome/browser/sync/glue/session_change_processor.cc:75: // (what has changed,
> how many sessions, etc. Not useful for now.)
> On 2010/10/15 05:47:41, timsteele wrote:
> > I get what you're saying... but I think we can skip the "todo: use details
if
> we
> > need it" todo :)
> > 
> > I would, however, add a source parameter (perhaps just this change
processor).
> 
> > WIthout it, it likely breaks the multi profile scenario, which would break
the
> > integration tests (if we had some running with this code :) )
> 
> Done.
> 
> http://codereview.chromium.org/3825005/diff/28002/29009#newcode112
> chrome/browser/sync/glue/session_change_processor.cc:112:
> NotificationType::FOREIGN_SESSION_DISABLED,
> On 2010/10/15 05:47:41, timsteele wrote:
> > I don't think I understand this notification, we call StopObserving in
> > ApplyModelChanges, for example...
> 
> Done.
> 
> I think this is more appropriate is |DisassociateModels|
> 
> http://codereview.chromium.org/3825005/diff/28002/29012
> File chrome/browser/sync/glue/session_model_associator.h (right):
> 
> http://codereview.chromium.org/3825005/diff/28002/29012#newcode129
> chrome/browser/sync/glue/session_model_associator.h:129: std::vector<const
> sync_pb::SessionSpecifics*>& specifics);
> On 2010/10/15 05:47:41, timsteele wrote:
> > nit - indent
> 
> Done.
> 
> http://codereview.chromium.org/3825005/diff/28002/29013
> File chrome/browser/sync/profile_sync_service_session_unittest.cc (right):
> 
> http://codereview.chromium.org/3825005/diff/28002/29013#newcode140
> chrome/browser/sync/profile_sync_service_session_unittest.cc:140: int
> notification_type_;
> On 2010/10/15 05:47:41, timsteele wrote:
> > what's this used for?
> 
> Done.

LGTM!