[Sync] Add per-navigation timestamps/unique ids to tab sync.

[Sync] Add per-navigation timestamps/unique ids to tab sync.

A navigation's timestamps are updated only if the navigation is new
or if the user recently moved back/foward to the navigation entry.
Otherwise, we preserve old timestamps.

To do this, we switch to using SyncedTabNavigations, which also contains
the unique id of the navigation (pulled from NavigationEntry) and the
timestamp of the navgation, and SyncedSessionTab (which we have to
use in order to support SyncedTabNavigations).

Lastly, the protos have been updated to include unique_id/timestamp per
navigation, remove some obsolete fields that were unimportant, and add
comments about what transitions mean and when they're set.

BUG=98892
TEST=unit_tests, manually inspecting timestamps in sync node browser.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133942

[Nicolas Zea, 2012-04-19 23:47:29]

PTAL Drew
+CC raz

[Nicolas Zea, 2012-04-21 00:51:07]

now with less deja vú!

[Andrew T Wilson (Slow), 2012-04-23 23:43:11]

Mostly LG, just a couple of questions.

http://codereview.chromium.org/10125002/diff/7001/chrome/browser/sync/glue/se...
File chrome/browser/sync/glue/session_model_associator.h (right):

http://codereview.chromium.org/10125002/diff/7001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:299: // TODO(zea): pull this
into it's own file.
nit: it's->its

http://codereview.chromium.org/10125002/diff/7002/chrome/browser/sync/glue/se...
File chrome/browser/sync/glue/session_model_associator.cc (right):

http://codereview.chromium.org/10125002/diff/7002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:416: const
sync_pb::SessionSpecifics old_specifics =
Does this imply if there's a pending load for the favicon, it'll get plopped
into this new tab?

http://codereview.chromium.org/10125002/diff/7002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:471: // Find the location
of the first navigation within the previous list of
I was trying to see if there's a good way to move this outside the for-loop (for
clarity - it's only run a single time) but I guess there isn't since we rely on
entry->GetUniqueID().

http://codereview.chromium.org/10125002/diff/7002/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_session.cc (right):

http://codereview.chromium.org/10125002/diff/7002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session.cc:17: : unique_id_(tab.unique_id_),
Are you sure this copy constructor copies over the TabNavigation fields?

[Nicolas Zea, 2012-04-24 22:09:49]

PTAL

http://codereview.chromium.org/10125002/diff/7001/chrome/browser/sync/glue/se...
File chrome/browser/sync/glue/session_model_associator.h (right):

http://codereview.chromium.org/10125002/diff/7001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:299: // TODO(zea): pull this
into it's own file.
On 2012/04/23 23:43:11, Andrew T Wilson wrote:
> nit: it's->its

Done.

http://codereview.chromium.org/10125002/diff/7002/chrome/browser/sync/glue/se...
File chrome/browser/sync/glue/session_model_associator.cc (right):

http://codereview.chromium.org/10125002/diff/7002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:416: const
sync_pb::SessionSpecifics old_specifics =
On 2012/04/23 23:43:11, Andrew T Wilson wrote:
> Does this imply if there's a pending load for the favicon, it'll get plopped
> into this new tab?

Yes. Either we've already loaded the favicon for this url, in which case we're
preserving it with this code, or it's pending, in which case
OnFaviconDataAvailable will write it later and we're preserving an empty favicon
with this code.

http://codereview.chromium.org/10125002/diff/7002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:471: // Find the location
of the first navigation within the previous list of
On 2012/04/23 23:43:11, Andrew T Wilson wrote:
> I was trying to see if there's a good way to move this outside the for-loop
(for
> clarity - it's only run a single time) but I guess there isn't since we rely
on
> entry->GetUniqueID().

Yeah, if I did I'd have to load the entry anyways, so this seems simplest.

http://codereview.chromium.org/10125002/diff/7002/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_session.cc (right):

http://codereview.chromium.org/10125002/diff/7002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session.cc:17: : unique_id_(tab.unique_id_),
On 2012/04/23 23:43:11, Andrew T Wilson wrote:
> Are you sure this copy constructor copies over the TabNavigation fields?

Good catch, I meant to invoke the constructor directly. Done.

[Andrew T Wilson (Slow), 2012-04-24 22:28:22]

LGTM

[Nicolas Zea, 2012-04-24 22:33:24]

+marja for sessions ownership

[marja, 2012-04-25 07:07:37]

chrome/browser/sessions LGTM.

[commit-bot: I haz the power, 2012-04-25 16:59:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/10125002/20001

[commit-bot: I haz the power, 2012-04-25 18:28:46]

Change committed as 133942