[SYNC] Sessions datatype refactor. Most things related to sessions under-the-...

[SYNC] Sessions datatype refactor. Most things related to sessions under-the-hood have changed. More tests on the way.

BUG=30519, 62753
TEST=unit_tests,sync_integration_tests


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

[Nicolas Zea, 2010-12-10 21:34:16]

+tim for review. More tests are forthcoming.

[Nicolas Zea, 2010-12-10 21:34:41]

This time actually +tim.

[tim (not reviewing), 2010-12-15 21:14:00]

I like where this is going!
I'm still trying to grok most of session_model_associator, but have some prelim
comments to chew on.

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/dom_ui/forei...
File chrome/browser/dom_ui/foreign_session_handler.cc (right):

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/dom_ui/forei...
chrome/browser/dom_ui/foreign_session_handler.cc:164: if (tab_id != -1) {
what's '-1'?  Can we get a named constant? (to use here and below)

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/resources/ne...
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/resources/ne...
chrome/browser/resources/new_new_tab.js:980: function maybeOpenForeignSession(e)
{
comment what 'maybe' means

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
File chrome/browser/sync/glue/session_change_processor.cc (right):

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_change_processor.cc:209:
NotificationService::AllSources());
seems like we should specify the source (Profile, in this case) we want and
avoid handling mismatches in Observe (could just have DCHECK_EQs)

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

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:48: void
SessionModelAssociator::DeleteForeignSessions() {
should we slap CalledOnValidThread in most of these places?

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:263: // TODO(zea): perhaps
sync state (scroll position, form entries, etc.) as well?
so far we explicitly didn't want to do this but I think we should favor having a
bug instead of a todo to track it

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:339: // TODO(zea): we will
need this once we support deleting foreign sessions.
is it called anywhere?
add a NOTIMPLEMENTED() ?

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:412: // TODO(zea): We need
a better way of creating a machine tag. The directory
reference bug #

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:856: // if we choose to
sync the previous browser session.
previous? is there a bug#?

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

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:171: explicit TabLinks()
remove explicit

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:172: : sync_id_(0),
nit - line above or 4 space indent.

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:208: typedef
std::map<SessionID, SessionTab*, SessionIDLessThan> IDToSessTabMap;
we should really use IDToSessionTabMap and TabToSessionMap

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:221: bool
ShouldSyncWindowType(Browser::Type type);
const? or static

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:231: std::istringstream
iss(tag);
we try not to use streams in chromium.  this also seems a bit brittle, although
probably faster than using a delimiter with SplitString.  But if we know the
first n chars are GetCurrentMachineTag, we could do something similar I think:

client_tag = GetCurrentMachineTag();
DCHECK(base::StartsWith(tag, client_tag, true));
StringToInt(tag.substr(client_tag.length, npos), &tab_node_id);

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:241: // Support wide char
strings.
where's this used? we can't use string16 can we?

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:247: // Note: creates a
syncable::BaseTransaction.
I'd say something like 'Precondition: do not call this within a transaction.'
Alternatively you can require the transaction be passed in to force the caller
to know.

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:351: // Pool of all
available syncid's for tab's we have created.
what's 'available' mean? the comment next to tab_pool_fp_ confuses me...
Is order important here?

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:357: // To get the next free
node, use tab_syncid_pool_[tab_pool_fp_--].
I actually would consider pulling all this pool stuff into it's own little class
to prevent dumb mistakes trying to combine the various variables.  the
Free/GetFree methods above could call in, perhaps, and we could test it
separately.

[Nicolas Zea, 2010-12-16 18:09:18]

New version up. Still trying to figure out why windows unit test didn't succeed.

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/dom_ui/forei...
File chrome/browser/dom_ui/foreign_session_handler.cc (right):

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/dom_ui/forei...
chrome/browser/dom_ui/foreign_session_handler.cc:164: if (tab_id != -1) {
On 2010/12/15 21:14:00, timsteele wrote:
> what's '-1'?  Can we get a named constant? (to use here and below)

Done.

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/resources/ne...
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/resources/ne...
chrome/browser/resources/new_new_tab.js:980: function maybeOpenForeignSession(e)
{
On 2010/12/15 21:14:00, timsteele wrote:
> comment what 'maybe' means

Done.

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
File chrome/browser/sync/glue/session_change_processor.cc (right):

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_change_processor.cc:209:
NotificationService::AllSources());
The source is either the Browser, NavigationController, or TabContents. I've
added check in observer to ensure we're only acting on the correct profile (and
doing nothing otherwise).
On 2010/12/15 21:14:00, timsteele wrote:
> seems like we should specify the source (Profile, in this case) we want and
> avoid handling mismatches in Observe (could just have DCHECK_EQs)

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

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:48: void
SessionModelAssociator::DeleteForeignSessions() {
Why not.

On 2010/12/15 21:14:00, timsteele wrote:
> should we slap CalledOnValidThread in most of these places?

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:263: // TODO(zea): perhaps
sync state (scroll position, form entries, etc.) as well?
On 2010/12/15 21:14:00, timsteele wrote:
> so far we explicitly didn't want to do this but I think we should favor having
a
> bug instead of a todo to track it

Done.

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:339: // TODO(zea): we will
need this once we support deleting foreign sessions.
On 2010/12/15 21:14:00, timsteele wrote:
> is it called anywhere?
> add a NOTIMPLEMENTED() ?

Done.

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:412: // TODO(zea): We need
a better way of creating a machine tag. The directory
See end of comment paragraph. I went ahead and turned it into a url as well.

On 2010/12/15 21:14:00, timsteele wrote:
> reference bug #

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:856: // if we choose to
sync the previous browser session.
No, should I make one? this refers to what Travis was mentioning, wherein we may
at some point want to sync previous browser sessions if we see that there are no
open tabs (ie the browser crashed or user quit). We didn't know if we were
planning to do this though.

On 2010/12/15 21:14:00, timsteele wrote:
> previous? is there a bug#?

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

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:171: explicit TabLinks()
On 2010/12/15 21:14:00, timsteele wrote:
> remove explicit

Done.

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:172: : sync_id_(0),
On 2010/12/15 21:14:00, timsteele wrote:
> nit - line above or 4 space indent.

Done.

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:208: typedef
std::map<SessionID, SessionTab*, SessionIDLessThan> IDToSessTabMap;
On 2010/12/15 21:14:00, timsteele wrote:
> we should really use IDToSessionTabMap and TabToSessionMap

Done.

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:221: bool
ShouldSyncWindowType(Browser::Type type);
On 2010/12/15 21:14:00, timsteele wrote:
> const? or static

Done.

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:231: std::istringstream
iss(tag);
See comment response below.

On 2010/12/15 21:14:00, timsteele wrote:
> we try not to use streams in chromium.  this also seems a bit brittle,
although
> probably faster than using a delimiter with SplitString.  But if we know the
> first n chars are GetCurrentMachineTag, we could do something similar I think:
> 
> client_tag = GetCurrentMachineTag();
> DCHECK(base::StartsWith(tag, client_tag, true));
> StringToInt(tag.substr(client_tag.length, npos), &tab_node_id);

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:241: // Support wide char
strings.
Originally I was parsing the title from syncapi (GetTitle()), which returned a
wstring. At this point, I'm not longer using that functionality, so we don't
need to bother with either of these methods.

On 2010/12/15 21:14:00, timsteele wrote:
> where's this used? we can't use string16 can we?

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:247: // Note: creates a
syncable::BaseTransaction.
On 2010/12/15 21:14:00, timsteele wrote:
> I'd say something like 'Precondition: do not call this within a transaction.'
> Alternatively you can require the transaction be passed in to force the caller
> to know.

Done.

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:351: // Pool of all
available syncid's for tab's we have created.
On 2010/12/15 21:14:00, timsteele wrote:
> what's 'available' mean? the comment next to tab_pool_fp_ confuses me...
> Is order important here?

Done.

http://codereview.chromium.org/5705004/diff/24002/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:357: // To get the next free
node, use tab_syncid_pool_[tab_pool_fp_--].
On 2010/12/15 21:14:00, timsteele wrote:
> I actually would consider pulling all this pool stuff into it's own little
class
> to prevent dumb mistakes trying to combine the various variables.  the
> Free/GetFree methods above could call in, perhaps, and we could test it
> separately.

Done.

[tim (not reviewing), 2010-12-20 18:58:44]

Looking good.  Any luck with the tests? Having a TabNodePool test to go in with
this commit would be real nice.

http://codereview.chromium.org/5705004/diff/45001/chrome/browser/dom_ui/forei...
File chrome/browser/dom_ui/foreign_session_handler.cc (right):

http://codereview.chromium.org/5705004/diff/45001/chrome/browser/dom_ui/forei...
chrome/browser/dom_ui/foreign_session_handler.cc:30: static const int kInvalid =
-1;
browser_sync::kInvalid is kinda generic. kInvalidId is a bit better.

http://codereview.chromium.org/5705004/diff/45001/chrome/browser/resources/ne...
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/5705004/diff/45001/chrome/browser/resources/ne...
chrome/browser/resources/new_new_tab.js:981: // not have any action (hence the
maybe).
Can't this case be made for any function ever? (i.e. foo() 'might' fail) :)  No
biggie.. I just meant to comment what warrants use of 'maybe' here vs in any
other function name.  It looks like it's because we handle and swallow
exceptions since they can occur in normal operation.

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

http://codereview.chromium.org/5705004/diff/45001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:373:
DCHECK(tab_pool_.capacity() == 0);
DCHECK_EQ

http://codereview.chromium.org/5705004/diff/45001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:773:
DCHECK(machine_tag_.length() > 0);
DCHECK_GT

http://codereview.chromium.org/5705004/diff/62001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:275:
PopulateSessionSpecificsNavigation(&tab_nav, nav_s);
nit - max_index - 1

http://codereview.chromium.org/5705004/diff/62001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:480: // This is a tab node.
We want to track these to reuse them in our free
DCHECK_EQ

http://codereview.chromium.org/5705004/diff/62001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:534: window_s,
nit - use static_cast (or use size_t for i here?)

http://codereview.chromium.org/5705004/diff/62001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:775: // Tab pool has no
free nodes, allocate new one.
nit - size() + 1

http://codereview.chromium.org/5705004/diff/62001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:780: LOG(ERROR) <<
kNoSessionsFolderError;
DCHECK_GT

http://codereview.chromium.org/5705004/diff/62001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:814: 
DCHECK_LT

http://codereview.chromium.org/5705004/diff/62001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:828: bool
SessionModelAssociator::GetForeignSession(
multi line if requires { }

[Nicolas Zea, 2010-12-21 18:12:23]

Round...3?

http://codereview.chromium.org/5705004/diff/45001/chrome/browser/dom_ui/forei...
File chrome/browser/dom_ui/foreign_session_handler.cc (right):

http://codereview.chromium.org/5705004/diff/45001/chrome/browser/dom_ui/forei...
chrome/browser/dom_ui/foreign_session_handler.cc:30: static const int kInvalid =
-1;
On 2010/12/20 18:58:44, timsteele wrote:
> browser_sync::kInvalid is kinda generic. kInvalidId is a bit better.

Done.

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

http://codereview.chromium.org/5705004/diff/45001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:373:
DCHECK(tab_pool_.capacity() == 0);
On 2010/12/20 18:58:44, timsteele wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/5705004/diff/45001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:773:
DCHECK(machine_tag_.length() > 0);
On 2010/12/20 18:58:44, timsteele wrote:
> DCHECK_GT

Done.

http://codereview.chromium.org/5705004/diff/62001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:275:
PopulateSessionSpecificsNavigation(&tab_nav, nav_s);
On 2010/12/20 18:58:44, timsteele wrote:
> nit - max_index - 1

Done.

http://codereview.chromium.org/5705004/diff/62001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:480: // This is a tab node.
We want to track these to reuse them in our free
On 2010/12/20 18:58:44, timsteele wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/5705004/diff/62001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:534: window_s,
On 2010/12/20 18:58:44, timsteele wrote:
> nit - use static_cast (or use size_t for i here?)

Done.

http://codereview.chromium.org/5705004/diff/62001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:775: // Tab pool has no
free nodes, allocate new one.
On 2010/12/20 18:58:44, timsteele wrote:
> nit - size() + 1

Done.

http://codereview.chromium.org/5705004/diff/62001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:780: LOG(ERROR) <<
kNoSessionsFolderError;
On 2010/12/20 18:58:44, timsteele wrote:
> DCHECK_GT

Done.

http://codereview.chromium.org/5705004/diff/62001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:814: 
On 2010/12/20 18:58:44, timsteele wrote:
> DCHECK_LT

Done.

http://codereview.chromium.org/5705004/diff/62001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:828: bool
SessionModelAssociator::GetForeignSession(
On 2010/12/20 18:58:44, timsteele wrote:
> multi line if requires { }

Done.

[tim (not reviewing), 2011-01-04 14:32:46]

Once all comments addressed, this LGTM!

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

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:49: class
ForeignSessionTracker {
this should be in a foreign_session_tracker.h/cc file

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:114: private:
does the Tracker own all the values in these maps?

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:201: // Load any ƒoreign
session info stored in sync db and update the sync db
whoa, fancy f!

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:298: // Getters.
don't really need this comment.

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:311: public:
nit - indent

Did you consider putting TabNodePool in its own file? Fine for now though.

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:313:
TabNodePool(ProfileSyncService* sync_service,
can you distinguish / explain why multiple ctors are needed? (see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi....
 Sometimes I prefer a named factory method vs second ctor (i.e.
TabNodePoolWithMachineTag, maybe not here though).

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:363: private:
nit - indent

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/glue/se...
File chrome/browser/sync/glue/session_model_associator_unittest.cc (right):

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator_unittest.cc:21: class
SessionModelAssociatorTest : public testing::Test {
note you can do 'typedef testing::Test SessionModelAssociatorTest'; if you like.
 Or if you dont think you'll ever need boilerplate (probably not the case here)
you could remove this altogether and just use TEST(...,...) below.

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator_unittest.cc:65:
scoped_ptr<SessionWindow> win(new SessionWindow());
instead of the extra clause at the end, could we just create a raw pointer here
and immediately push_back on session->windows?

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_session_unittest.cc (right):

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_session_unittest.cc:368: // Test the
TabNodePool when it starts off empty
nit - you should end comments with a period.

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/protoco...
File chrome/browser/sync/protocol/session_specifics.proto (right):

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/protoco...
chrome/browser/sync/protocol/session_specifics.proto:28: // Unique (to the
owner) id for this window.
nit- indent

http://codereview.chromium.org/5705004/diff/81001/chrome/test/live_sync/live_...
File chrome/test/live_sync/live_sessions_sync_test.h (right):

http://codereview.chromium.org/5705004/diff/81001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sessions_sync_test.h:252: // Caller owns sessions,
but not ForeignSession objects.
saying 'Caller owns |sessions|, but not ForeignSession values within' would be
more clear. unless i'm reading this wrong.

http://codereview.chromium.org/5705004/diff/81001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sessions_sync_test.h:311: const TabNavigation&
actual) {
nit - indent

[Nicolas Zea, 2011-01-06 01:14:13]

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

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:49: class
ForeignSessionTracker {
On 2011/01/04 14:32:47, timsteele wrote:
> this should be in a foreign_session_tracker.h/cc file

Done.

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:114: private:
On 2011/01/04 14:32:47, timsteele wrote:
> does the Tracker own all the values in these maps?

Done.

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:201: // Load any ƒoreign
session info stored in sync db and update the sync db
On 2011/01/04 14:32:47, timsteele wrote:
> whoa, fancy f!

Done.

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:298: // Getters.
On 2011/01/04 14:32:47, timsteele wrote:
> don't really need this comment.

Done.

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:311: public:
I did, but since it seemed very session model associator specific, I figured it
was more appropriate in here.

On 2011/01/04 14:32:47, timsteele wrote:
> nit - indent
> 
> Did you consider putting TabNodePool in its own file? Fine for now though.

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:313:
TabNodePool(ProfileSyncService* sync_service,
On 2011/01/04 14:32:47, timsteele wrote:
> can you distinguish / explain why multiple ctors are needed? (see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi....
>  Sometimes I prefer a named factory method vs second ctor (i.e.
> TabNodePoolWithMachineTag, maybe not here though).

Done.

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.h:363: private:
On 2011/01/04 14:32:47, timsteele wrote:
> nit - indent

Done.

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/glue/se...
File chrome/browser/sync/glue/session_model_associator_unittest.cc (right):

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator_unittest.cc:21: class
SessionModelAssociatorTest : public testing::Test {
On 2011/01/04 14:32:47, timsteele wrote:
> note you can do 'typedef testing::Test SessionModelAssociatorTest'; if you
like.
>  Or if you dont think you'll ever need boilerplate (probably not the case
here)
> you could remove this altogether and just use TEST(...,...) below.

Done.

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator_unittest.cc:65:
scoped_ptr<SessionWindow> win(new SessionWindow());
On 2011/01/04 14:32:47, timsteele wrote:
> instead of the extra clause at the end, could we just create a raw pointer
here
> and immediately push_back on session->windows?

Done.

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_session_unittest.cc (right):

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_session_unittest.cc:368: // Test the
TabNodePool when it starts off empty
On 2011/01/04 14:32:47, timsteele wrote:
> nit - you should end comments with a period.

Done.

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/protoco...
File chrome/browser/sync/protocol/session_specifics.proto (right):

http://codereview.chromium.org/5705004/diff/81001/chrome/browser/sync/protoco...
chrome/browser/sync/protocol/session_specifics.proto:28: // Unique (to the
owner) id for this window.
On 2011/01/04 14:32:47, timsteele wrote:
> nit- indent

Done.

http://codereview.chromium.org/5705004/diff/81001/chrome/test/live_sync/live_...
File chrome/test/live_sync/live_sessions_sync_test.h (right):

http://codereview.chromium.org/5705004/diff/81001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sessions_sync_test.h:252: // Caller owns sessions,
but not ForeignSession objects.
On 2011/01/04 14:32:47, timsteele wrote:
> saying 'Caller owns |sessions|, but not ForeignSession values within' would be
> more clear. unless i'm reading this wrong.

Done.

http://codereview.chromium.org/5705004/diff/81001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sessions_sync_test.h:311: const TabNavigation&
actual) {
On 2011/01/04 14:32:47, timsteele wrote:
> nit - indent

Done.