[Sync] Restore previous session if no tabbed windows present

[Sync] Restore previous session if no tabbed windows present

First, adds logic to support restoring the in-memory representation of the local
session based on the previous synced version of that session. This involves
updating rewriting SessionIds and updating the SyncedSessionTracker on each
startup.

Second, adds logic to include the previously synced session as part of
reassociation if the current session have at least one tabbed window. This
addresses the issue in Android where a custom tab can be opened without the
original Chrome for Android session being restored. We still want to sync the
custom tab's data, but don't want to lose the old session.

Lastly, this fixes an issue where the default value for Sync ids that are
persisted outside of sync (e.g. in the Android Tab state) were being initialized
to 0. 0 Is a valid sync id, so initialize them to -1.

BUG=639009
Review-Url: https://codereview.chromium.org/2791183003
Cr-Commit-Position: refs/heads/master@{#465156}
Committed: https://chromium.googlesource.com/chromium/src/+/92d4a108882a01897d144d810bc22bcce20e9b46

[Nicolas Zea, 2017-04-17 18:39:21]

The CQ bit was checked by zea@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 18:39:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nicolas Zea, 2017-04-17 18:42:03]

Description was changed from

==========
[Sync] Restore previous session if no tabbed windows present

Adds logic to restore a previously synced session if the current session
doesn't have at least one tabbed window. This addresses the issue in Android
where a custom tab can be opened without the original Chrome for Android
session being restored. We still want to sync the custom tab's data, but
don't want to lose the old session.

This introduces rewriting of session ids on restore though, which means that
all open tabs will have their id rewritten.

BUG=639009
==========

to

==========
[Sync] Restore previous session if no tabbed windows present

First, adds logic to support restoring the in-memory representation of the local
session based on the previous synced version of that session. This involves
updating rewriting SessionIds and updating the SyncedSessionTracker on each
startup.

Second, adds logic to include the previously synced session as part of
reassociation if the current session have at least one tabbed window. This
addresses the issue in Android where a custom tab can be opened without the
original Chrome for Android session being restored. We still want to sync the
custom tab's data, but don't want to lose the old session.

Lastly, this fixes an issue where the default value for Sync ids that are
persisted outside of sync (e.g. in the Android Tab state) were being initialized
to 0. 0 Is a valid sync id, so initialize them to -1.

BUG=639009
==========

[Nicolas Zea, 2017-04-17 18:44:55]

zea@chromium.org changed reviewers:
+ pnoland@chromium.org, skym@chromium.org

[Nicolas Zea, 2017-04-17 18:44:55]

PTAL!

[commit-bot: I haz the power, 2017-04-17 19:25:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-17 19:25:02]

Dry run: This issue passed the CQ dry run.

[skym, 2017-04-17 20:37:02]

lgtm

There is so much logic in sessions sync manager. :(

https://codereview.chromium.org/2791183003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/TabState.java (right):

https://codereview.chromium.org/2791183003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/TabState.java:259:
tabState.syncId = -1;
Can we redefine an invalid sync id in java land that we can point back to C++
constant via a comment?

https://codereview.chromium.org/2791183003/diff/40001/components/sync_session...
File components/sync_sessions/sessions_sync_manager.cc (right):

https://codereview.chromium.org/2791183003/diff/40001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:129: // Intentionally
fall-through
Why allow a fall through? Why not have a compile error when someone adds a new
type?

https://codereview.chromium.org/2791183003/diff/40001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:256: // the hood, e.g. during
ResetSessionTracking(..), so care must be taken
I don't understand what this means and how you're taking care in this function.

https://codereview.chromium.org/2791183003/diff/40001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:386: // Note: We don't bother
to set selected tab index anymore. We still
Seems like we should ideally be able to stop committing headers that have just a
selected index, like avoid including this field in the hash or something. Maybe
this could be a TODO instead until we have that ability?

https://codereview.chromium.org/2791183003/diff/40001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:763:
session_id_map[tab_ids->Get(i)] = SessionID().id();
I was really confused by this SessionID call until I looked up the code. I wish
SessionID::SessionID() was private and you had to go through a static function
like

private SessionID::SessionID(id_type id) : id_(id) {}

public static SessionID SessionID::GenerateUnique() {
  return SessionID(next_id++);
}

session_id_map[tab_ids->Get(i)] = SessionID::GenerateUnique().id();

https://codereview.chromium.org/2791183003/diff/40001/components/sync_session...
File components/sync_sessions/sessions_sync_manager.h (right):

https://codereview.chromium.org/2791183003/diff/40001/components/sync_session...
components/sync_sessions/sessions_sync_manager.h:250: // Updates a sync tab node
based on local tab information.
I don't like the term update being used here. Yes, you're using an
ACTION_UPDATE, but this is really preparing the sync change to be given to sync
for an existing tab. What do you think of AppendChangeForExistingTab().

https://codereview.chromium.org/2791183003/diff/40001/ios/chrome/browser/sync...
File ios/chrome/browser/sync/ios_chrome_synced_tab_delegate.mm (right):

https://codereview.chromium.org/2791183003/diff/40001/ios/chrome/browser/sync...
ios/chrome/browser/sync/ios_chrome_synced_tab_delegate.mm:39: :
web_state_(web_state), sync_session_id_(-1) {}
This should be able to ref the real invalid id constant, right?

[Nicolas Zea, 2017-04-17 21:13:10]

The CQ bit was checked by zea@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 21:13:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-17 21:22:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-17 21:22:06]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Nicolas Zea, 2017-04-17 21:23:19]

