Make TabRestoreService::Entry noncopyable and fix up surrounding code.

components/sessions/core/tab_restore_service.h

Index: components/sessions/core/tab_restore_service.h
diff --git a/components/sessions/core/tab_restore_service.h b/components/sessions/core/tab_restore_service.h
index f9eda99c3c0155adddf34530ab4b0d7dcaea2439..bba091a918183d82da7cf7d88e0f0209469f5e2e 100644
--- a/components/sessions/core/tab_restore_service.h
+++ b/components/sessions/core/tab_restore_service.h
@@ -53,56 +53,52 @@ class SESSIONS_EXPORT TabRestoreService : public KeyedService {
 
   struct SESSIONS_EXPORT Entry {
     Entry();
-    explicit Entry(Type type);
-    virtual ~Entry();
+    virtual ~Entry() = 0;
+    virtual Type type() const = 0;

[sky, 2016/08/02 23:12:30]

Virtual functions shouldn't use unix_hacker_style. That said, can you keep type
as a member with Entry(Type) the only constructor?

[Sidney San Martín, 2016/08/03 14:35:32]

Just to confirm, the non-unix_hacker_style would just be Type()?

[Sidney San Martín, 2016/08/03 14:35:32]

Could you fill me in on why (I'm still new on the team)? Since every instance
will be a specific subclass of Entry, I thought a member func made more sense. I
can change it back if you disagree.

[sky, 2016/08/03 15:16:07]

Expressing the type in the constructor implies the type isn't going to change. A
virtual function has no such contract. Additionally a virtual function is more
expensive.

[Sidney San Martín, 2016/08/03 15:50:23]

OK. Can I make the member const, and the constructor protected?

 
     // Unique id for this entry. The id is guaranteed to be unique for a
     // session.
     SessionID::id_type id;
 
-    // The type of the entry.
-    Type type;
-
     // The time when the window or tab was closed.
     base::Time timestamp;
 
     // Is this entry from the last session? This is set to true for entries that
     // were closed during the last session, and false for entries that were
     // closed during this session.
-    bool from_last_session;
+    bool from_last_session{false};

[sky, 2016/08/02 23:12:30]

Why the {} for many of these? = works just fine.

[Sidney San Martín, 2016/08/03 14:35:32]

I do uniform initialization because it doesn't allow narrowing, works for any
kind of initialization, and doesn't trigger the Most Vexing Parse. Consistently
using it means I don't have to think about when `=` and `()` are safe.

What makes `=` preferable? It looks like C++ Dos and Don'ts advises against
using it for "simple" initialization, with the justification that Google's style
guide says the same thing… but it doesn't say that anymore, as far as I can
tell.

If you think it's particularly weird, I'm happy to change it.

https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GF96FshwHLw
https://google.github.io/styleguide/cppguide.html#Variable_and_Array_Initiali...

[sky, 2016/08/03 15:16:07]

"Use C++11 "uniform init" syntax ("{}" without '=') only when neither of the
above work:" where the above includes the '='. I take that to mean prefer '='.

[Sidney San Martín, 2016/08/04 04:24:57]

Done. I misread the mailing list thread — I thought that quote was based on an
old version of the Google style guide, but it comes from go/totw:88, which still
makes that recommendation. The justification is:

- Ambiguity when an initializer list constructor and another constructor both
apply.
- "no other common language uses something like it"

Fair enough.

+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(Entry);
   };
 
   // Represents a previously open tab.
   struct SESSIONS_EXPORT Tab : public Entry {
     Tab();
-    Tab(const Tab& tab);
     ~Tab() override;
-
-    Tab& operator=(const Tab& tab);
-
-    bool has_browser() const { return browser_id > 0; }
+    Type type() const override;
 
     // The navigations.
     std::vector<SerializedNavigationEntry> navigations;
 
     // Index of the selected navigation in navigations.
-    int current_navigation_index;
+    size_t current_navigation_index{-1};

[sky, 2016/08/02 23:12:30]

Please keep these as ints, I don't want to have to worry about size_t
comparisons.

[Sidney San Martín, 2016/08/03 14:35:32]

Done. For future reference, what issues come up around this? I changed it to get
rid of some casts, and the style guide suggests using size_t to represent
indices.

[sky, 2016/08/03 15:16:07]

My concern is around overflow and special casing -1.

[Sidney San Martín, 2016/08/04 04:24:57]

Done.

 
     // The ID of the browser to which this tab belonged, so it can be restored
     // there. May be 0 (an invalid SessionID) when restoring an entire session.
-    SessionID::id_type browser_id;
+    SessionID::id_type browser_id{0};
 
     // Index within the tab strip. May be -1 for an unknown index.
-    int tabstrip_index;
+    size_t tabstrip_index{-1};
 
     // True if the tab was pinned.
-    bool pinned;
+    bool pinned{false};
 
     // If non-empty gives the id of the extension for the tab.
     std::string extension_app_id;
 
     // The associated client data.
-    std::unique_ptr<PlatformSpecificTabData> platform_data;
+    std::unique_ptr<const PlatformSpecificTabData> platform_data;
 
     // The user agent override used for the tab's navigations (if applicable).
     std::string user_agent_override;
@@ -112,18 +108,19 @@ class SESSIONS_EXPORT TabRestoreService : public KeyedService {
   struct SESSIONS_EXPORT Window : public Entry {
     Window();
     ~Window() override;
+    Type type() const override;
 
     // The tabs that comprised the window, in order.
-    std::vector<Tab> tabs;
+    std::vector<std::unique_ptr<Tab>> tabs;
 
     // Index of the selected tab.
-    int selected_tab_index;
+    size_t selected_tab_index{-1};
 
     // If an application window, the name of the app.
     std::string app_name;
   };
 
-  typedef std::list<Entry*> Entries;
+  typedef std::list<std::unique_ptr<Entry>> Entries;
 
   ~TabRestoreService() override;
 
@@ -134,7 +131,7 @@ class SESSIONS_EXPORT TabRestoreService : public KeyedService {
 
   // Creates a Tab to represent |live_tab| and notifies observers the list of
   // entries has changed.
-  virtual void CreateHistoricalTab(LiveTab* live_tab, int index) = 0;
+  virtual void CreateHistoricalTab(LiveTab* live_tab, size_t index) = 0;
 
   // TODO(blundell): Rename and fix comment.
   // Invoked when a browser is closing. If |context| is a tabbed browser with
@@ -160,9 +157,8 @@ class SESSIONS_EXPORT TabRestoreService : public KeyedService {
   virtual std::vector<LiveTab*> RestoreMostRecentEntry(
       LiveTabContext* context) = 0;
 
-  // Removes the Tab with id |id| from the list and returns it; ownership is
-  // passed to the caller.
-  virtual Tab* RemoveTabEntryById(SessionID::id_type id) = 0;
+  // Removes the Tab with id |id| from the list and returns it.
+  virtual std::unique_ptr<Tab> RemoveTabEntryById(SessionID::id_type id) = 0;
 
   // Restores an entry by id. If there is no entry with an id matching |id|,
   // this does nothing. If |context| is NULL, this creates a new window for the
@@ -188,16 +184,9 @@ class SESSIONS_EXPORT TabRestoreService : public KeyedService {
 
 // A class that is used to associate platform-specific data with
 // TabRestoreService::Tab. See LiveTab::GetPlatformSpecificTabData().
-// Subclasses of this class must be copyable by implementing the Clone() method
-// for usage by the Tab struct, which is itself copyable and assignable.
 class SESSIONS_EXPORT PlatformSpecificTabData {
  public:
   virtual ~PlatformSpecificTabData();
-
- private:
-  friend TabRestoreService::Tab;
-
-  virtual std::unique_ptr<PlatformSpecificTabData> Clone() = 0;
 };
 
 }  // namespace sessions