Reland v5 of Sessions Refactor

Reland v5 of Sessions Refactor

Changes from last review:
Added support for mapping a tab that had already been mapped to a different
window. This can happen in Android when a placeholder tab is created from
a previous window that hasn't yet closed. In addition, for testing/repro
purposes this patch makes the iteration order of windows deterministic (
by going through them in SessionID order).

Previous review: https://codereview.chromium.org/2683263002/

TEST=SessionsSyncMagerTest.PlaceholderConflictAcrossWindows
BUG=639009
Review-Url: https://codereview.chromium.org/2712743006
Cr-Commit-Position: refs/heads/master@{#453992}
Committed: https://chromium.googlesource.com/chromium/src/+/9b42f431d47b0e3a00b9435c719ce42f7c53c2c9

[Nicolas Zea, 2017-02-24 23:32:50]

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

[commit-bot: I haz the power, 2017-02-24 23:33:28]

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-02-24 23:46:08]

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

[commit-bot: I haz the power, 2017-02-24 23:46:09]

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

[Nicolas Zea, 2017-02-25 01:17:13]

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

[commit-bot: I haz the power, 2017-02-25 01:17:27]

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-02-25 01:22:38]

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

[Nicolas Zea, 2017-02-25 01:22:39]

+Sky for review, +Patrick FYI

The last two patchsets show the changes I've made from the original reland
(although that reland was modified significantly when I rebased onto the unit
test refactor)

[commit-bot: I haz the power, 2017-02-25 01:27:45]

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

[commit-bot: I haz the power, 2017-02-25 01:27:46]

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

[skym, 2017-02-27 18:22:38]

