sync: Add --enable-sync-sessions-v2 and necessary plumbing to use SessionsSyncManager.

sync: Add --enable-sync-sessions-v2 and necessary plumbing to use SessionsSyncManager.

Not a fully working solution as SSM doesn't listen to session changes after the
initial association, but does work across restarts.

Adds OpenTabsUIDelegate to a) reduce SessionModelAssociator proliferation and b)
allow swapping in a SessionsSyncManager if the command line flag is passed.

The changes in non sync/ files are basically s/SessionModelAssociator/OpenTabsUIDelegate.  There are a few places I didn't make that change yet (such as OnSyncCycleComplete in ProfileSyncService), which I'll have to do in a follow up as SSM supports more of the bells-and-whistles in session sync.

BUG=98892
R=pkasting@chromium.org, rlarocque@chromium.org, yfriedman@chromium.org
TBR=tburkard@chromium.org, asargent@chromium.org, yfriedman@chromium.org, dbeam@chromium.org, pkasting@chromium.org
for chrome/browser/prerender

for chrome/browser/extensions/api

for chrome/browser/android and chrome/browser/ui/webui/ntp/android

for chrome/browser/ui/webui/ntp

for remainder of chrome/browser/ui

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236572

[rlarocque, 2013-11-18 23:20:08]

https://codereview.chromium.org/74653002/diff/530001/chrome/browser/sync/open...
File chrome/browser/sync/open_tabs_ui_delegate.h (right):

https://codereview.chromium.org/74653002/diff/530001/chrome/browser/sync/open...
chrome/browser/sync/open_tabs_ui_delegate.h:20: class OpenTabsUIDelegate {
I think this name is a bit misleading.  Shouldn't the work "sync" be in there
somewhere?

https://codereview.chromium.org/74653002/diff/530001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/74653002/diff/530001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:310: // ### DONT USE THIS ANYMORE!
***
That's a pretty strong comment.

nit: Could you be consistent in the use of * vs #?

https://codereview.chromium.org/74653002/diff/530001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:335: virtual std::string
GetLocalDeviceGUID() const OVERRIDE;
Is this the sync cache GUID?  If so, could you rename this method to make that
more explicit?  You might also want to mention the lifetime of this ID (It gets
reset when we sign out and sign back in to sync, or when we receive a birthday
error, etc.)

The reason I make this comment is that there's more than one client GUID. 
Invalidations has its own ID with its own lifecycle.

[tim (not reviewing), 2013-11-20 18:38:56]

https://codereview.chromium.org/74653002/diff/530001/chrome/browser/sync/open...
File chrome/browser/sync/open_tabs_ui_delegate.h (right):

https://codereview.chromium.org/74653002/diff/530001/chrome/browser/sync/open...
chrome/browser/sync/open_tabs_ui_delegate.h:20: class OpenTabsUIDelegate {
On 2013/11/18 23:20:08, rlarocque wrote:
> I think this name is a bit misleading.  Shouldn't the work "sync" be in there
> somewhere?

It is; it's a browser_sync::OpenTabsUIDelegate. Perhaps you didn't notice from
this file alone, but I think it reads well at the callsites right now and adding
Sync would be redundant..

https://codereview.chromium.org/74653002/diff/530001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/74653002/diff/530001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:310: // ### DONT USE THIS ANYMORE!
***
On 2013/11/18 23:20:08, rlarocque wrote:
> That's a pretty strong comment.
> 
> nit: Could you be consistent in the use of * vs #?

Yeah, I want to be a bit scary :)  I'll be removing this Real Soon Now with any
luck.

https://codereview.chromium.org/74653002/diff/530001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:335: virtual std::string
GetLocalDeviceGUID() const OVERRIDE;
On 2013/11/18 23:20:08, rlarocque wrote:
> Is this the sync cache GUID?  If so, could you rename this method to make that
> more explicit?  You might also want to mention the lifetime of this ID (It
gets
> reset when we sign out and sign back in to sync, or when we receive a birthday
> error, etc.)
> 
> The reason I make this comment is that there's more than one client GUID. 
> Invalidations has its own ID with its own lifecycle.  

Good point.  I actually had it called CacheGuid (see sessions_sync_manager.h),
but since I wrote that patch someone checked in this method in
ProfileSyncService and I decided to consolidate them.  But I'll change and
update the callsites of this method.

[rlarocque, 2013-11-20 18:41:54]

lgtm

[tim (not reviewing), 2013-11-20 20:51:57]

TBR=tburkard@chromium.org
for chrome/browser/prerender

TBR=asargent@chromium.org
for chrome/browser/extensions/api

TBR=yfriedman@chromium.org
for chrome/browser/android

TBR=dbeam@chromium.org
for chrome/browser/ui/webui

TBR=pkasting@chromium.org 
for remainder of chrome/browser/ui

[Peter Kasting, 2013-11-20 21:07:45]

The non-nit here should probably be addressed in a separate change, so LGTM, but
do consider addressing it eventually.

https://codereview.chromium.org/74653002/diff/740001/chrome/browser/ui/toolba...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h (right):

https://codereview.chromium.org/74653002/diff/740001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:50: // If |delegate| is
NULL, default delegate for |browser|'s profile will
Nit: default -> the default

https://codereview.chromium.org/74653002/diff/740001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:54:
browser_sync::OpenTabsUIDelegate* delegate);
Nit: Name this identically to the member (without trailing underscore)

