Add a new SyncedTabDelegate for Android.

Add a new SyncedTabDelegate for Android.

Android can have tabs without any web contents. Tab association logic
assumes that all tabs have web contents. This prevents Android from
cleanly doing session reassociation. This change adds a
SyncedTabDelegate which does not depend on existence of web contents.
It also ensures that SessionID for a tab does not change when web
contents get attached to the tab.

BUG=139670, 125549
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203071

[shashi, 2013-05-21 01:32:14]

Full overview CL: https://chromiumcodereview.appspot.com/15055003/

chrome/browser/sync, chrome/browser/ui/sync: zea
chrome/browser/android, chrome/browser/ui/android: nyquist, yfriedman

chrome/browser/sessions, chrome/chrome.gypi:sky

[sky, 2013-05-21 04:53:01]

https://chromiumcodereview.appspot.com/15499005/diff/10001/chrome/browser/ses...
File chrome/browser/sessions/session_tab_helper.h (right):

https://chromiumcodereview.appspot.com/15499005/diff/10001/chrome/browser/ses...
chrome/browser/sessions/session_tab_helper.h:22: void SetSessionId(const
SessionID& id);
Why do you need to dynamically change the session id? Doing so could cause all
sorts of problems and I don't want to expose this.

[shashi, 2013-05-21 05:16:34]

https://chromiumcodereview.appspot.com/15499005/diff/10001/chrome/browser/ses...
File chrome/browser/sessions/session_tab_helper.h (right):

https://chromiumcodereview.appspot.com/15499005/diff/10001/chrome/browser/ses...
chrome/browser/sessions/session_tab_helper.h:22: void SetSessionId(const
SessionID& id);
Sorry my intent is not to dynamically change but to set it at instantiation
time. Now I realize it can be potentially misused, Perhaps a better way would be
to add a new static method:

static public void CreateForWebContents(WebContents * webcontents, const
SessionId& id) {
  CreateForWebContents(webcontents);
   FromWebContents(webcontents)->SetSessionId(id);
}
 
the new method SetSessionID is then private. This way we never expose this
method.

Background:
On Android we can have tab id assigned to tab even before web contents have been
created and all our tabs do not have web contents, (we do this to reduce memory
footprint of app, otherwise Chrome will be killed by operating system.) This
means if a user switches to a tab which was not in memory the SessionID
associated with it can change, since a lot of places use
SessionTabHelper::FromWebContents()->session_id for getting the tab id.

A lot of places in Chrome use SessionTabHelper to get SessionID.

On 2013/05/21 04:53:01, sky wrote:
> Why do you need to dynamically change the session id? Doing so could cause all
> sorts of problems and I don't want to expose this.

[sky, 2013-05-21 15:00:44]

https://chromiumcodereview.appspot.com/15499005/diff/10001/chrome/browser/ses...
File chrome/browser/sessions/session_tab_helper.h (right):

https://chromiumcodereview.appspot.com/15499005/diff/10001/chrome/browser/ses...
chrome/browser/sessions/session_tab_helper.h:22: void SetSessionId(const
SessionID& id);
On 2013/05/21 05:16:35, shashi wrote:
> Sorry my intent is not to dynamically change but to set it at instantiation
> time. Now I realize it can be potentially misused, Perhaps a better way would
be
> to add a new static method:
> 
> static public void CreateForWebContents(WebContents * webcontents, const
> SessionId& id) {
>   CreateForWebContents(webcontents);
>    FromWebContents(webcontents)->SetSessionId(id);
> }
>  
> the new method SetSessionID is then private. This way we never expose this
> method.
> 
> Background:
> On Android we can have tab id assigned to tab even before web contents have
been
> created and all our tabs do not have web contents, (we do this to reduce
memory
> footprint of app, otherwise Chrome will be killed by operating system.) This
> means if a user switches to a tab which was not in memory the SessionID
> associated with it can change, since a lot of places use
> SessionTabHelper::FromWebContents()->session_id for getting the tab id.
> 
> A lot of places in Chrome use SessionTabHelper to get SessionID.
> 
> On 2013/05/21 04:53:01, sky wrote:
> > Why do you need to dynamically change the session id? Doing so could cause
all
> > sorts of problems and I don't want to expose this.
> 

