Reland v3 of Session refactor

Reland v3 of Session refactor

Original CL review: https://codereview.chromium.org/2494533002

This relands that CL, with a couple key fixes:
- Detect if a previously synced tab no longer exists, and ignore it if so
- Simplifies the logic to add tab ids to a window to avoid possibly out-of-bounds
- Adds checking for invalid tab node ids
- Simplifies the logic that either calls AssociateTab or AssociateRestoredPlaceholderTab
- Adds some CHECKs to help detect remaining issues.

BUG=639009, 673618
Review-Url: https://codereview.chromium.org/2651583006
Cr-Commit-Position: refs/heads/master@{#446405}
Committed: https://chromium.googlesource.com/chromium/src/+/0b0a0a13587273010f58db836c86c0d4748e9e5b

[Nicolas Zea, 2017-01-24 00:35:55]

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

[commit-bot: I haz the power, 2017-01-24 00:36:42]

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-01-24 01:27:38]

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

[commit-bot: I haz the power, 2017-01-24 01:27:39]

Dry run: This issue passed the CQ dry run.

[Nicolas Zea, 2017-01-24 17:42:51]

Description was changed from

==========
Reland v3 of Session refactor

Original CL review: https://codereview.chromium.org/2494533002

This relands that CL, with several fixes

BUG=639009, 673618
==========

to

==========
Reland v3 of Session refactor

Original CL review: https://codereview.chromium.org/2494533002

This relands that CL, with a couple key fixes:
- Detect if a previously synced tab no longer exists, and ignore it if so
- Simplifies the logic to add tab ids to a window to avoid possibly
out-of-bounds
- Adds checking for invalid tab node ids
- Simplifies the logic that either calls AssociateTab or
AssociateRestoredPlaceholderTab
- Adds some CHECKs to help detect remaining issues.

BUG=639009, 673618
==========

[Nicolas Zea, 2017-01-24 17:43:49]

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

[Nicolas Zea, 2017-01-24 17:43:50]

PTAL

[skym, 2017-01-24 20:46:19]

