[Sync] Introduce SyncedSessionWindow type.

components/sync_sessions/synced_session.h

Index: components/sync_sessions/synced_session.h
diff --git a/components/sync_sessions/synced_session.h b/components/sync_sessions/synced_session.h
index e0defd96623490463ba4a8182e13dae483b711c4..dc2badddf82052209fa9d57048c9d3579d6722ec 100644
--- a/components/sync_sessions/synced_session.h
+++ b/components/sync_sessions/synced_session.h
@@ -16,12 +16,26 @@
 #include "components/sessions/core/session_types.h"
 #include "components/sync/protocol/session_specifics.pb.h"
 
-namespace sessions {
-struct SessionWindow;
-}
-
 namespace sync_sessions {
 
+// A Sync wrapper for a SessionWindow.
+struct SyncedSessionWindow {
+  SyncedSessionWindow();
+  ~SyncedSessionWindow();
+
+  // Convert this object into its sync protocol buffer equivalent.
+  sync_pb::SessionWindow ToSyncSessionWindowPB() const;

[skym, 2017/03/28 04:48:55]

The "PB" looks really ugly to me, I'd prefer something like
ToSyncProtoSessionWindow(), but I'm fine with whatever you think is best.

[Nicolas Zea, 2017/03/29 19:01:33]

Switched to ToSessionWindowProto

+
+  // Type of the window. See session_specifics.proto.
+  sync_pb::SessionWindow::BrowserType window_type;
+
+  // The SessionWindow this object wraps.
+  sessions::SessionWindow wrapped_window;

[skym, 2017/03/28 04:48:55]

Does it really make sense to hang onto a SessionWindow indefinitely? It seems
like there are quite a few fields that we don't care about. workspace, bounds,
show_state, app_name, etc. Especially for foreign sessions, having a
wrapped_window is kind of lie. What do you think of holding onto our members
directly here, and having another ToNativeSessionWindow() accessor that creates
a sessions::SessionWindow on demand?

What if we remove all concepts of "wrapping in SessionsSyncManager and friends
and they simply had its own first class models that held data in the way that
was the most convenient to them. I feel like adding .wrapped_window. is going
the wrong way here.

[Patrick Noland, 2017/03/28 20:42:00]

It seems like many accesses of wrapped_window just want the tabs; would it make
sense to add an accessor to insulate these callers from the fact that there's a
wrapped window? 

[Nicolas Zea, 2017/03/29 19:01:33]

I do think it probably makes sense to move away from sessions::SessionWindow
entirely at some point, but I think that's outside of the scope of this patch
(as discussed offline). I think that will eventually also address Patrick's
concern about callers not caring about whether the window is wrapped or not.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SyncedSessionWindow);
+};
+
 // Defines a synced session for use by session sync. A synced session is a
 // list of windows along with a unique session identifer (tag) and meta-data
 // about the device being synced.
@@ -55,8 +69,7 @@ struct SyncedSession {
   base::Time modified_time;
 
   // Map of windows that make up this session.
-  std::map<SessionID::id_type, std::unique_ptr<sessions::SessionWindow>>
-      windows;
+  std::map<SessionID::id_type, std::unique_ptr<SyncedSessionWindow>> windows;
 
   // A tab node id is part of the identifier for the sync tab objects. Tab node
   // ids are not used for interacting with the model/browser tabs. However, when
@@ -95,7 +108,7 @@ struct SyncedSession {
 
   // Convert this object to its protocol buffer equivalent. Shallow conversion,
   // does not create SessionTab protobufs.
-  sync_pb::SessionHeader ToSessionHeader() const;
+  sync_pb::SessionHeader ToSyncSessionHeaderPB() const;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(SyncedSession);