Is it possible to lookup the id you want to assign in SessionTabHelper's
constructor?

[shashi, 2013-05-21 16:01:13]

https://chromiumcodereview.appspot.com/15499005/diff/10001/chrome/browser/ses...
File chrome/browser/sessions/session_tab_helper.h (right):

https://chromiumcodereview.appspot.com/15499005/diff/10001/chrome/browser/ses...
chrome/browser/sessions/session_tab_helper.h:22: void SetSessionId(const
SessionID& id);
The id I want to assign is TabAndroid id, I don't think I can look it up in the
constructor, unless I pass the pointer to TabAndroid here.
On 2013/05/21 15:00:44, sky wrote:
> On 2013/05/21 05:16:35, shashi wrote:
> > Sorry my intent is not to dynamically change but to set it at instantiation
> > time. Now I realize it can be potentially misused, Perhaps a better way
would
> be
> > to add a new static method:
> > 
> > static public void CreateForWebContents(WebContents * webcontents, const
> > SessionId& id) {
> >   CreateForWebContents(webcontents);
> >    FromWebContents(webcontents)->SetSessionId(id);
> > }
> >  
> > the new method SetSessionID is then private. This way we never expose this
> > method.
> > 
> > Background:
> > On Android we can have tab id assigned to tab even before web contents have
> been
> > created and all our tabs do not have web contents, (we do this to reduce
> memory
> > footprint of app, otherwise Chrome will be killed by operating system.) This
> > means if a user switches to a tab which was not in memory the SessionID
> > associated with it can change, since a lot of places use
> > SessionTabHelper::FromWebContents()->session_id for getting the tab id.
> > 
> > A lot of places in Chrome use SessionTabHelper to get SessionID.
> > 
> > On 2013/05/21 04:53:01, sky wrote:
> > > Why do you need to dynamically change the session id? Doing so could cause
> all
> > > sorts of problems and I don't want to expose this.
> > 
> 
> Is it possible to lookup the id you want to assign in SessionTabHelper's
> constructor?

[sky, 2013-05-21 16:25:56]

Then I'm fine with the factory you mentioned.

On Tue, May 21, 2013 at 9:01 AM,  <shashishekhar@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/15499005/diff/10001/chrome/browser/ses...
> File chrome/browser/sessions/session_tab_helper.h (right):
>
>
https://chromiumcodereview.appspot.com/15499005/diff/10001/chrome/browser/ses...
> chrome/browser/sessions/session_tab_helper.h:22: void SetSessionId(const
> SessionID& id);
> The id I want to assign is TabAndroid id, I don't think I can look it up
> in the constructor, unless I pass the pointer to TabAndroid here.
>
> On 2013/05/21 15:00:44, sky wrote:
>>
>> On 2013/05/21 05:16:35, shashi wrote:
>> > Sorry my intent is not to dynamically change but to set it at
>
> instantiation
>>
>> > time. Now I realize it can be potentially misused, Perhaps a better
>
> way would
>>
>> be
>> > to add a new static method:
>> >
>> > static public void CreateForWebContents(WebContents * webcontents,
>
> const
>>
>> > SessionId& id) {
>> >   CreateForWebContents(webcontents);
>> >    FromWebContents(webcontents)->SetSessionId(id);
>> > }
>> >
>> > the new method SetSessionID is then private. This way we never
>
> expose this
>>
>> > method.
>> >
>> > Background:
>> > On Android we can have tab id assigned to tab even before web
>
> contents have
>>
>> been
>> > created and all our tabs do not have web contents, (we do this to
>
> reduce
>>
>> memory
>> > footprint of app, otherwise Chrome will be killed by operating
>
> system.) This
>>
>> > means if a user switches to a tab which was not in memory the
>
> SessionID
>>
>> > associated with it can change, since a lot of places use
>> > SessionTabHelper::FromWebContents()->session_id for getting the tab
>
> id.
>>
>> >
>> > A lot of places in Chrome use SessionTabHelper to get SessionID.
>> >
>> > On 2013/05/21 04:53:01, sky wrote:
>> > > Why do you need to dynamically change the session id? Doing so
>
> could cause
>>
>> all
>> > > sorts of problems and I don't want to expose this.
>> >
>
>
>> Is it possible to lookup the id you want to assign in
>
> SessionTabHelper's
>>
>> constructor?
>
>
> https://chromiumcodereview.appspot.com/15499005/