lgtm. Really only looked at difference between PS#2 and PS#4.

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
File components/sync_sessions/session_data_type_controller.cc (right):

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/session_data_type_controller.cc:49: for (auto
window_iter_pair : windows) {
const &?

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

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:110: const
google::protobuf::RepeatedField<int>& synced_tab_ids) {
Should this param be removed?

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
File components/sync_sessions/synced_session_tracker.cc (right):

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:271: // The tab has already
been mapped, possibly because of the sync id being
"sync id" being the "tab node id"? As nice as it would have been to our two id's
names not contain node, we should probably stick to consistent names. Maybe a
follow up rename CL could make this happen everywhere.

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:280: return tab.get() ==
tab_ptr;
I don't understand why it's safe here to assume that we have the more
significant piece of information right now, and we should replace/overwrite how
it was previously mapped as.

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:289: CHECK(tab) << " Unable
to find tab " << tab_id
CHECK should be temporary in this scenario, right? Either switch to DCHECK or
add crbug.com/????? if you want more data for investigating failures.

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:292: DVLOG(1) << "Moving tab
" << tab_id << " from window " << old_window_id
Why do this after the break? Why not immediately before the break? Would remove
the need for the temp variable.

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
File components/sync_sessions/synced_session_tracker_unittest.cc (right):

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker_unittest.cc:468: ASSERT_EQ(1U,
GetTabNodePool()->Capacity());
Don't do this in this CL, but these lines would be smaller if these accessors
were just pool() and tracker(), and I think they'd be equally clear.

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker_unittest.cc:485:
GetTracker()->CleanupLocalTabs(&free_node_ids);
Should deleted_node_ids.empty() be checked here as well?

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker_unittest.cc:502:
EXPECT_TRUE(GetTracker()->IsTabUnmappedForTesting(kTab2));
Shouldn't kTab1 be unmapped as well?

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker_unittest.cc:508:
EXPECT_FALSE(GetTracker()->IsTabUnmappedForTesting(kTab2));
..is kTab1 unmapped?

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker_unittest.cc:511: // GetSession
as well as the GetTab.
Did you mean to re-assign |session| via GetSession()? Or is the non-owned
SyncedSession object being updated a feature we want our tests to verify?

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker_unittest.cc:516: ASSERT_EQ(1U,
GetTabNodePool()->Capacity());
Do all these really need to be assert and not expect?

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker_unittest.cc:519:
ASSERT_NE(GetTracker()->GetTab(kTag, kTab1),
What about making sure that kTab1's tab node is isn't kTabNode as well?

[Nicolas Zea, 2017-02-28 00:08:22]

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

[Nicolas Zea, 2017-02-28 00:08:59]

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

[Nicolas Zea, 2017-02-28 00:09:02]

+Rohit for ios/ changes

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
File components/sync_sessions/session_data_type_controller.cc (right):

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/session_data_type_controller.cc:49: for (auto
window_iter_pair : windows) {
On 2017/02/27 18:22:38, skym wrote:
> const &?

Done.

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

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:110: const
google::protobuf::RepeatedField<int>& synced_tab_ids) {
On 2017/02/27 18:22:38, skym wrote:
> Should this param be removed?

Woops, yes it should. Done.

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
File components/sync_sessions/synced_session_tracker.cc (right):

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:271: // The tab has already
been mapped, possibly because of the sync id being
On 2017/02/27 18:22:38, skym wrote:
> "sync id" being the "tab node id"? As nice as it would have been to our two
id's
> names not contain node, we should probably stick to consistent names. Maybe a
> follow up rename CL could make this happen everywhere.

Yeah, good point. Switched to tab node id.

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:280: return tab.get() ==
tab_ptr;
On 2017/02/27 18:22:38, skym wrote:
> I don't understand why it's safe here to assume that we have the more
> significant piece of information right now, and we should replace/overwrite
how
> it was previously mapped as.

From my testing, the situations in which this can happen are transient, and in
part due to how the Chrome for Android code handles moving a tab between windows
(which appears to involve a period of time when the tab is duplicated). As such,
the goal is to handle this scenario gracefully, with the assumption that
eventually things will become consistent (with only one open tab being
associated with that sync id).

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:289: CHECK(tab) << " Unable
to find tab " << tab_id
On 2017/02/27 18:22:38, skym wrote:
> CHECK should be temporary in this scenario, right? Either switch to DCHECK or
> add crbug.com/????? if you want more data for investigating failures.

Yes, this should be temporary. Added the crbug.

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:292: DVLOG(1) << "Moving tab
" << tab_id << " from window " << old_window_id
On 2017/02/27 18:22:38, skym wrote:
> Why do this after the break? Why not immediately before the break? Would
remove
> the need for the temp variable.

Good point, done.

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
File components/sync_sessions/synced_session_tracker_unittest.cc (right):

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker_unittest.cc:468: ASSERT_EQ(1U,
GetTabNodePool()->Capacity());
On 2017/02/27 18:22:38, skym wrote:
> Don't do this in this CL, but these lines would be smaller if these accessors
> were just pool() and tracker(), and I think they'd be equally clear.

Acknowledged.

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker_unittest.cc:485:
GetTracker()->CleanupLocalTabs(&free_node_ids);
On 2017/02/27 18:22:38, skym wrote:
> Should deleted_node_ids.empty() be checked here as well?

Do you mean free_node_ids? Sure, done.

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker_unittest.cc:502:
EXPECT_TRUE(GetTracker()->IsTabUnmappedForTesting(kTab2));
On 2017/02/27 18:22:38, skym wrote:
> Shouldn't kTab1 be unmapped as well?

kTab1 doesn't exist at this point (the SessionTab object was reused for kTab2,
so it's no longer associated with anything). So it's neither mapped nor unmapped
(and IsTabUnmappedForTesting will actually return false).

Adding a LookupSessionTab check to verify that.

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker_unittest.cc:508:
EXPECT_FALSE(GetTracker()->IsTabUnmappedForTesting(kTab2));
On 2017/02/27 18:22:38, skym wrote:
> ..is kTab1 unmapped?

Same as above. Since it doesn't exist anymore, IsTabUnmapped would return false.

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker_unittest.cc:511: // GetSession
as well as the GetTab.
On 2017/02/27 18:22:38, skym wrote:
> Did you mean to re-assign |session| via GetSession()? Or is the non-owned
> SyncedSession object being updated a feature we want our tests to verify?

GetSession is meant to give you access to a Session object that will live
indefinitely, since the caller will actually be modifying the session object as
well. I'd say it's fine to rely on that behavior here.

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker_unittest.cc:516: ASSERT_EQ(1U,
GetTabNodePool()->Capacity());
On 2017/02/27 18:22:38, skym wrote:
> Do all these really need to be assert and not expect?

Done.

https://codereview.chromium.org/2712743006/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker_unittest.cc:519:
ASSERT_NE(GetTracker()->GetTab(kTag, kTab1),
On 2017/02/27 18:22:38, skym wrote:
> What about making sure that kTab1's tab node is isn't kTabNode as well?

See above. This GetTab method is actually creating a new placeholder SessionTab
that has no data, and isn't associated with any tab node.

Added a GetTabNodeFromLocalTabId to verify that.

[commit-bot: I haz the power, 2017-02-28 00:09:08]

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-02-28 00:22:23]

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

[commit-bot: I haz the power, 2017-02-28 00:22:24]

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...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Nicolas Zea, 2017-02-28 00:34:34]

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

[commit-bot: I haz the power, 2017-02-28 00:34:57]

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-02-28 01:53:31]

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

[commit-bot: I haz the power, 2017-02-28 01:53:32]

Dry run: This issue passed the CQ dry run.

[Nicolas Zea, 2017-03-01 00:55:51]

Friendly ping?

[rohitrao (ping after 24h), 2017-03-01 17:57:56]

ios lgtm

[Nicolas Zea, 2017-03-01 18:16:26]

The CQ bit was checked by zea@chromium.org

[Nicolas Zea, 2017-03-01 18:16:27]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org
Link to the patchset: https://codereview.chromium.org/2712743006/#ps120001
(title: "Fix typo")

[commit-bot: I haz the power, 2017-03-01 18:16:50]

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

[skym, 2017-03-01 18:18:24]

I thought this relied on the test changes that were reverted recently
https://codereview.chromium.org/2722953002/

[Nicolas Zea, 2017-03-01 18:20:04]

On 2017/03/01 18:18:24, skym wrote:
> I thought this relied on the test changes that were reverted recently
> https://codereview.chromium.org/2722953002/

Those were Patrick's integration test changes, not my unit test refactor.

[skym, 2017-03-01 18:20:27]

On 2017/03/01 18:18:24, skym wrote:
> I thought this relied on the test changes that were reverted recently
> https://codereview.chromium.org/2722953002/

Never mind! I was confused, disregard :)