The CQ bit was checked by zea@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 21:23:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nicolas Zea, 2017-04-17 21:24:53]

zea@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Nicolas Zea, 2017-04-17 21:24:54]

Comments addressed. Ted, PTAL for Android changes

https://codereview.chromium.org/2791183003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/TabState.java (right):

https://codereview.chromium.org/2791183003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/TabState.java:259:
tabState.syncId = -1;
On 2017/04/17 20:37:02, skym wrote:
> Can we redefine an invalid sync id in java land that we can point back to C++
> constant via a comment?

Done.

https://codereview.chromium.org/2791183003/diff/40001/components/sync_session...
File components/sync_sessions/sessions_sync_manager.cc (right):

https://codereview.chromium.org/2791183003/diff/40001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:129: // Intentionally
fall-through
On 2017/04/17 20:37:02, skym wrote:
> Why allow a fall through? Why not have a compile error when someone adds a new
> type?

Good call, done.

https://codereview.chromium.org/2791183003/diff/40001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:256: // the hood, e.g. during
ResetSessionTracking(..), so care must be taken
On 2017/04/17 20:37:02, skym wrote:
> I don't understand what this means and how you're taking care in this
function.

Expanded the comment to clarify. ResetSessionTracking will e.g. delete tabs from
the SyncedSession object. As such, you shouldn't expect any data previously
within the SyncedSession object (like SessionTab pointers) to remain valid after
interactions with the SyncedSessionTracker.

https://codereview.chromium.org/2791183003/diff/40001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:386: // Note: We don't bother
to set selected tab index anymore. We still
On 2017/04/17 20:37:02, skym wrote:
> Seems like we should ideally be able to stop committing headers that have just
a
> selected index, like avoid including this field in the hash or something.
Maybe
> this could be a TODO instead until we have that ability?

This comment is out of date actually (was copied over). We're not doing anything
with selected tab index. We never commit it.

https://codereview.chromium.org/2791183003/diff/40001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:763:
session_id_map[tab_ids->Get(i)] = SessionID().id();
On 2017/04/17 20:37:02, skym wrote:
> I was really confused by this SessionID call until I looked up the code. I
wish
> SessionID::SessionID() was private and you had to go through a static function
> like
> 
> private SessionID::SessionID(id_type id) : id_(id) {}
> 
> public static SessionID SessionID::GenerateUnique() {
>   return SessionID(next_id++);
> }
> 
> session_id_map[tab_ids->Get(i)] = SessionID::GenerateUnique().id();

Yeah, I know what you mean. Added a comment to make clear the intent here.

https://codereview.chromium.org/2791183003/diff/40001/components/sync_session...
File components/sync_sessions/sessions_sync_manager.h (right):

https://codereview.chromium.org/2791183003/diff/40001/components/sync_session...
components/sync_sessions/sessions_sync_manager.h:250: // Updates a sync tab node
based on local tab information.
On 2017/04/17 20:37:02, skym wrote:
> I don't like the term update being used here. Yes, you're using an
> ACTION_UPDATE, but this is really preparing the sync change to be given to
sync
> for an existing tab. What do you think of AppendChangeForExistingTab().

Done.

https://codereview.chromium.org/2791183003/diff/40001/ios/chrome/browser/sync...
File ios/chrome/browser/sync/ios_chrome_synced_tab_delegate.mm (right):

https://codereview.chromium.org/2791183003/diff/40001/ios/chrome/browser/sync...
ios/chrome/browser/sync/ios_chrome_synced_tab_delegate.mm:39: :
web_state_(web_state), sync_session_id_(-1) {}
On 2017/04/17 20:37:02, skym wrote:
> This should be able to ref the real invalid id constant, right?