https://codereview.chromium.org/74653002/diff/740001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:120:
browser_sync::OpenTabsUIDelegate* GetOpenTabsUIDelegate();
This function should be separated from the one above, and have a comment,
especially since it's actually crucial currently that no one use |open_tabs_|
directly, and instead must call this function!

This seems very error-prone.  It's easy for a later person to just use
open_tabs_ and then have things break.  Can we instead inline the functionality
of this function into the constructor, so |open_tabs_| is set correctly?  If not
(because we need to wait for an event that happens after construction), can we
replace this accessor function by a callback registration so that once the
correct value is available, we get called back to update open_tabs_?

https://codereview.chromium.org/74653002/diff/740001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:128:
browser_sync::OpenTabsUIDelegate* open_tabs_;  // Weak.
Nit: |open_tabs_delegate_|

[Yaron, 2013-11-20 22:18:01]

lgtm

[tim (not reviewing), 2013-11-21 02:00:06]

Addressed Peter's comments.

https://codereview.chromium.org/74653002/diff/740001/chrome/browser/ui/toolba...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h (right):

https://codereview.chromium.org/74653002/diff/740001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:50: // If |delegate| is
NULL, default delegate for |browser|'s profile will
On 2013/11/20 21:07:45, Peter Kasting wrote:
> Nit: default -> the default

Done.

https://codereview.chromium.org/74653002/diff/740001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:54:
browser_sync::OpenTabsUIDelegate* delegate);
On 2013/11/20 21:07:45, Peter Kasting wrote:
> Nit: Name this identically to the member (without trailing underscore)

Done.

https://codereview.chromium.org/74653002/diff/740001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:120:
browser_sync::OpenTabsUIDelegate* GetOpenTabsUIDelegate();
On 2013/11/20 21:07:45, Peter Kasting wrote:
> This function should be separated from the one above, and have a comment,
> especially since it's actually crucial currently that no one use |open_tabs_|
> directly, and instead must call this function!
> 
> This seems very error-prone.  It's easy for a later person to just use
> open_tabs_ and then have things break.  Can we instead inline the
functionality
> of this function into the constructor, so |open_tabs_| is set correctly?  If
not
> (because we need to wait for an event that happens after construction), can we
> replace this accessor function by a callback registration so that once the
> correct value is available, we get called back to update open_tabs_?

Yeah, I'm not sure we can guarantee the delegate can always be pulled up on
construction (I'll reply back if I find otherwise).

As to your suggestion of a callback registration, are you voting for requiring
code accessing open_tabs_delegate_ to NULL-check the value first and deal with
that, rather than force them to go through the lazy-initializer?  Or did you
have something else in mind for avoiding pre-initialized usage.

I'll add a TODO here for now and follow up.

https://codereview.chromium.org/74653002/diff/740001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:128:
browser_sync::OpenTabsUIDelegate* open_tabs_;  // Weak.
On 2013/11/20 21:07:45, Peter Kasting wrote:
> Nit: |open_tabs_delegate_|

Done.

[Peter Kasting, 2013-11-21 02:15:43]

https://codereview.chromium.org/74653002/diff/740001/chrome/browser/ui/toolba...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h (right):

https://codereview.chromium.org/74653002/diff/740001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:120:
browser_sync::OpenTabsUIDelegate* GetOpenTabsUIDelegate();
On 2013/11/21 02:00:06, timsteele wrote:
> As to your suggestion of a callback registration, are you voting for requiring
> code accessing open_tabs_delegate_ to NULL-check the value first and deal with
> that, rather than force them to go through the lazy-initializer?

The issue is whether such code can run before we've gotten the callback.  If so,
then there's nothing to be done: this object will be NULL and no attempt to
lazy-init will succeed, so the methods in question have to handle NULL (and have
to do so in today's code).

If, however, we expect some of these methods to only be called once this object
ought to be initialized, they can simply DCHECK that it's non-NULL and not have
to do anything else.

[commit-bot: I haz the power, 2013-11-21 02:40:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/74653002/1070001

[commit-bot: I haz the power, 2013-11-21 04:21:45]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-21 18:45:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/74653002/1300001

[tim (not reviewing), 2013-11-21 19:37:23]

Committed patchset #5 manually as r236572 (presubmit successful).