[Yaron, 2013-05-21 18:32:07]

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

https://codereview.chromium.org/15499005/diff/10001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/session_model_associator.cc:243: if (tab &&
tab->HasWebContents() && !AssociateTab(*tab, error)) {
I believe the existence-check for tab was purely for the webcontents-less case
on Android. Can't we always ensure that we have a tab now?

https://codereview.chromium.org/15499005/diff/10001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/synced_tab_delegate_android.cc (right):

https://codereview.chromium.org/15499005/diff/10001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/synced_tab_delegate_android.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
2013 and no "(c)"

https://codereview.chromium.org/15499005/diff/10001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/synced_tab_delegate_android.cc:27: return
TabContentsSyncedTabDelegate::FromWebContents(web_contents_)
So it's up to caller to ensure not to call these if there's no web_contents?

https://codereview.chromium.org/15499005/diff/10001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/synced_tab_delegate_android.h (right):

https://codereview.chromium.org/15499005/diff/10001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/synced_tab_delegate_android.h:17: // On Android a tab
can exist even without web_contents.
Nit; missing newline between

https://codereview.chromium.org/15499005/diff/10001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/synced_window_delegate_android.cc (right):

https://codereview.chromium.org/15499005/diff/10001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/synced_window_delegate_android.cc:84: return tab ?
tab->GetSessionId() : -1;
Can this be null? Don't we always have an android tab?

[shashi, 2013-05-21 23:11:21]

https://chromiumcodereview.appspot.com/15499005/diff/10001/chrome/browser/ses...
File chrome/browser/sessions/session_tab_helper.h (right):

https://chromiumcodereview.appspot.com/15499005/diff/10001/chrome/browser/ses...
chrome/browser/sessions/session_tab_helper.h:22: void SetSessionId(const
SessionID& id);
On 2013/05/21 16:01:13, shashi wrote:
> The id I want to assign is TabAndroid id, I don't think I can look it up in
the
> constructor, unless I pass the pointer to TabAndroid here.
> On 2013/05/21 15:00:44, sky wrote:
> > On 2013/05/21 05:16:35, shashi wrote:
> > > Sorry my intent is not to dynamically change but to set it at
instantiation
> > > time. Now I realize it can be potentially misused, Perhaps a better way
> would
> > be
> > > to add a new static method:
> > > 
> > > static public void CreateForWebContents(WebContents * webcontents, const
> > > SessionId& id) {
> > >   CreateForWebContents(webcontents);
> > >    FromWebContents(webcontents)->SetSessionId(id);
> > > }
> > >  
> > > the new method SetSessionID is then private. This way we never expose this
> > > method.
> > > 
> > > Background:
> > > On Android we can have tab id assigned to tab even before web contents
have
> > been
> > > created and all our tabs do not have web contents, (we do this to reduce
> > memory
> > > footprint of app, otherwise Chrome will be killed by operating system.)
This
> > > means if a user switches to a tab which was not in memory the SessionID
> > > associated with it can change, since a lot of places use
> > > SessionTabHelper::FromWebContents()->session_id for getting the tab id.
> > > 
> > > A lot of places in Chrome use SessionTabHelper to get SessionID.
> > > 
> > > On 2013/05/21 04:53:01, sky wrote:
> > > > Why do you need to dynamically change the session id? Doing so could
cause
> > all
> > > > sorts of problems and I don't want to expose this.
> > > 
> > 
> > Is it possible to lookup the id you want to assign in SessionTabHelper's
> > constructor?
> 

Done.

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

https://chromiumcodereview.appspot.com/15499005/diff/10001/chrome/browser/syn...
chrome/browser/sync/glue/session_model_associator.cc:243: if (tab &&
tab->HasWebContents() && !AssociateTab(*tab, error)) {
On 2013/05/21 18:32:07, Yaron wrote:
> I believe the existence-check for tab was purely for the webcontents-less case
> on Android. Can't we always ensure that we have a tab now? 

Done.

https://chromiumcodereview.appspot.com/15499005/diff/10001/chrome/browser/syn...
File chrome/browser/sync/glue/synced_tab_delegate_android.cc (right):

https://chromiumcodereview.appspot.com/15499005/diff/10001/chrome/browser/syn...
chrome/browser/sync/glue/synced_tab_delegate_android.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2013/05/21 18:32:07, Yaron wrote:
> 2013 and no "(c)"

Done.

https://chromiumcodereview.appspot.com/15499005/diff/10001/chrome/browser/syn...
chrome/browser/sync/glue/synced_tab_delegate_android.cc:27: return
TabContentsSyncedTabDelegate::FromWebContents(web_contents_)
Yes otherwise there will be a DCHECK failure in FromWebContents.
We only override, the methods for GetSessionId (and sync id methods to be added
later.) We may want to change that later when necessary.

On 2013/05/21 18:32:07, Yaron wrote:
> So it's up to caller to ensure not to call these if there's no web_contents?

https://chromiumcodereview.appspot.com/15499005/diff/10001/chrome/browser/syn...
File chrome/browser/sync/glue/synced_tab_delegate_android.h (right):

https://chromiumcodereview.appspot.com/15499005/diff/10001/chrome/browser/syn...
chrome/browser/sync/glue/synced_tab_delegate_android.h:17: // On Android a tab
can exist even without web_contents.
On 2013/05/21 18:32:07, Yaron wrote:
> Nit; missing newline between

Done.

https://chromiumcodereview.appspot.com/15499005/diff/10001/chrome/browser/syn...
File chrome/browser/sync/glue/synced_window_delegate_android.cc (right):

https://chromiumcodereview.appspot.com/15499005/diff/10001/chrome/browser/syn...
chrome/browser/sync/glue/synced_window_delegate_android.cc:84: return tab ?
tab->GetSessionId() : -1;
This should not be null, but TabModelImpl uses GetNativeTab for finding native
tab which can return NULL.
I am going to add a CHECK and say this cannot be NULL in TabModelImpl downstream
and remove this check.
On 2013/05/21 18:32:07, Yaron wrote:
> Can this be null? Don't we always have an android tab?

[sky, 2013-05-21 23:49:44]

https://chromiumcodereview.appspot.com/15499005/diff/54001/chrome/browser/ses...
File chrome/browser/sessions/session_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/15499005/diff/54001/chrome/browser/ses...
chrome/browser/sessions/session_tab_helper.cc:27:
FromWebContents(contents)->session_id_ = id;
Could you add a new constructor that this code uses so that session_id_ can
remain const? You can do something like:

contents->SetUserData(new SessionTabHelper(contents, id));

[shashi, 2013-05-22 00:34:32]

https://chromiumcodereview.appspot.com/15499005/diff/54001/chrome/browser/ses...
File chrome/browser/sessions/session_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/15499005/diff/54001/chrome/browser/ses...
chrome/browser/sessions/session_tab_helper.cc:27:
FromWebContents(contents)->session_id_ = id;
On 2013/05/21 23:49:44, sky wrote:
> Could you add a new constructor that this code uses so that session_id_ can
> remain const? You can do something like:
> 
> contents->SetUserData(new SessionTabHelper(contents, id));

Done.

[sky, 2013-05-22 00:46:36]

LGTM

https://chromiumcodereview.appspot.com/15499005/diff/66001/chrome/browser/ses...
File chrome/browser/sessions/session_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/15499005/diff/66001/chrome/browser/ses...
chrome/browser/sessions/session_tab_helper.cc:24: :
content::WebContentsObserver(contents), session_id_(id) {}
nit: since the constructor wrapped one param on each line.

[Nicolas Zea, 2013-05-22 20:36:37]

mainly nits

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

https://chromiumcodereview.appspot.com/15499005/diff/76001/chrome/browser/and...
chrome/browser/android/tab_android.h:81: jobject content_view);
nit: newline after

https://chromiumcodereview.appspot.com/15499005/diff/76001/chrome/browser/ses...
File chrome/browser/sessions/session_tab_helper.h (right):

https://chromiumcodereview.appspot.com/15499005/diff/76001/chrome/browser/ses...
chrome/browser/sessions/session_tab_helper.h:21: static void
CreateForWebContentsWithId(content::WebContents* contents,
nit: contents can be modified, and is therefore an output, so should be ordered
after id (here and below)

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

https://chromiumcodereview.appspot.com/15499005/diff/76001/chrome/browser/syn...
chrome/browser/sync/glue/session_model_associator.cc:243: if
(tab->HasWebContents() && !AssociateTab(*tab, error)) {
can GetTabAt not return NULL anymore? Perhaps (tab && tab->HasWebContents() &&
...

https://chromiumcodereview.appspot.com/15499005/diff/76001/chrome/browser/syn...
File chrome/browser/sync/glue/synced_tab_delegate_android.h (right):

https://chromiumcodereview.appspot.com/15499005/diff/76001/chrome/browser/syn...
chrome/browser/sync/glue/synced_tab_delegate_android.h:45: explicit
SyncedTabDelegateAndroid(TabAndroid* owning_tab_);
nit: move to top of list of public methods.

[shashi, 2013-05-22 21:18:59]

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

https://chromiumcodereview.appspot.com/15499005/diff/76001/chrome/browser/and...
chrome/browser/android/tab_android.h:81: jobject content_view);
On 2013/05/22 20:36:37, Nicolas Zea wrote:
> nit: newline after

Done.

https://chromiumcodereview.appspot.com/15499005/diff/76001/chrome/browser/ses...
File chrome/browser/sessions/session_tab_helper.h (right):

https://chromiumcodereview.appspot.com/15499005/diff/76001/chrome/browser/ses...
chrome/browser/sessions/session_tab_helper.h:21: static void
CreateForWebContentsWithId(content::WebContents* contents,
In this case contents will not be modified, if there is a different contents
then we will have a new instance of SessionTabHelper.
On 2013/05/22 20:36:37, Nicolas Zea wrote:
> nit: contents can be modified, and is therefore an output, so should be
ordered
> after id (here and below)

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

https://chromiumcodereview.appspot.com/15499005/diff/76001/chrome/browser/syn...
chrome/browser/sync/glue/session_model_associator.cc:243: if
(tab->HasWebContents() && !AssociateTab(*tab, error)) {
Yes, my downstream patch makes sure that it will no longer null, Yaron suggested
to remove the null check in the comment on previous patch. I will land my
downstream change as soon as this gets rolled in.
On 2013/05/22 20:36:37, Nicolas Zea wrote:
> can GetTabAt not return NULL anymore? Perhaps (tab && tab->HasWebContents() &&
> ...

https://chromiumcodereview.appspot.com/15499005/diff/76001/chrome/browser/syn...
File chrome/browser/sync/glue/synced_tab_delegate_android.h (right):

https://chromiumcodereview.appspot.com/15499005/diff/76001/chrome/browser/syn...
chrome/browser/sync/glue/synced_tab_delegate_android.h:45: explicit
SyncedTabDelegateAndroid(TabAndroid* owning_tab_);
On 2013/05/22 20:36:37, Nicolas Zea wrote:
> nit: move to top of list of public methods.

Done.

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

https://chromiumcodereview.appspot.com/15499005/diff/76001/chrome/browser/ses...
File chrome/browser/sessions/session_tab_helper.h (right):

https://chromiumcodereview.appspot.com/15499005/diff/76001/chrome/browser/ses...
chrome/browser/sessions/session_tab_helper.h:21: static void
CreateForWebContentsWithId(content::WebContents* contents,
On 2013/05/22 21:18:59, shashi wrote:
> In this case contents will not be modified, if there is a different contents
> then we will have a new instance of SessionTabHelper.
> On 2013/05/22 20:36:37, Nicolas Zea wrote:
> > nit: contents can be modified, and is therefore an output, so should be
> ordered
> > after id (here and below)
> 

By modified, I include any mutable methods, which I think SetUserData is.
Otherwise it'd be good if contents were passed as a const.

[shashi, 2013-05-22 22:30:47]

https://chromiumcodereview.appspot.com/15499005/diff/76001/chrome/browser/ses...
File chrome/browser/sessions/session_tab_helper.h (right):

https://chromiumcodereview.appspot.com/15499005/diff/76001/chrome/browser/ses...
chrome/browser/sessions/session_tab_helper.h:21: static void
CreateForWebContentsWithId(content::WebContents* contents,
On 2013/05/22 22:06:06, Nicolas Zea wrote:
> On 2013/05/22 21:18:59, shashi wrote:
> > In this case contents will not be modified, if there is a different contents
> > then we will have a new instance of SessionTabHelper.
> > On 2013/05/22 20:36:37, Nicolas Zea wrote:
> > > nit: contents can be modified, and is therefore an output, so should be
> > ordered
> > > after id (here and below)
> > 
> 
> By modified, I include any mutable methods, which I think SetUserData is.
> Otherwise it'd be good if contents were passed as a const.

Ah, I misunderstood, makes sense. Done.

[Nicolas Zea, 2013-05-22 22:33:43]

lgtm!

[Yaron, 2013-05-23 18:59:28]

lgtm

[commit-bot: I haz the power, 2013-05-24 00:19:04]

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

[commit-bot: I haz the power, 2013-05-24 06:28:23]

Retried try job too often on win_rel for step(s) chrome_frame_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-05-28 16:04:05]

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

[commit-bot: I haz the power, 2013-05-28 19:17:06]

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

[commit-bot: I haz the power, 2013-05-28 19:27:35]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[shashi, 2013-05-28 22:55:06]

yfriedman: based on feedback on downstream patch, I have added a small change
for fixing  prerendering and tab ids on Android in tab_android.cc.

[shashi, 2013-05-29 19:21:47]

I have to revert my changes to SessionTabHelper, since it will break
prerendering. Now tab id of a tab will change if web contents gets attached to
it. Sync will be notified of changes to tab id. In fact in case of reattach
since old web contents of the tab will be destroyed which itself will fire
NOTIFICATION_WEB_CONTENTS_DESTROYED and trigger reassociation.

Unfortunately there does not seem to be an easy way to maintain a consistent tab
id when tab is in or not out of memory.

[Yaron, 2013-05-30 00:47:22]

lgtm assuming my comment isn't an issue

https://chromiumcodereview.appspot.com/15499005/diff/169001/chrome/browser/sy...
File chrome/browser/sync/glue/session_model_associator.cc (right):

https://chromiumcodereview.appspot.com/15499005/diff/169001/chrome/browser/sy...
chrome/browser/sync/glue/session_model_associator.cc:226: // If the tab is
valid, it would have been added to the tracker either
Does this logic still work with the changing tab id? I'm concerned that if we
change the tab id and the sessionchangeprocessor doesn't get any navigation
(cause tab is gone) then we're in trouble. That said, this happens when the tab
goes to background so maybe it's ok?

[shashi, 2013-05-30 00:56:13]

https://chromiumcodereview.appspot.com/15499005/diff/169001/chrome/browser/sy...
File chrome/browser/sync/glue/session_model_associator.cc (right):

https://chromiumcodereview.appspot.com/15499005/diff/169001/chrome/browser/sy...
chrome/browser/sync/glue/session_model_associator.cc:226: // If the tab is
valid, it would have been added to the tracker either
Discussed offline, this should work similar to Desktop, on prerender web
contents are swapped and destruction of old web contents should trigger
reassociation.

On 2013/05/30 00:47:23, Yaron wrote:
> Does this logic still work with the changing tab id? I'm concerned that if we
> change the tab id and the sessionchangeprocessor doesn't get any navigation
> (cause tab is gone) then we're in trouble. That said, this happens when the
tab
> goes to background so maybe it's ok?

[commit-bot: I haz the power, 2013-05-30 00:56:35]

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

[commit-bot: I haz the power, 2013-05-30 04:51:07]

Change committed as 203071