lgtm

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

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:95: if (tab_id ==
TabNodePool::kInvalidTabID)
This looks unit testable.

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:355: if (tab_node_id <=
TabNodePool::kInvalidTabNodeID)
Why is this condition using <= when everything else is checking equality?

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:374: if (old_tab_id !=
TabNodePool::kInvalidTabID &&
There should never be an entry for kInvalidTabID though, right? Should this just
be a CHECK?

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
File components/sync_sessions/synced_session_tracker.h (right):

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.h:169: bool
IsLocalTabNodeValid(int tab_node_id);
"Valid" is kind of confusing. Why is it invalid if it hasn't been seen? Our
kInvalidXxId constants use a different definition of valid, it would seem. Maybe
one of seen/stale/fresh would be better?

Also, should this thing get unit tests?

[Nicolas Zea, 2017-01-24 20:46:20]

On 2017/01/24 17:43:50, Nicolas Zea wrote:
> PTAL

FYI I forgot to mention, patchset 1 is just the original patch that was
reverted. Patchset 2 contains the actual fixes that differ from the original
patch.

[Patrick Noland, 2017-01-25 19:23:41]

https://codereview.chromium.org/2651583006/diff/30010/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc (right):

https://codereview.chromium.org/2651583006/diff/30010/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:1094: // There
should be two changes, one for the fully associated tab, and
Update to say "three changes"

https://codereview.chromium.org/2651583006/diff/30010/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:1132: const char
kFoo1[] = "http://foo1/";
You've got the same two constants in SwappedOutOnRestore, any reason not to make
them top level?

https://codereview.chromium.org/2651583006/diff/30010/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:1614:
ASSERT_EQ(3U, out.size());
Is there a way to make the verification of the "shape" of the output more
generic/declarative? There are a lot of similar lines checking that an item in
index i is and ADD and that it contains a header or a tab.

https://codereview.chromium.org/2651583006/diff/30010/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:2039: for (int i
= 0; i < 10; i++) {
Can you make 5 and 10 variables with names? Tracking the meaning throughout the
function is tricky

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

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:290: if
(synced_tab->GetSyncId() > TabNodePool::kInvalidTabNodeID &&
Not part of the current change, but this would be clearer if there was a helper
function whose name describes the purpose of this check

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:324: for (int tab_node_id :
deleted_tab_node_ids) {
Might be worth making a helper to delete a list of tab node ids, since it's now
done in two different places

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

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:255: // in the system, so we
permit ourselves to use kInvalidTabNodeID here and
Is the comment still applicable now that you're not passing
TabNodePool::kInvalidTabNodeID ?

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:282: sessions::SessionTab*
SyncedSessionTracker::GetTab(
This function does two things: it creates a SessionTab for the given session_tag
and tab_id if one doesn't exist, and it returns the SessionTab for the tab_id.
Is there a good way to rename this or else  refactor it to functions that only
do one thing? e.g. AllocateSessionTab(), LookupSessionTab(). This would mean you
don't have to do confusing things like ignore the result of it.

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:286: sessions::SessionTab*
tab_ptr = nullptr;
Can this use LookupSessionTab ?

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:336: bool
SyncedSessionTracker::GetTabNodeForLocalTab(int tab_id, int* tab_node_id) {
Given that GetTab refers to a SessionTab* and this refers to a tab_id, it would
be clearer to name this "GetTabNodeForLocalTabId". Or else refer to SessionTabs
by their full name elsewhere

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:349:
DCHECK_NE(TabNodePool::kInvalidTabNodeID, *tab_node_id);
Why DCHECK here and CHECK at the call site?

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:362: SessionID::id_type
new_tab_id) {
I'm confused by the usage of both SessionID::id_type and int as the type for a
tab_id. Could you make it consistent?

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:368:
local_tab_pool_.GetTabIdFromTabNodeId(tab_node_id);
I'm also confused by some functions using the term "TabNode" and others using
"TabNodeId" although this issue obviously predates this change

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

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker_unittest.cc:424: const int
kWindow1 = 1;
You use these constants in the previous function as well

[Nicolas Zea, 2017-01-25 23:55:05]

PTAL

https://codereview.chromium.org/2651583006/diff/30010/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc (right):

https://codereview.chromium.org/2651583006/diff/30010/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:1094: // There
should be two changes, one for the fully associated tab, and
On 2017/01/25 19:23:41, Patrick Noland wrote:
> Update to say "three changes"

Done.

https://codereview.chromium.org/2651583006/diff/30010/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:1132: const char
kFoo1[] = "http://foo1/";
On 2017/01/25 19:23:41, Patrick Noland wrote:
> You've got the same two constants in SwappedOutOnRestore, any reason not to
make
> them top level?

Done.

https://codereview.chromium.org/2651583006/diff/30010/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:1614:
ASSERT_EQ(3U, out.size());
On 2017/01/25 19:23:41, Patrick Noland wrote:
> Is there a way to make the verification of the "shape" of the output more
> generic/declarative? There are a lot of similar lines checking that an item in
> index i is and ADD and that it contains a header or a tab. 

I think that's a fair point, although something I'd like to postpone to a more
thorough cleanup patch.

https://codereview.chromium.org/2651583006/diff/30010/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc:2039: for (int i
= 0; i < 10; i++) {
On 2017/01/25 19:23:41, Patrick Noland wrote:
> Can you make 5 and 10 variables with names? Tracking the meaning throughout
the
> function is tricky

Done.

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

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:290: if
(synced_tab->GetSyncId() > TabNodePool::kInvalidTabNodeID &&
On 2017/01/25 19:23:41, Patrick Noland wrote:
> Not part of the current change, but this would be clearer if there was a
helper
> function whose name describes the purpose of this check

This is just checking that the tab and tab node ids are valid. Did that not seem
clear?

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:324: for (int tab_node_id :
deleted_tab_node_ids) {
On 2017/01/25 19:23:41, Patrick Noland wrote:
> Might be worth making a helper to delete a list of tab node ids, since it's
now
> done in two different places

Done.

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

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:95: if (tab_id ==
TabNodePool::kInvalidTabID)
On 2017/01/24 20:46:19, skym wrote:
> This looks unit testable.

Done.

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:255: // in the system, so we
permit ourselves to use kInvalidTabNodeID here and
On 2017/01/25 19:23:41, Patrick Noland wrote:
> Is the comment still applicable now that you're not passing
> TabNodePool::kInvalidTabNodeID ?

Reworded to be clearer.

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:282: sessions::SessionTab*
SyncedSessionTracker::GetTab(
On 2017/01/25 19:23:41, Patrick Noland wrote:
> This function does two things: it creates a SessionTab for the given
session_tag
> and tab_id if one doesn't exist, and it returns the SessionTab for the tab_id.
> Is there a good way to rename this or else  refactor it to functions that only
> do one thing? e.g. AllocateSessionTab(), LookupSessionTab(). This would mean
you
> don't have to do confusing things like ignore the result of it.

I think that would actually make things more complicated. Knowing which to call,
Allocate or Lookup, is something that only SyncedSessionTracker can do. Also
keep in mind that LookupSessionTab doesn't provide access to a mutable version
of the SessionTab, only a immutable one.

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:286: sessions::SessionTab*
tab_ptr = nullptr;
On 2017/01/25 19:23:41, Patrick Noland wrote:
> Can this use LookupSessionTab ?

That's specifically a const SessionTab*, and this method is returning a
non-const.

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:336: bool
SyncedSessionTracker::GetTabNodeForLocalTab(int tab_id, int* tab_node_id) {
On 2017/01/25 19:23:41, Patrick Noland wrote:
> Given that GetTab refers to a SessionTab* and this refers to a tab_id, it
would
> be clearer to name this "GetTabNodeForLocalTabId". Or else refer to
SessionTabs
> by their full name elsewhere

Done.

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:349:
DCHECK_NE(TabNodePool::kInvalidTabNodeID, *tab_node_id);
On 2017/01/25 19:23:41, Patrick Noland wrote:
> Why DCHECK here and CHECK at the call site?

The CHECK at the callsite is just temporary while I verify some assumptions.
This will stay around.

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:355: if (tab_node_id <=
TabNodePool::kInvalidTabNodeID)
On 2017/01/24 20:46:19, skym wrote:
> Why is this condition using <= when everything else is checking equality?

Done.

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:362: SessionID::id_type
new_tab_id) {
On 2017/01/25 19:23:41, Patrick Noland wrote:
> I'm confused by the usage of both SessionID::id_type and int as the type for a
> tab_id. Could you make it consistent?

Good point, done (all tab_id's should use SessionID::id_type).

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:368:
local_tab_pool_.GetTabIdFromTabNodeId(tab_node_id);
On 2017/01/25 19:23:41, Patrick Noland wrote:
> I'm also confused by some functions using the term "TabNode" and others using
> "TabNodeId" although this issue obviously predates this change

Fair point, although I think that should probably be in a separate CL since it
would touch a fair amount of code.

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:374: if (old_tab_id !=
TabNodePool::kInvalidTabID &&
On 2017/01/24 20:46:19, skym wrote:
> There should never be an entry for kInvalidTabID though, right? Should this
just
> be a CHECK?

If the tab_node_id isn't found (because it wasn't previously associated with
anything), GetTabIdFromTabNodeId will return kInvalidTabId. So we should handle
that.

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
File components/sync_sessions/synced_session_tracker.h (right):

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.h:169: bool
IsLocalTabNodeValid(int tab_node_id);
On 2017/01/24 20:46:19, skym wrote:
> "Valid" is kind of confusing. Why is it invalid if it hasn't been seen? Our
> kInvalidXxId constants use a different definition of valid, it would seem.
Maybe
> one of seen/stale/fresh would be better?
> 
> Also, should this thing get unit tests?

Updated to IsLocalTabNodeAssociated

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

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker_unittest.cc:424: const int
kWindow1 = 1;
On 2017/01/25 19:23:41, Patrick Noland wrote:
> You use these constants in the previous function as well

Done.

[Nicolas Zea, 2017-01-25 23:55:11]

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

[commit-bot: I haz the power, 2017-01-25 23:56:00]

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

[skym, 2017-01-26 00:05:36]

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

https://codereview.chromium.org/2651583006/diff/30010/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:374: if (old_tab_id !=
TabNodePool::kInvalidTabID &&
On 2017/01/25 23:55:05, Nicolas Zea wrote:
> On 2017/01/24 20:46:19, skym wrote:
> > There should never be an entry for kInvalidTabID though, right? Should this
> just
> > be a CHECK?
> 
> If the tab_node_id isn't found (because it wasn't previously associated with
> anything), GetTabIdFromTabNodeId will return kInvalidTabId. So we should
handle
> that.

Are you saying it's possible to have
synced_tab_map_[local_session_tab_][TabNodePool::kInvalidTabID] actually contain
something?

[Patrick Noland, 2017-01-26 00:05:46]

lgtm

[commit-bot: I haz the power, 2017-01-26 03:04:30]

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

[commit-bot: I haz the power, 2017-01-26 03:04:30]

Dry run: Try jobs failed on following builders:
  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-01-26 18:41:50]

The CQ bit was checked by zea@chromium.org

[Nicolas Zea, 2017-01-26 18:41:50]

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/2651583006/#ps70001
(title: "Address Patrick comments")

[commit-bot: I haz the power, 2017-01-26 18:42:21]

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-01-26 19:49:45]

CQ is committing da patch.

Bot data: {"patchset_id": 70001, "attempt_start_ts": 1485456110208090,
"parent_rev": "72cb24d232fb79d878f8c0ab16b736e207d9c355", "commit_rev":
"0b0a0a13587273010f58db836c86c0d4748e9e5b"}

[commit-bot: I haz the power, 2017-01-26 19:50:15]

Description was changed from

==========
Reland v3 of Session refactor

Original CL review: https://codereview.chromium.org/2494533002

This relands that CL, with a couple key fixes:
- Detect if a previously synced tab no longer exists, and ignore it if so
- Simplifies the logic to add tab ids to a window to avoid possibly
out-of-bounds
- Adds checking for invalid tab node ids
- Simplifies the logic that either calls AssociateTab or
AssociateRestoredPlaceholderTab
- Adds some CHECKs to help detect remaining issues.

BUG=639009, 673618
==========

to

==========
Reland v3 of Session refactor

Original CL review: https://codereview.chromium.org/2494533002

This relands that CL, with a couple key fixes:
- Detect if a previously synced tab no longer exists, and ignore it if so
- Simplifies the logic to add tab ids to a window to avoid possibly
out-of-bounds
- Adds checking for invalid tab node ids
- Simplifies the logic that either calls AssociateTab or
AssociateRestoredPlaceholderTab
- Adds some CHECKs to help detect remaining issues.

BUG=639009, 673618

Review-Url: https://codereview.chromium.org/2651583006
Cr-Commit-Position: refs/heads/master@{#446405}
Committed:
https://chromium.googlesource.com/chromium/src/+/0b0a0a13587273010f58db836c86...
==========

[commit-bot: I haz the power, 2017-01-26 19:50:16]

Committed patchset #5 (id:70001) as
https://chromium.googlesource.com/chromium/src/+/0b0a0a13587273010f58db836c86...

[Nicolas Zea, 2017-01-30 23:59:25]

A revert of this CL (patchset #5 id:70001) has been created in
https://codereview.chromium.org/2664023002/ by zea@chromium.org.

The reason for reverting is: crbug.com/686620.