[Sync] Refactor handling of session tabs

[Sync] Refactor handling of session tabs

Add SessionTab <-> sync_pb::SessionTab conversion functions.

Add IsPinned() method to SyncedTabDelegate and fix latent NULL-dereference bug in the process.  Remove HasExtensionAppId() also.

Rework tab contents association to:

  - update a SessionTab from the tab delegate (UpdateSessionTabFromDelegate);
  - get the current virtual URL for the tab (GetCurrentVirtualURL);
  - and convert to sync_pb::SessionTab

in separate phases.

Rework the SessionModelAssociator tests.

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

[akalin, 2012-09-25 00:58:16]

+zea for sync stuff
+sky for everything else (sessions, ui)

[sky, 2012-09-25 15:48:38]

LGTM

[Nicolas Zea, 2012-09-26 00:26:46]

http://codereview.chromium.org/10990012/diff/14001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/session_model_associator.cc (right):

http://codereview.chromium.org/10990012/diff/14001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/session_model_associator.cc:490:
previous_navigations.swap(session_tab->navigations);
is there any reason you modify previous_navigations instead of
session_tab->navigations directly?

[akalin, 2012-09-26 00:32:12]

http://codereview.chromium.org/10990012/diff/14001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/session_model_associator.cc (right):

http://codereview.chromium.org/10990012/diff/14001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/session_model_associator.cc:490:
previous_navigations.swap(session_tab->navigations);
On 2012/09/26 00:26:47, nzea wrote:
> is there any reason you modify previous_navigations instead of
> session_tab->navigations directly?

session_tab->navigations should be cleared and filled in from the new
NavigationEntry's right?  But we need to keep around the old navigations to
compare timestamps, so that's why we put those in previous_navigations.

[Nicolas Zea, 2012-09-26 00:35:16]

LGTM

http://codereview.chromium.org/10990012/diff/14001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/session_model_associator.cc (right):

http://codereview.chromium.org/10990012/diff/14001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/session_model_associator.cc:490:
previous_navigations.swap(session_tab->navigations);
On 2012/09/26 00:32:12, akalin wrote:
> On 2012/09/26 00:26:47, nzea wrote:
> > is there any reason you modify previous_navigations instead of
> > session_tab->navigations directly?
> 
> session_tab->navigations should be cleared and filled in from the new
> NavigationEntry's right?  But we need to keep around the old navigations to
> compare timestamps, so that's why we put those in previous_navigations.

Ah, right. Makes sense.

[commit-bot: I haz the power, 2012-09-26 07:28:41]

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

[commit-bot: I haz the power, 2012-09-26 09:57:52]

Change committed as 158777