[commit-bot: I haz the power, 2017-03-01 19:15:49]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488392186619630,
"parent_rev": "778f0744fecc3c50d4d23b0df97e3eaef9cbf817", "commit_rev":
"9b42f431d47b0e3a00b9435c719ce42f7c53c2c9"}

[commit-bot: I haz the power, 2017-03-01 19:16:33]

Description was changed from

==========
Reland v5 of Sessions Refactor

Changes from last review:
Added support for mapping a tab that had already been mapped to a different
window. This can happen in Android when a placeholder tab is created from
a previous window that hasn't yet closed. In addition, for testing/repro
purposes this patch makes the iteration order of windows deterministic (
by going through them in SessionID order).

Previous review: https://codereview.chromium.org/2683263002/

TEST=SessionsSyncMagerTest.PlaceholderConflictAcrossWindows
BUG=639009
==========

to

==========
Reland v5 of Sessions Refactor

Changes from last review:
Added support for mapping a tab that had already been mapped to a different
window. This can happen in Android when a placeholder tab is created from
a previous window that hasn't yet closed. In addition, for testing/repro
purposes this patch makes the iteration order of windows deterministic (
by going through them in SessionID order).

Previous review: https://codereview.chromium.org/2683263002/

TEST=SessionsSyncMagerTest.PlaceholderConflictAcrossWindows
BUG=639009

Review-Url: https://codereview.chromium.org/2712743006
Cr-Commit-Position: refs/heads/master@{#453992}
Committed:
https://chromium.googlesource.com/chromium/src/+/9b42f431d47b0e3a00b9435c719c...
==========

[commit-bot: I haz the power, 2017-03-01 19:16:35]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/9b42f431d47b0e3a00b9435c719c...