Revamp TabNavigation class

Move all TabNavigation logic to session_types.{h,cc}

In particular, move the conversion to/from NavigationEntry,
to/from sync_pb::TabNavigation, and pickling/unpickling logic
to member functions of TabNavigation.

Merge TabNavigation and SyncedTabNavigation (i.e., add unique_id and timestamp to TabNavigation).

Mark most TabNavigation accessors ForTest().

Make CreateUpdateTabNavigationCommand() and UpdateTabNavigation() take a TabNavigation instead of an index + NavigationEntry.

Make TabNavigation use default copy constructor and assignment operator.  Also do some other misc. cleanups.

Also remove unused field 'unique_id' from sync_pb::TabNavigation.

Use correct indices in session model associator.

Add some unit tests for TabNavigation.

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

[akalin, 2012-09-15 00:43:10]

+zea for sync stuff, +sky for everything else

[sky, 2012-09-17 14:03:36]

http://codereview.chromium.org/10917231/diff/1021/chrome/browser/sessions/ses...
File chrome/browser/sessions/session_types.cc (right):

http://codereview.chromium.org/10917231/diff/1021/chrome/browser/sessions/ses...
chrome/browser/sessions/session_types.cc:147: void WriteStringToPickle(Pickle*
pickle, int* bytes_written, int max_bytes,
Each param on its own line. Any how about a template?

http://codereview.chromium.org/10917231/diff/1021/chrome/browser/sessions/ses...
chrome/browser/sessions/session_types.cc:226: int type_mask = 0;
nit: type_mask = has_post_data ? HAS_POST_DATA : 0

http://codereview.chromium.org/10917231/diff/1021/chrome/browser/sessions/ses...
chrome/browser/sessions/session_types.cc:227: if (has_post_data_) {
no {}

http://codereview.chromium.org/10917231/diff/1021/chrome/browser/sessions/ses...
chrome/browser/sessions/session_types.cc:290: if
(!iterator->ReadBool(&is_overriding_user_agent_)) {
no {}

http://codereview.chromium.org/10917231/diff/1021/chrome/browser/sessions/ses...
File chrome/browser/sessions/session_types.h (right):

http://codereview.chromium.org/10917231/diff/1021/chrome/browser/sessions/ses...
chrome/browser/sessions/session_types.h:43: // Default copy constructor and
assignment operator welcome.
Move this back to class description.

http://codereview.chromium.org/10917231/diff/1021/chrome/browser/sessions/ses...
chrome/browser/sessions/session_types.h:104: // Functions for testing.
I don't like all the for test foo. They clutter up the class, may be
inadvertently used and may increase compile time. Instead create a class that is
only compiled when testing that is friended, takes a TabNavigation and does all
this.

http://codereview.chromium.org/10917231/diff/1021/chrome/browser/sessions/ses...
chrome/browser/sessions/session_types.h:138: int unique_id_;
Why do we now need the unique id?

http://codereview.chromium.org/10917231/diff/1021/chrome/browser/sessions/ses...
chrome/browser/sessions/session_types.h:152: base::Time timestamp_;
Can we add the two at the same time?

[Nicolas Zea, 2012-09-17 21:34:20]

sync LGTM

[akalin, 2012-09-17 23:24:32]

Addressed comments, sky PTAL

https://codereview.chromium.org/10917231/diff/1021/chrome/browser/sessions/se...
File chrome/browser/sessions/session_types.cc (right):

https://codereview.chromium.org/10917231/diff/1021/chrome/browser/sessions/se...
chrome/browser/sessions/session_types.cc:147: void WriteStringToPickle(Pickle*
pickle, int* bytes_written, int max_bytes,
On 2012/09/17 14:03:36, sky wrote:
> Each param on its own line. Any how about a template?

Put them on their own lines.  Can't template, since WriteString is used in one
and WriteString16 is used in another.

Well, I guess you could template it if you pass in a member function pointer, or
have a traits class.  But I don't know if it's worth it.

https://codereview.chromium.org/10917231/diff/1021/chrome/browser/sessions/se...
chrome/browser/sessions/session_types.cc:226: int type_mask = 0;
On 2012/09/17 14:03:36, sky wrote:
> nit: type_mask = has_post_data ? HAS_POST_DATA : 0

Done.

https://codereview.chromium.org/10917231/diff/1021/chrome/browser/sessions/se...
chrome/browser/sessions/session_types.cc:227: if (has_post_data_) {
On 2012/09/17 14:03:36, sky wrote:
> no {}

moot

https://codereview.chromium.org/10917231/diff/1021/chrome/browser/sessions/se...
chrome/browser/sessions/session_types.cc:290: if
(!iterator->ReadBool(&is_overriding_user_agent_)) {
On 2012/09/17 14:03:36, sky wrote:
> no {}

Done.

https://codereview.chromium.org/10917231/diff/1021/chrome/browser/sessions/se...
File chrome/browser/sessions/session_types.h (right):

https://codereview.chromium.org/10917231/diff/1021/chrome/browser/sessions/se...
chrome/browser/sessions/session_types.h:43: // Default copy constructor and
assignment operator welcome.
On 2012/09/17 14:03:36, sky wrote:
> Move this back to class description.

Done.

https://codereview.chromium.org/10917231/diff/1021/chrome/browser/sessions/se...
chrome/browser/sessions/session_types.h:104: // Functions for testing.
On 2012/09/17 14:03:36, sky wrote:
> I don't like all the for test foo. They clutter up the class, may be
> inadvertently used and may increase compile time. Instead create a class that
is
> only compiled when testing that is friended, takes a TabNavigation and does
all
> this.

Done, moved to SessionTypesTestHelper.

https://codereview.chromium.org/10917231/diff/1021/chrome/browser/sessions/se...
chrome/browser/sessions/session_types.h:138: int unique_id_;
On 2012/09/17 14:03:36, sky wrote:
> Why do we now need the unique id?

Sync needs it to disambiguate navigations (even when we have timestamps, those
won't be enough).

https://codereview.chromium.org/10917231/diff/1021/chrome/browser/sessions/se...
chrome/browser/sessions/session_types.h:152: base::Time timestamp_;
On 2012/09/17 14:03:36, sky wrote:
> Can we add the two at the same time?

NavigationEntry already has a unique ID field (although it's read-only).

[sky, 2012-09-18 02:17:54]

LGTM with the following changes.

https://codereview.chromium.org/10917231/diff/11006/chrome/browser/sessions/s...
File chrome/browser/sessions/session_restore_android.cc (right):

https://codereview.chromium.org/10917231/diff/11006/chrome/browser/sessions/s...
chrome/browser/sessions/session_restore_android.cc:30:
TabNavigation::CreateNavigationEntriesFromTabNavigations(
spacing

https://codereview.chromium.org/10917231/diff/11006/chrome/browser/sessions/s...
File chrome/browser/sessions/session_service.cc (right):

https://codereview.chromium.org/10917231/diff/11006/chrome/browser/sessions/s...
chrome/browser/sessions/session_service.cc:691: const TabNavigation& navigation
=
remove &

https://codereview.chromium.org/10917231/diff/11006/chrome/browser/sessions/s...
chrome/browser/sessions/session_service.cc:715: const TabNavigation& navigation
=
no &

https://codereview.chromium.org/10917231/diff/11006/chrome/browser/sessions/s...
chrome/browser/sessions/session_service.cc:1363: const TabNavigation& navigation
=
no &

https://codereview.chromium.org/10917231/diff/11006/chrome/browser/sessions/s...
File chrome/browser/sessions/session_types_test_helper.cc (right):

https://codereview.chromium.org/10917231/diff/11006/chrome/browser/sessions/s...
chrome/browser/sessions/session_types_test_helper.cc:13: void
SessionTypesTestHelper::AssertNavigationEquals(
Name this ExpectNavigationEquals to match EXPECT_.

https://codereview.chromium.org/10917231/diff/11006/chrome/browser/sessions/s...
File chrome/browser/sessions/session_types_unittest.cc (right):

https://codereview.chromium.org/10917231/diff/11006/chrome/browser/sessions/s...
chrome/browser/sessions/session_types_unittest.cc:75: class TabNavigationTest :
public ::testing::Test {};
Use TEST( and remove this.

[akalin, 2012-09-18 20:11:10]

Addressed comments, committing via bots

http://codereview.chromium.org/10917231/diff/11006/chrome/browser/sessions/se...
File chrome/browser/sessions/session_restore_android.cc (right):

http://codereview.chromium.org/10917231/diff/11006/chrome/browser/sessions/se...
chrome/browser/sessions/session_restore_android.cc:30:
TabNavigation::CreateNavigationEntriesFromTabNavigations(
On 2012/09/18 02:17:54, sky wrote:
> spacing

Done.

http://codereview.chromium.org/10917231/diff/11006/chrome/browser/sessions/se...
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/10917231/diff/11006/chrome/browser/sessions/se...
chrome/browser/sessions/session_service.cc:691: const TabNavigation& navigation
=
On 2012/09/18 02:17:54, sky wrote:
> remove &

Done.  Although binding the return value to a const ref avoids a copy.  I guess
the copy is probably optimized out anyway.

http://codereview.chromium.org/10917231/diff/11006/chrome/browser/sessions/se...
chrome/browser/sessions/session_service.cc:715: const TabNavigation& navigation
=
On 2012/09/18 02:17:54, sky wrote:
> no &

Done.

http://codereview.chromium.org/10917231/diff/11006/chrome/browser/sessions/se...
chrome/browser/sessions/session_service.cc:1363: const TabNavigation& navigation
=
On 2012/09/18 02:17:54, sky wrote:
> no &

Done.

http://codereview.chromium.org/10917231/diff/11006/chrome/browser/sessions/se...
File chrome/browser/sessions/session_types_test_helper.cc (right):

http://codereview.chromium.org/10917231/diff/11006/chrome/browser/sessions/se...
chrome/browser/sessions/session_types_test_helper.cc:13: void
SessionTypesTestHelper::AssertNavigationEquals(
On 2012/09/18 02:17:54, sky wrote:
> Name this ExpectNavigationEquals to match EXPECT_.

Done.  I left the names in SessionServiceTestHelper alone, though. (Those should
probably be fixed in a separate CL)

http://codereview.chromium.org/10917231/diff/11006/chrome/browser/sessions/se...
File chrome/browser/sessions/session_types_unittest.cc (right):

http://codereview.chromium.org/10917231/diff/11006/chrome/browser/sessions/se...
chrome/browser/sessions/session_types_unittest.cc:75: class TabNavigationTest :
public ::testing::Test {};
On 2012/09/18 02:17:54, sky wrote:
> Use TEST( and remove this.

Done.

[commit-bot: I haz the power, 2012-09-18 20:11:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/10917231/13040

[commit-bot: I haz the power, 2012-09-18 20:58:29]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-18 23:33:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/10917231/33003

[commit-bot: I haz the power, 2012-09-19 04:04:21]

Change committed as 157491