[Sync] Add logic to reassociate tab nodes after restart.

[Sync] Add logic to reassociate tab nodes after restart.

Change SessionModelAssociator and TabNodePool to reuse old tab nodes.
Old tab nodes are now added to TabNodePool and if there is an
existing old tab node with syncid of tab, the tab is reassociated
with that node.
Note: In order for tab reassociation to work, the syncid of a tab
should be persisted. This CL just introduces the sync changes for
reassociating tab nodes. Persisting syncid with tab will be done
in a separate CL for Desktop, and in a downstream CL for Android.

BUG=139670, 12549, 139666
TEST=Includes unit tests + MANUAL testing on Android.

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

[shashi, 2013-06-05 22:03:29]

Downstream patch:
https://gerrit-int.chromium.org/#/c/39177

[Nicolas Zea, 2013-06-06 21:27:39]

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/session_model_associator.cc (right):

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:213: SyncedTabDelegate*
syncedTab = (*i)->GetTabAt(j);
Given your other patch, you probably need to handle the !syncedTab case.

Also, this should be named synced_tab per style guide

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:217: if
(syncedTab->GetSyncId() > 0 &&
perhaps have GetSyncId() return syncer::kInvalidId and check against that
instead?

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:218:
UpdateTabIdIfNeccessary(syncedTab->GetSyncId(), tab_id)) {
Remind me why we need to update the sync node's id, rather than the android
tab's id? The sync node's id corresponds to the id encoded in the unique tag, so
should never change.

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:219:
used_sync_ids.insert(syncedTab->GetSyncId());
shouldn't this always be called? Even if you generated a new sync id?

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:248: if
(window_s.tab_size() > 0) {
note that this is going to change behavior. Previously we would only add the
window if valid non-chrome:// tabs were found. Now we'll add the window, even
though it will show up with no tabs (unless you also chnage the definition of
valid tab)

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:264:
tab_pool_.FreeUnusedOldSyncNodes(used_sync_ids);
UnusedOld seems a bit redundant. Perhaps FreeUnsyncedSynceNodes is enough

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:333: if
(tab_pool_.RemoveIfOldSyncNodeExists(tab->GetSyncId())) {
This name isn't very clear about what's going on. How about
ReassociateOldTabNode

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/session_model_associator.h (right):

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.h:113: bool
AssociateTab(SyncedTabDelegate* tab, syncer::SyncError* error);
comment that |tab| will be updated with the sync id if necessary

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.h:333: bool
UpdateTabIdIfNeccessary(int64 sync_id, SessionID::id_type new_tab_id);
nit: neccessary only has one c

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/tab_node_pool.cc (right):

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
chrome/browser/sync/glue/tab_node_pool.cc:45: old_node_syncids_.insert(sync_id);
I'm not sure we really need to have the notion of "old tab nodes" at all. Could
you not track all these old syncids as just free nodes within the tab syncid
pool (although this might switching from vector to something else)

The general flow could be:
AddTabNode // adds old node to sync id pool, but doesn't track it as associated
AssociateTabNode  // marks it as associated, and therefore not free
FreeUnusuedTabNodes  // free's all non-associated nodes
GetFreeTabNode  // returns next free node, or creates one as necessary

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/tab_node_pool.h (right):

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
chrome/browser/sync/glue/tab_node_pool.h:35: void AddOldTabNode(int64 sync_id,
size_t tab_node_id);
I think this should be moved down to be next to the other "old tab" methods

[shashi, 2013-06-07 01:44:08]

I thought to address design questions first, to make sure I am in the right
direction. I am working on fixing the other (mostly style related) comments.

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/session_model_associator.cc (right):

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:218:
UpdateTabIdIfNeccessary(syncedTab->GetSyncId(), tab_id)) {
On 2013/06/06 21:27:39, Nicolas Zea wrote:
> Remind me why we need to update the sync node's id, rather than the android
> tab's id? The sync node's id corresponds to the id encoded in the unique tag,
so
> should never change.
There are two different fields: tab_node_id and tab_id. tab_id is
SyncedTabDelegate->GetSessionID essentially, tab_node_id is unique and used by
the unique tag.
We only update tab_id of the tab. We do not update tab_node_id, which still
remains unique. tab_id is updated because SessionID changes with restarts even
for tabs without web contents (TWW).
In short:
1. For normal tabs : update everything, use the normal
SMA::WriteTabContentsToSyncModel flow.
2. For TWW: just update tab_id, since it may have changed after the restart,
cannot update the rest because webcontents are available.

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:248: if
(window_s.tab_size() > 0) {
Oh, I thought the found_tabs flag was redundant, it seemed to be set whenever we
add a tab to the window.

On 2013/06/06 21:27:39, Nicolas Zea wrote:
> note that this is going to change behavior. Previously we would only add the
> window if valid non-chrome:// tabs were found. Now we'll add the window, even
> though it will show up with no tabs (unless you also chnage the definition of
> valid tab)

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/tab_node_pool.cc (right):

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
chrome/browser/sync/glue/tab_node_pool.cc:45: old_node_syncids_.insert(sync_id);
In case of Android we have two requirements:
1. Old tab nodes must not be deleted.
2. Less obvious, old tab nodes should get reassociated with the *same* old sync
node.

Requirement 1 should be obvious, let me explain reason for requirement 2:

On Android some tabs can be without webcontents. For tabs without webcontents
(TWW) we do not have data to write tabs contents to sync model. Instead we use a
trick, we let the old sync nodes associated with those tabs to be unchanged. 

Now consider this from the point of view of a remote client.
Remote client views that the sync nodes associated with TWW are unchanged, it
will display TWW as a regular tab.
For tabs which have webcontents any free node will work, because SMA will
rewrite the tab contents to the sync node (in SMA::WriteTabContentsToSyncModel).


Keeping track of old sync ids is simply to make sure that we do not accidentally
overwrite a sync node associated with TWW.

On 2013/06/06 21:27:39, Nicolas Zea wrote:
> I'm not sure we really need to have the notion of "old tab nodes" at all.
Could
> you not track all these old syncids as just free nodes within the tab syncid
> pool (although this might switching from vector to something else)
> 
> The general flow could be:
> AddTabNode // adds old node to sync id pool, but doesn't track it as
associated
> AssociateTabNode  // marks it as associated, and therefore not free
> FreeUnusuedTabNodes  // free's all non-associated nodes
> GetFreeTabNode  // returns next free node, or creates one as necessary

[Nicolas Zea, 2013-06-07 14:42:28]

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/session_model_associator.cc (right):

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:218:
UpdateTabIdIfNeccessary(syncedTab->GetSyncId(), tab_id)) {
On 2013/06/07 01:44:08, shashi wrote:
> On 2013/06/06 21:27:39, Nicolas Zea wrote:
> > Remind me why we need to update the sync node's id, rather than the android
> > tab's id? The sync node's id corresponds to the id encoded in the unique
tag,
> so
> > should never change.
> There are two different fields: tab_node_id and tab_id. tab_id is
> SyncedTabDelegate->GetSessionID essentially, tab_node_id is unique and used by
> the unique tag.
> We only update tab_id of the tab. We do not update tab_node_id, which still
> remains unique. tab_id is updated because SessionID changes with restarts even
> for tabs without web contents (TWW).
> In short:
> 1. For normal tabs : update everything, use the normal
> SMA::WriteTabContentsToSyncModel flow.
> 2. For TWW: just update tab_id, since it may have changed after the restart,
> cannot update the rest because webcontents are available.

Ah, right, that makes sense.

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:248: if
(window_s.tab_size() > 0) {
LookupSessionTab actually does a validity check for whether we should sync the
tab or not. So found_tab is set to true only when "valid" tabs are found.

On 2013/06/07 01:44:08, shashi wrote:
> Oh, I thought the found_tabs flag was redundant, it seemed to be set whenever
we
> add a tab to the window.
> 
> On 2013/06/06 21:27:39, Nicolas Zea wrote:
> > note that this is going to change behavior. Previously we would only add the
> > window if valid non-chrome:// tabs were found. Now we'll add the window,
even
> > though it will show up with no tabs (unless you also chnage the definition
of
> > valid tab)
>

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/tab_node_pool.cc (right):

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
chrome/browser/sync/glue/tab_node_pool.cc:45: old_node_syncids_.insert(sync_id);
On 2013/06/07 01:44:08, shashi wrote:
> In case of Android we have two requirements:
> 1. Old tab nodes must not be deleted.
> 2. Less obvious, old tab nodes should get reassociated with the *same* old
sync
> node.
> 
> Requirement 1 should be obvious, let me explain reason for requirement 2:
> 
> On Android some tabs can be without webcontents. For tabs without webcontents
> (TWW) we do not have data to write tabs contents to sync model. Instead we use
a
> trick, we let the old sync nodes associated with those tabs to be unchanged. 
> 
> Now consider this from the point of view of a remote client.
> Remote client views that the sync nodes associated with TWW are unchanged, it
> will display TWW as a regular tab.
> For tabs which have webcontents any free node will work, because SMA will
> rewrite the tab contents to the sync node (in
SMA::WriteTabContentsToSyncModel).
> 
> 
> Keeping track of old sync ids is simply to make sure that we do not
accidentally
> overwrite a sync node associated with TWW.

Right, and I agree with that approach. My point is that (I think) the API itself
doesn't need to reflect this. From an external point of view there are the
following categories of nodes:
1. Those associated with an active tab (associated nodes)
2. Those not associated with an active tab (free nodes)

All nodes have a "last associated tab node id", but start off as free on any
particular restart. The associator, at association time, tells the tab pool
which of the free nodes are in fact associated. Previously it didn't care about
associating with a particular node, so GetFreeTabNode was enough. Now, we do, so
a specific AssociateTabNode(sync_id) method is probably necessary.

But, given some sort of AssociateTabNode method, it's not clear to me that the
notion of "old" nodes really needs to be exposed.

[Yaron, 2013-06-07 17:08:34]

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/android/ta...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/android/ta...
chrome/browser/android/tab_android.cc:142: int64 TabAndroid::GetSyncId() const {
This class is abstract anyway. Just mark as pure virtual in the header and
remove implementations.

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/android/ta...
File chrome/browser/android/tab_android.h (right):

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/android/ta...
chrome/browser/android/tab_android.h:74: virtual int64 GetSyncId() const;
Add brief comment for the pair.

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/session_model_associator.cc (right):

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:1175:
syncer::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare());
On Android, you're more likely than not to take this route and you'd do it for
every swapped out tab. Nicolas: is there a lot of overhead to the transaction
such that it should be done once per associate or not much overhead and better
to keep it as narrow as possible?

[Nicolas Zea, 2013-06-07 17:24:04]

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/session_model_associator.cc (right):

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:1175:
syncer::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare());
On 2013/06/07 17:08:35, Yaron wrote:
> On Android, you're more likely than not to take this route and you'd do it for
> every swapped out tab. Nicolas: is there a lot of overhead to the transaction
> such that it should be done once per associate or not much overhead and better
> to keep it as narrow as possible?

All transactions involve locking, so yes, they should be minimized as much as
possible. I think one way to do that would be to update the tab id only as part
of AssociateTab (where we'll be opening a transaction anyways). 

This might involve ensuring we store the local tab information in the
SyncedSessionTracker. That way we don't have to load a tab node to check the tab
id, it'll already be part of the synced session tracker's information. To do
that, as part of UpdateAssociationsFromSyncModel, we'd pass the local tab node's
information to the SyncedSessionTracker.

[shashi, 2013-06-07 19:08:35]

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/session_model_associator.cc (right):

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:1175:
syncer::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare());
Thanks, I am going to try to implement Nicolas' suggestion, a map of (sync_id ->
tab_id), in SyncedSessionTracker, that way we open a transaction only when we
have to update the tab_id.

On 2013/06/07 17:24:04, Nicolas Zea wrote:
> On 2013/06/07 17:08:35, Yaron wrote:
> > On Android, you're more likely than not to take this route and you'd do it
for
> > every swapped out tab. Nicolas: is there a lot of overhead to the
transaction
> > such that it should be done once per associate or not much overhead and
better
> > to keep it as narrow as possible?
> 
> All transactions involve locking, so yes, they should be minimized as much as
> possible. I think one way to do that would be to update the tab id only as
part
> of AssociateTab (where we'll be opening a transaction anyways). 
> 
> This might involve ensuring we store the local tab information in the
> SyncedSessionTracker. That way we don't have to load a tab node to check the
tab
> id, it'll already be part of the synced session tracker's information. To do
> that, as part of UpdateAssociationsFromSyncModel, we'd pass the local tab
node's
> information to the SyncedSessionTracker.

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/tab_node_pool.cc (right):

https://codereview.chromium.org/16421003/diff/41018/chrome/browser/sync/glue/...
chrome/browser/sync/glue/tab_node_pool.cc:45: old_node_syncids_.insert(sync_id);
Got it, I am going to cleanup the API as per the suggestion.
On 2013/06/07 14:42:28, Nicolas Zea wrote:
> On 2013/06/07 01:44:08, shashi wrote:
> > In case of Android we have two requirements:
> > 1. Old tab nodes must not be deleted.
> > 2. Less obvious, old tab nodes should get reassociated with the *same* old
> sync
> > node.
> > 
> > Requirement 1 should be obvious, let me explain reason for requirement 2:
> > 
> > On Android some tabs can be without webcontents. For tabs without
webcontents
> > (TWW) we do not have data to write tabs contents to sync model. Instead we
use
> a
> > trick, we let the old sync nodes associated with those tabs to be unchanged.

> > 
> > Now consider this from the point of view of a remote client.
> > Remote client views that the sync nodes associated with TWW are unchanged,
it
> > will display TWW as a regular tab.
> > For tabs which have webcontents any free node will work, because SMA will
> > rewrite the tab contents to the sync node (in
> SMA::WriteTabContentsToSyncModel).
> > 
> > 
> > Keeping track of old sync ids is simply to make sure that we do not
> accidentally
> > overwrite a sync node associated with TWW.
> 
> Right, and I agree with that approach. My point is that (I think) the API
itself
> doesn't need to reflect this. From an external point of view there are the
> following categories of nodes:
> 1. Those associated with an active tab (associated nodes)
> 2. Those not associated with an active tab (free nodes)
> 
> All nodes have a "last associated tab node id", but start off as free on any
> particular restart. The associator, at association time, tells the tab pool
> which of the free nodes are in fact associated. Previously it didn't care
about
> associating with a particular node, so GetFreeTabNode was enough. Now, we do,
so
> a specific AssociateTabNode(sync_id) method is probably necessary.
> 
> But, given some sort of AssociateTabNode method, it's not clear to me that the
> notion of "old" nodes really needs to be exposed.

[shashi, 2013-06-11 19:15:06]

In order to apply feedback, I had to rewrite TabNodePool.
The new TabNodePool now maintains mapping of SyncID -> TabID and API does not
expose internal details about old tab nodes.

This also allows us to lookup tabId associated with a syncId instead of always
rewriting tabId for swapped out tabs. Overall I feel API is much cleaner.

https://chromiumcodereview.appspot.com/16421003/diff/41018/chrome/browser/and...
File chrome/browser/android/tab_android.cc (right):

https://chromiumcodereview.appspot.com/16421003/diff/41018/chrome/browser/and...
chrome/browser/android/tab_android.cc:142: int64 TabAndroid::GetSyncId() const {
On 2013/06/07 17:08:35, Yaron wrote:
> This class is abstract anyway. Just mark as pure virtual in the header and
> remove implementations.

Done.

https://chromiumcodereview.appspot.com/16421003/diff/41018/chrome/browser/and...
File chrome/browser/android/tab_android.h (right):

https://chromiumcodereview.appspot.com/16421003/diff/41018/chrome/browser/and...
chrome/browser/android/tab_android.h:74: virtual int64 GetSyncId() const;
On 2013/06/07 17:08:35, Yaron wrote:
> Add brief comment for the pair.

Done.

https://chromiumcodereview.appspot.com/16421003/diff/41018/chrome/browser/syn...
File chrome/browser/sync/glue/session_model_associator.cc (right):

https://chromiumcodereview.appspot.com/16421003/diff/41018/chrome/browser/syn...
chrome/browser/sync/glue/session_model_associator.cc:213: SyncedTabDelegate*
syncedTab = (*i)->GetTabAt(j);
On 2013/06/06 21:27:39, Nicolas Zea wrote:
> Given your other patch, you probably need to handle the !syncedTab case.
> 
> Also, this should be named synced_tab per style guide

Done.

https://chromiumcodereview.appspot.com/16421003/diff/41018/chrome/browser/syn...
chrome/browser/sync/glue/session_model_associator.cc:217: if
(syncedTab->GetSyncId() > 0 &&
kInvalidId, is declared in base_node.h and adding it to
tab_contents_synced_tab_delegate fails a deps check.
On 2013/06/06 21:27:39, Nicolas Zea wrote:
> perhaps have GetSyncId() return syncer::kInvalidId and check against that
> instead?

https://chromiumcodereview.appspot.com/16421003/diff/41018/chrome/browser/syn...
chrome/browser/sync/glue/session_model_associator.cc:219:
used_sync_ids.insert(syncedTab->GetSyncId());
On 2013/06/06 21:27:39, Nicolas Zea wrote:
> shouldn't this always be called? Even if you generated a new sync id?

It is also called when we look up for session tab:
synced_session_tracker_.LookupSessionTab is successful.

https://chromiumcodereview.appspot.com/16421003/diff/41018/chrome/browser/syn...
chrome/browser/sync/glue/session_model_associator.cc:248: if
(window_s.tab_size() > 0) {
Reverted.
On 2013/06/07 14:42:28, Nicolas Zea wrote:
> LookupSessionTab actually does a validity check for whether we should sync the
> tab or not. So found_tab is set to true only when "valid" tabs are found.
> 
> On 2013/06/07 01:44:08, shashi wrote:
> > Oh, I thought the found_tabs flag was redundant, it seemed to be set
whenever
> we
> > add a tab to the window.
> > 
> > On 2013/06/06 21:27:39, Nicolas Zea wrote:
> > > note that this is going to change behavior. Previously we would only add
the
> > > window if valid non-chrome:// tabs were found. Now we'll add the window,
> even
> > > though it will show up with no tabs (unless you also chnage the definition
> of
> > > valid tab)
> > 
>

https://chromiumcodereview.appspot.com/16421003/diff/41018/chrome/browser/syn...
chrome/browser/sync/glue/session_model_associator.cc:264:
tab_pool_.FreeUnusedOldSyncNodes(used_sync_ids);
On 2013/06/06 21:27:39, Nicolas Zea wrote:
> UnusedOld seems a bit redundant. Perhaps FreeUnsyncedSynceNodes is enough

Done.

https://chromiumcodereview.appspot.com/16421003/diff/41018/chrome/browser/syn...
chrome/browser/sync/glue/session_model_associator.cc:333: if
(tab_pool_.RemoveIfOldSyncNodeExists(tab->GetSyncId())) {
On 2013/06/06 21:27:39, Nicolas Zea wrote:
> This name isn't very clear about what's going on. How about
> ReassociateOldTabNode
Hopefully more clear now.

https://chromiumcodereview.appspot.com/16421003/diff/41018/chrome/browser/syn...
chrome/browser/sync/glue/session_model_associator.cc:1175:
syncer::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare());
Instead of adding to SessionTracker, tab_node_pool keeps track of mapping of
sync_id -> tab_id, this way we only open a transaction if needed.
On 2013/06/07 19:08:35, shashi wrote:
> Thanks, I am going to try to implement Nicolas' suggestion, a map of (sync_id
->
> tab_id), in SyncedSessionTracker, that way we open a transaction only when we
> have to update the tab_id.
> 
> On 2013/06/07 17:24:04, Nicolas Zea wrote:
> > On 2013/06/07 17:08:35, Yaron wrote:
> > > On Android, you're more likely than not to take this route and you'd do it
> for
> > > every swapped out tab. Nicolas: is there a lot of overhead to the
> transaction
> > > such that it should be done once per associate or not much overhead and
> better
> > > to keep it as narrow as possible?
> > 
> > All transactions involve locking, so yes, they should be minimized as much
as
> > possible. I think one way to do that would be to update the tab id only as
> part
> > of AssociateTab (where we'll be opening a transaction anyways). 
> > 
> > This might involve ensuring we store the local tab information in the
> > SyncedSessionTracker. That way we don't have to load a tab node to check the
> tab
> > id, it'll already be part of the synced session tracker's information. To do
> > that, as part of UpdateAssociationsFromSyncModel, we'd pass the local tab
> node's
> > information to the SyncedSessionTracker.
>

https://chromiumcodereview.appspot.com/16421003/diff/41018/chrome/browser/syn...
File chrome/browser/sync/glue/session_model_associator.h (right):

https://chromiumcodereview.appspot.com/16421003/diff/41018/chrome/browser/syn...
chrome/browser/sync/glue/session_model_associator.h:333: bool
UpdateTabIdIfNeccessary(int64 sync_id, SessionID::id_type new_tab_id);
On 2013/06/06 21:27:39, Nicolas Zea wrote:
> nit: neccessary only has one c

Done.

https://chromiumcodereview.appspot.com/16421003/diff/41018/chrome/browser/syn...
File chrome/browser/sync/glue/tab_node_pool.h (right):

https://chromiumcodereview.appspot.com/16421003/diff/41018/chrome/browser/syn...
chrome/browser/sync/glue/tab_node_pool.h:35: void AddOldTabNode(int64 sync_id,
size_t tab_node_id);
On 2013/06/06 21:27:39, Nicolas Zea wrote:
> I think this should be moved down to be next to the other "old tab" methods
Done.

[shashi, 2013-06-11 20:38:14]

I realized in our current design the size of free nodes can grow without bound,
(like in case of a bug or bad restarts) I have added a cap on number of free
nodes. Let me know what you think.

[Yaron, 2013-06-12 20:02:56]

android-y bits look fine. I assume Nicolas is looking at SMA and TabNodePool

[Yaron, 2013-06-12 20:03:04]

err they lgtm

[Nicolas Zea, 2013-06-19 21:35:33]

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/session_model_associator.cc (right):

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/session_model_associator.cc:216: // GetTabAt can return
a null tab, in case tab is null just skip it.
"tab; in that case just skip it."

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/session_model_associator.cc:221: // For tabs without
webcontents update the tab_id, tab_id could have
"For tabs without WebContents, update the |tab_id|, as it could have changed.."

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/session_model_associator.cc:223: if
(synced_tab->GetSyncId() > 0 &&
!= syncer::kInvalidId
Maybe worth checking whether tab_id is valid as well?

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/session_model_associator.cc:224:
UpdateTabIdIfNecessary(synced_tab->GetSyncId(), tab_id)) {
why is this part of the condition? Whether the tab id was already accurate or
not should not affect whether the tab is now used or not right?

Also, probably worth commenting that we can't check whether a tab is valid or
not if it has no webcontents, so we assume it's valid

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/session_model_associator.cc:320: CHECK(tab);
nit: I don't think this Check is necessary. We'd still null-ref crash two lines
lower without it.

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/session_model_associator.cc:360:
tab->SetSyncId(sync_id);
move this into the if block? It's redundant for successful reassociations right?

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/session_model_associator.cc:1187: if (old_tab_id !=
TabNodePool::kInvalidTabID && old_tab_id == new_tab_id) {
Why does it matter whether the tab id changed? Shouldn't the return value just
describe whether we succeeded in finding the sync id or not?

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/session_model_associator_unittest.cc (right):

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/session_model_associator_unittest.cc:195: 
remove newline

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/tab_node_pool.cc (right):

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.cc:40: DCHECK_GT(sync_id, 0);
DCHECK_GT(sync_id, syncer::kInvalidId);

also DCHECK_GT(tab_node_id, kInvalidTabId)

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.cc:52: if (it != free_nodes_pool_.end())
{
nit: don't handle DCHECK's. Remove the if statement and let this crash in the
case of a bug

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.cc:102: if (it !=
tab_syncid_tab_id_map_.end()) {
here too

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.cc:110: if
(free_nodes_pool_.find(sync_id) == free_nodes_pool_.end()) {
This also seems like it should just be DCHECK'd. Double free's are not permitted

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.cc:139: void
TabNodePool::FreeUnusedTabNodes(const std::set<int64>& used_sync_ids) {
Is this method still necessary? Can't the caller just directly call FreeTabNode
directly for all the sync ids?

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/tab_node_pool.h (right):

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.h:35: // then be used to associate with a
tab by calling AssociateTabNode.
Mention explicitly that the node is still considered free after this is called,
and will only be removed from the pool at Associate time.

That said, it seems risky to have repeated calls to this return the same value.
Perhaps we should just go ahead and remove the node from the pool at the time
this is called?

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.h:36: // Note: We make use of the
following "id's"
I think at this point this section on what the ids are should be moved to the
comment just above the class declaration (i.e. right above "class TabNodePool
{")

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.h:67: SessionID::id_type
TabIDForSyncID(int64 sync_id) const;
Rename to GetTabIdFromSyncId

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.h:70: // Returns true if it is able to
reassociate the node, false if sync node
"if a sync node with id |sync_id|"

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.h:78: void clear() {
nit: this was violating style guide before too, but this should now be named
Clear() and have the definition moved into the cc file. Same with the other
methods below (they're not simple getter/setters).

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.h:101:
FRIEND_TEST_ALL_PREFIXES(SyncTabNodePoolTest, TabNodeIdIncreases);
I prefer friending a test class, and then having the test class expose methods
for accessing the protected ones. That way you don't wind up with tons of
individually friended tests.

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.h:108: SyncIDToTabIDMap
tab_syncid_tab_id_map_;
I think it's implied that sync ids belong to tabs, so perhaps syncid_tabid_map_
is better

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.h:110: // The set of free nodes in the
free node pool.
"The sync ids for the set of free sync nodes" is a bit clearer I think

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/tab_node_pool_unittest.cc (right):

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool_unittest.cc:11: typedef testing::Test
SyncTabNodePoolTest;
Given what I mentioned about friending a test class, this should probably an
actual class inheriting from testing::Test that exposes the max used tab node
id. That also means you can leave the tests themselves within an anon namespace

[shashi, 2013-06-20 00:49:32]

PTAL

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/session_model_associator.cc (right):

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/session_model_associator.cc:216: // GetTabAt can return
a null tab, in case tab is null just skip it.
On 2013/06/19 21:35:33, Nicolas Zea wrote:
> "tab; in that case just skip it."

Done.

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/session_model_associator.cc:221: // For tabs without
webcontents update the tab_id, tab_id could have
On 2013/06/19 21:35:33, Nicolas Zea wrote:
> "For tabs without WebContents, update the |tab_id|, as it could have
changed.."

Done.

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/session_model_associator.cc:223: if
(synced_tab->GetSyncId() > 0 &&
I cannot check != syncer::kInvalidId, since TabContentsSyncedDelegate returns a
0 instead of syncer::kInvalidId and I cannot change it to return kInvalidId
because that violates a DEPS rule.
On 2013/06/19 21:35:33, Nicolas Zea wrote:
> != syncer::kInvalidId
> Maybe worth checking whether tab_id is valid as well?

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/session_model_associator.cc:224:
UpdateTabIdIfNecessary(synced_tab->GetSyncId(), tab_id)) {
Done, removed the condition. I was guarding against the case when
there is no sync node for an old tab, did not want to add it to
used_sync_nodes, but again we wouldn't have added the non existent node to tab
node pool in the first place.
On 2013/06/19 21:35:33, Nicolas Zea wrote:
> why is this part of the condition? Whether the tab id was already accurate or
> not should not affect whether the tab is now used or not right?
> 
> Also, probably worth commenting that we can't check whether a tab is valid or
> not if it has no webcontents, so we assume it's valid

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/session_model_associator.cc:320: CHECK(tab);
On 2013/06/19 21:35:33, Nicolas Zea wrote:
> nit: I don't think this Check is necessary. We'd still null-ref crash two
lines
> lower without it.

Done. Sometimes though, the stack trace of a crash can get messed up in case of
SIGSEGV but with a CHECK we always get a proper stack trace.

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/session_model_associator.cc:360:
tab->SetSyncId(sync_id);
On 2013/06/19 21:35:33, Nicolas Zea wrote:
> move this into the if block? It's redundant for successful reassociations
right?

Done. You are right, it is redundant for successful reassociations.

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/session_model_associator.cc:1187: if (old_tab_id !=
TabNodePool::kInvalidTabID && old_tab_id == new_tab_id) {
Done, in fact removed the return value.
On 2013/06/19 21:35:33, Nicolas Zea wrote:
> Why does it matter whether the tab id changed? Shouldn't the return value just
> describe whether we succeeded in finding the sync id or not?

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/session_model_associator_unittest.cc (right):

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/session_model_associator_unittest.cc:195: 
On 2013/06/19 21:35:33, Nicolas Zea wrote:
> remove newline

Done.

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/tab_node_pool.cc (right):

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.cc:40: DCHECK_GT(sync_id, 0);
On 2013/06/19 21:35:33, Nicolas Zea wrote:
> DCHECK_GT(sync_id, syncer::kInvalidId);
> 
> also DCHECK_GT(tab_node_id, kInvalidTabId)

Done.

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.cc:52: if (it != free_nodes_pool_.end())
{
On 2013/06/19 21:35:33, Nicolas Zea wrote:
> nit: don't handle DCHECK's. Remove the if statement and let this crash in the
> case of a bug

Done.

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.cc:102: if (it !=
tab_syncid_tab_id_map_.end()) {
On 2013/06/19 21:35:33, Nicolas Zea wrote:
> here too

Done.

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.cc:110: if
(free_nodes_pool_.find(sync_id) == free_nodes_pool_.end()) {
Done, moved the DCHECK about since this constraint should always be true.
On 2013/06/19 21:35:33, Nicolas Zea wrote:
> This also seems like it should just be DCHECK'd. Double free's are not
permitted

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.cc:139: void
TabNodePool::FreeUnusedTabNodes(const std::set<int64>& used_sync_ids) {
On 2013/06/19 21:35:33, Nicolas Zea wrote:
> Is this method still necessary? Can't the caller just directly call
FreeTabNode
> directly for all the sync ids?
This method is needed so that we delete nodes for which there are no tabs. E.g.
On a session restore n out of n+1 tabs are restored, 1 tab is lost (due to a
bug/ or the restore limit of 25 restored tabs) but all n+1 sync nodes of old
tabs exist. In this case we can no longer free the sync node for lost tab if we
do not check it in syncid_tabid_map_.

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/tab_node_pool.h (right):

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.h:35: // then be used to associate with a
tab by calling AssociateTabNode.
Updated the comment. AssociateTabNode does the actual association and requires
that the node to be associated should be free.
If we remove node from the pool, then we may have to put it in syncid_tabid_map_
and make sure we always reassociate the sync node with a tab node and do not end
up with any unused free nodes.

I have also added DCHECKs to guard against potential misuse.

On 2013/06/19 21:35:33, Nicolas Zea wrote:
> Mention explicitly that the node is still considered free after this is
called,
> and will only be removed from the pool at Associate time.
> 
> That said, it seems risky to have repeated calls to this return the same
value.
> Perhaps we should just go ahead and remove the node from the pool at the time
> this is called?

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.h:36: // Note: We make use of the
following "id's"
Done
On 2013/06/19 21:35:33, Nicolas Zea wrote:
> I think at this point this section on what the ids are should be moved to the
> comment just above the class declaration (i.e. right above "class TabNodePool
> {")

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.h:67: SessionID::id_type
TabIDForSyncID(int64 sync_id) const;
On 2013/06/19 21:35:33, Nicolas Zea wrote:
> Rename to GetTabIdFromSyncId

Done.

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.h:70: // Returns true if it is able to
reassociate the node, false if sync node
On 2013/06/19 21:35:33, Nicolas Zea wrote:
> "if a sync node with id |sync_id|"

Done.

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.h:78: void clear() {
On 2013/06/19 21:35:33, Nicolas Zea wrote:
> nit: this was violating style guide before too, but this should now be named
> Clear() and have the definition moved into the cc file. Same with the other
> methods below (they're not simple getter/setters).

Done.

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.h:101:
FRIEND_TEST_ALL_PREFIXES(SyncTabNodePoolTest, TabNodeIdIncreases);
Thanks, done, it is more cleaner now :).
On 2013/06/19 21:35:33, Nicolas Zea wrote:
> I prefer friending a test class, and then having the test class expose methods
> for accessing the protected ones. That way you don't wind up with tons of
> individually friended tests.

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.h:108: SyncIDToTabIDMap
tab_syncid_tab_id_map_;
On 2013/06/19 21:35:33, Nicolas Zea wrote:
> I think it's implied that sync ids belong to tabs, so perhaps
syncid_tabid_map_
> is better

Done.

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool.h:110: // The set of free nodes in the
free node pool.
On 2013/06/19 21:35:33, Nicolas Zea wrote:
> "The sync ids for the set of free sync nodes" is a bit clearer I think

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/tab_node_pool_unittest.cc (right):

https://codereview.chromium.org/16421003/diff/134001/chrome/browser/sync/glue...
chrome/browser/sync/glue/tab_node_pool_unittest.cc:11: typedef testing::Test
SyncTabNodePoolTest;
Done.
On 2013/06/19 21:35:33, Nicolas Zea wrote:
> Given what I mentioned about friending a test class, this should probably an
> actual class inheriting from testing::Test that exposes the max used tab node
> id. That also means you can leave the tests themselves within an anon
namespace

[shashi, 2013-06-22 00:09:03]

Gentle reminder.

[Nicolas Zea, 2013-06-22 00:28:05]

LGTM!

https://chromiumcodereview.appspot.com/16421003/diff/166001/chrome/browser/sy...
File chrome/browser/sync/glue/tab_node_pool_unittest.cc (right):

https://chromiumcodereview.appspot.com/16421003/diff/166001/chrome/browser/sy...
chrome/browser/sync/glue/tab_node_pool_unittest.cc:45: 
remove newline

[commit-bot: I haz the power, 2013-06-22 04:40:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shashishekhar@chromium.org/16421003/17...

[commit-bot: I haz the power, 2013-06-22 13:49:57]

Change committed as 208024