Do not submit: high level overview patch.

Do not submit: high level overview patch.

Uploaded for high level guidance. My plan is to split the patch
into multiple patches and land each with some test coverage.
This patch only shows part of changes to the existing test coverage.

Details of changes:
1. Persist sync id with tab.
2. Store sync id in SessionTabHelper.
3. On Android side, make tab id for Java and native tabs same.(part of
this is in a separate downstream patch, I might move the code to
tab_android so it is reviewable upstream).
4. Change tab node pool and session association logic.

BUG=139666, 139670, 125549

[shashi, 2013-05-13 23:47:29]

Corresponding downstream patch:
https://gerrit-int.chromium.org/#/c/37832/

[Nicolas Zea, 2013-05-16 22:59:02]

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

https://codereview.chromium.org/15055003/diff/16001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:253:
UpdateTabIdForOldTab(tab_delegate->GetSyncSessionId(), tab_id);
I don't think you should ever update the tab id. That's just sync's id for the
current tab node count at the time it was created. Multiple tabs, over the life
of a sync session, can use the tab sync node, but the tab id for that sync node
should never change. Perhaps I'm misunderstanding something?

https://codereview.chromium.org/15055003/diff/16001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:389:
NotifySyncIdGenerated(tab);
would it be better to have the equivalency check and the notify call part of the
SetSyncSessionId method?

https://codereview.chromium.org/15055003/diff/16001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:807:
tab_pool_.AddTabNode(id, specifics.tab_node_id());
else tombstone? (it would be the above error case where more than one session
header was found or the node was just corrupted somehow)

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

https://codereview.chromium.org/15055003/diff/16001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/tab_node_pool.cc:73: tab_node.Tombstone();
shouldn't these nodes be added to the tab_syncid_pool? Otherwise you'll
constantly be creating new nodes with ever increasing ids, and losing the ids of
old nodes right?

[shashi, 2013-05-17 00:29:15]

Nicolas, thanks a lot for comments, I will incorporate some of the feedback when
I start uploading patches.

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

https://codereview.chromium.org/15055003/diff/16001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:253:
UpdateTabIdForOldTab(tab_delegate->GetSyncSessionId(), tab_id);
When restore happens, the tab ids (different from tab_node_id) are
reinitialized.
I discovered this during prototyping, easiest way to explain this is by an
example:
Lets say before restore I had 3 tabs (A,B,C) with tab ids (1, 2, 3) after
restore the tab ids may be (20, 30, 10).
Suppose only tab A is in memory.
After tab association, the window_s will have tabs:
(2, 30, 10). We will rewrite the session node for A. 
So A will have tab_id=20 but since B and C are not in memory the session nodes
for them will be unchanged and will have 2 and 3 tab ids.

Remote client will see a window with tab ids: (20, 30, 10) but will not able to
make out the tab nodes corresponding to these tab ids.


On 2013/05/16 22:59:03, Nicolas Zea wrote:
> I don't think you should ever update the tab id. That's just sync's id for the
> current tab node count at the time it was created. Multiple tabs, over the
life
> of a sync session, can use the tab sync node, but the tab id for that sync
node
> should never change. Perhaps I'm misunderstanding something?

https://codereview.chromium.org/15055003/diff/16001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:389:
NotifySyncIdGenerated(tab);
On 2013/05/16 22:59:03, Nicolas Zea wrote:
> would it be better to have the equivalency check and the notify call part of
the
> SetSyncSessionId method?

https://codereview.chromium.org/15055003/diff/16001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:389:
NotifySyncIdGenerated(tab);
Yes, I can replace it with an equivalency check, any maybe generate notification
directly from SetSyncSessionId.

On 2013/05/16 22:59:03, Nicolas Zea wrote:
> would it be better to have the equivalency check and the notify call part of
the
> SetSyncSessionId method?

https://codereview.chromium.org/15055003/diff/16001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:807:
tab_pool_.AddTabNode(id, specifics.tab_node_id());
Good point, I should  handle the case for multiple header nodes/corrupted nodes.
Will fix this.
On 2013/05/16 22:59:03, Nicolas Zea wrote:
> else tombstone? (it would be the above error case where more than one session
> header was found or the node was just corrupted somehow)

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

https://codereview.chromium.org/15055003/diff/16001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/tab_node_pool.cc:73: tab_node.Tombstone();
Yes, they should be returned to the free tab pool., this is a remnant from my
prototype. I was debugging and found it easier to just delete the sync node.
Btw should we limit the number of free nodes. 
Can there be scenarios like crashes, when we lose tabs and keep adding back to
the free tab pool. Should we limit adding to free tab pool, if we have say 25
free tab nodes?

On 2013/05/16 22:59:03, Nicolas Zea wrote:
> shouldn't these nodes be added to the tab_syncid_pool? Otherwise you'll
> constantly be creating new nodes with ever increasing ids, and losing the ids
of
> old nodes right?