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

components/sessions/core/tab_restore_service.h

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_SESSIONS_CORE_TAB_RESTORE_SERVICE_H_
 #define COMPONENTS_SESSIONS_CORE_TAB_RESTORE_SERVICE_H_
 
 #include <list>
 #include <string>
 #include <vector>
@@ skipping 35 matching lines @@
   };
 
   // The type of entry.
   enum Type {
     TAB,
     WINDOW
   };
 
   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;
   };
 
   // Represents a previously open window.
   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;
 
   // Adds/removes an observer. TabRestoreService does not take ownership of
   // the observer.
   virtual void AddObserver(TabRestoreServiceObserver* observer) = 0;
   virtual void RemoveObserver(TabRestoreServiceObserver* observer) = 0;
 
   // 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
   // at least one tab, a Window is created, added to entries and observers are
   // notified.
   virtual void BrowserClosing(LiveTabContext* context) = 0;
 
   // TODO(blundell): Rename and fix comment.
   // Invoked when the browser is done closing.
   virtual void BrowserClosed(LiveTabContext* context) = 0;
 
   // Removes all entries from the list and notifies observers the list
   // of tabs has changed.
   virtual void ClearEntries() = 0;
 
   // Returns the entries, ordered with most recently closed entries at the
   // front.
   virtual const Entries& entries() const = 0;
 
   // Restores the most recently closed entry. Does nothing if there are no
   // entries to restore. If the most recently restored entry is a tab, it is
   // added to |context|. Returns the LiveTab instances of the restored tab(s).
   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
   // entry. |disposition| is respected, but the attributes (tabstrip index,
   // browser window) of the tab when it was closed will be respected if
   // disposition is UNKNOWN. Returns the LiveTab instances of the restored
   // tab(s).
   virtual std::vector<LiveTab*> RestoreEntryById(
       LiveTabContext* context,
       SessionID::id_type id,
       WindowOpenDisposition disposition) = 0;
 
   // Loads the tabs and previous session. This does nothing if the tabs
   // from the previous session have already been loaded.
   virtual void LoadTabsFromLastSession() = 0;
 
   // Returns true if the tab entries have been loaded.
   virtual bool IsLoaded() const = 0;
 
   // Deletes the last session.
   virtual void DeleteLastSession() = 0;
 };
 
 // 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
 
 #endif  // COMPONENTS_SESSIONS_CORE_TAB_RESTORE_SERVICE_H_