Done.

[commit-bot: I haz the power, 2017-04-17 22:09:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-17 22:09:55]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-04-18 04:53:49]

android lgtm

https://codereview.chromium.org/2791183003/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2791183003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:180: private
int mSyncId = -1;
should this be SyncConstants.INVALID_TAB_NODE_ID as well?

https://codereview.chromium.org/2791183003/diff/100001/chrome/browser/ui/sync...
File chrome/browser/ui/sync/tab_contents_synced_tab_delegate.cc (right):

https://codereview.chromium.org/2791183003/diff/100001/chrome/browser/ui/sync...
chrome/browser/ui/sync/tab_contents_synced_tab_delegate.cc:51: :
web_contents_(web_contents), sync_session_id_(-1) {}
should -1 be TabNodePool::kInvalidTabNodeID?

[Nicolas Zea, 2017-04-18 05:04:11]

Thanks! Comments addressed.

https://codereview.chromium.org/2791183003/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2791183003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:180: private
int mSyncId = -1;
On 2017/04/18 04:53:49, Ted C wrote:
> should this be SyncConstants.INVALID_TAB_NODE_ID as well?

Done.

https://codereview.chromium.org/2791183003/diff/100001/chrome/browser/ui/sync...
File chrome/browser/ui/sync/tab_contents_synced_tab_delegate.cc (right):

https://codereview.chromium.org/2791183003/diff/100001/chrome/browser/ui/sync...
chrome/browser/ui/sync/tab_contents_synced_tab_delegate.cc:51: :
web_contents_(web_contents), sync_session_id_(-1) {}
On 2017/04/18 04:53:49, Ted C wrote:
> should -1 be TabNodePool::kInvalidTabNodeID?

Done.

[Nicolas Zea, 2017-04-18 05:04:32]

The CQ bit was checked by zea@chromium.org

[Nicolas Zea, 2017-04-18 05:04:32]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org,
tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2791183003/#ps120001
(title: "Address missing TabNodeId references")

[commit-bot: I haz the power, 2017-04-18 05:04:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 05:26:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-18 05:26:51]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Nicolas Zea, 2017-04-18 05:38:59]

The CQ bit was checked by zea@chromium.org

[Nicolas Zea, 2017-04-18 05:39:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, skym@chromium.org
Link to the patchset: https://codereview.chromium.org/2791183003/#ps140001
(title: "Fix android compile")

[commit-bot: I haz the power, 2017-04-18 05:39:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 06:26:11]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492493939720250,
"parent_rev": "223e40da117757e1ddc9d4e5eb71d563f50254c9", "commit_rev":
"92d4a108882a01897d144d810bc22bcce20e9b46"}

[commit-bot: I haz the power, 2017-04-18 06:26:22]

Description was changed from

==========
[Sync] Restore previous session if no tabbed windows present

First, adds logic to support restoring the in-memory representation of the local
session based on the previous synced version of that session. This involves
updating rewriting SessionIds and updating the SyncedSessionTracker on each
startup.

Second, adds logic to include the previously synced session as part of
reassociation if the current session have at least one tabbed window. This
addresses the issue in Android where a custom tab can be opened without the
original Chrome for Android session being restored. We still want to sync the
custom tab's data, but don't want to lose the old session.

Lastly, this fixes an issue where the default value for Sync ids that are
persisted outside of sync (e.g. in the Android Tab state) were being initialized
to 0. 0 Is a valid sync id, so initialize them to -1.

BUG=639009
==========

to

==========
[Sync] Restore previous session if no tabbed windows present

First, adds logic to support restoring the in-memory representation of the local
session based on the previous synced version of that session. This involves
updating rewriting SessionIds and updating the SyncedSessionTracker on each
startup.

Second, adds logic to include the previously synced session as part of
reassociation if the current session have at least one tabbed window. This
addresses the issue in Android where a custom tab can be opened without the
original Chrome for Android session being restored. We still want to sync the
custom tab's data, but don't want to lose the old session.

Lastly, this fixes an issue where the default value for Sync ids that are
persisted outside of sync (e.g. in the Android Tab state) were being initialized
to 0. 0 Is a valid sync id, so initialize them to -1.

BUG=639009

Review-Url: https://codereview.chromium.org/2791183003
Cr-Commit-Position: refs/heads/master@{#465156}
Committed:
https://chromium.googlesource.com/chromium/src/+/92d4a108882a01897d144d810bc2...
==========

[commit-bot: I haz the power, 2017-04-18 06:26:23]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/92d4a108882a01897d144d810bc2...