Revamp TabNavigation class

chrome/browser/sessions/session_types.h

Index: chrome/browser/sessions/session_types.h
diff --git a/chrome/browser/sessions/session_types.h b/chrome/browser/sessions/session_types.h
index 0c3335b358f2ee251b6765b32b4c22c3b3793286..6d041be08708501e1e84936b2d0169c274a5955b 100644
--- a/chrome/browser/sessions/session_types.h
+++ b/chrome/browser/sessions/session_types.h
@@ -8,119 +8,148 @@
 #include <string>
 #include <vector>
 
-#include "base/stl_util.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/string16.h"
 #include "base/time.h"
 #include "chrome/browser/sessions/session_id.h"
 #include "content/public/common/page_transition_types.h"
 #include "content/public/common/referrer.h"
 #include "googleurl/src/gurl.h"
+#include "sync/protocol/session_specifics.pb.h"
 #include "ui/base/ui_base_types.h"
 #include "ui/gfx/rect.h"
 
-class Profile;
+class Pickle;
+class PickleIterator;
 
 namespace content {
+class BrowserContext;
 class NavigationEntry;
 }
 
 // TabNavigation  -------------------------------------------------------------
 
-// TabNavigation corresponds to the parts of NavigationEntry needed to restore
-// the NavigationEntry during session restore and tab restore.
-//
-// TabNavigation is cheap and supports copy semantics.
+// TabNavigation is a "freeze-dried" version of NavigationEntry.  It
+// contains the data needed to restore a NavigationEntry during
+// session restore and tab restore, and it can also be pickled and
+// unpickled.  It is also convertible to a sync protocol buffer for
+// session syncing.
 class TabNavigation {
  public:
-  enum TypeMask {
-    HAS_POST_DATA = 1
-  };
-
+  // Creates an invalid (index < 0) TabNavigation.
   TabNavigation();
-  TabNavigation(int index,
-                const GURL& virtual_url,
-                const content::Referrer& referrer,
-                const string16& title,
-                const std::string& state,
-                content::PageTransition transition);
-  TabNavigation(const TabNavigation& tab);
-  virtual ~TabNavigation();
-  TabNavigation& operator=(const TabNavigation& tab);
-
-  // Converts this TabNavigation into a NavigationEntry with a page id of
-  // |page_id|. The caller owns the returned NavigationEntry.
-  content::NavigationEntry* ToNavigationEntry(int page_id,
-                                              Profile* profile) const;
-
-  // Resets this TabNavigation from |entry|.
-  void SetFromNavigationEntry(const content::NavigationEntry& entry);
-
-  // Virtual URL of the page. See NavigationEntry::GetVirtualURL() for details.
-  void set_virtual_url(const GURL& url) { virtual_url_ = url; }
-  const GURL& virtual_url() const { return virtual_url_; }
+  ~TabNavigation();
 
-  // The referrer.
-  const content::Referrer& referrer() const { return referrer_; }
+  // Default copy constructor and assignment operator welcome.

[sky, 2012/09/17 14:03:36]

Move this back to class description.

[akalin, 2012/09/17 23:24:32]

Done.

+
+  // Construct a TabNavigation for a particular index from a
+  // NavigationEntry with the given timestamp.
+  //
+  // TODO(akalin): Add a timestamp field to
+  // navigation::NavigationEntry and use that instead of passing a
+  // separate timestamp.
+  static TabNavigation FromNavigationEntry(
+      int index,
+      const content::NavigationEntry& entry,
+      base::Time timestamp);
+
+  // Construct a TabNavigation for a particular index from a sync
+  // protocol buffer.  Note that the sync protocol buffer doesn't
+  // contain all TabNavigation fields.
+  static TabNavigation FromSyncData(
+      int index,
+      const sync_pb::TabNavigation& specifics);
+
+  // Note that not all TabNavigation fields are preserved.
+  void WriteToPickle(Pickle* pickle) const;
+  bool ReadFromPickle(PickleIterator* iterator);
+
+  // Convert this TabNavigation into a NavigationEntry with the given
+  // page ID and context.  The NavigationEntry will have a transition
+  // type of PAGE_TRANSITION_RELOAD and a new unique ID.
+  scoped_ptr<content::NavigationEntry> ToNavigationEntry(
+      int page_id,
+      content::BrowserContext* browser_context) const;
+
+  // Convert this navigation into its sync protocol buffer equivalent.
+  // Note that the protocol buffer doesn't contain all TabNavigation
+  // fields.
+  sync_pb::TabNavigation ToSyncData() const;
+
+  // The index in the NavigationController. This TabNavigation is
+  // valid only when the index is non-negative.
+  //
+  // This is used when determining the selected TabNavigation and only
+  // used by SessionService.
+  int index() const { return index_; }
+  void set_index(int index) { index_ = index; }
 
-  // The title of the page.
-  void set_title(const string16& title) { title_ = title; }
+  // Accessors for some fields taken from NavigationEntry.
+  int unique_id() const { return unique_id_; }
+  const GURL& virtual_url() const { return virtual_url_; }
   const string16& title() const { return title_; }
+  const std::string& content_state() const { return content_state_; }
 
-  // State bits.
-  const std::string& state() const { return state_; }
+  // Timestamp this navigation occurred.
+  base::Time timestamp() const { return timestamp_; }
 
-  // Transition type.
-  void set_transition(content::PageTransition transition) {
-    transition_ = transition;
-  }
-  content::PageTransition transition() const { return transition_; }
+  // Converts a set of TabNavigations into a list of NavigationEntrys
+  // with sequential page IDs and the given context. The caller owns
+  // the returned NavigationEntrys.
+  static std::vector<content::NavigationEntry*>
+  CreateNavigationEntriesFromTabNavigations(
+      const std::vector<TabNavigation>& navigations,
+      content::BrowserContext* browser_context);
 
-  // A mask used for arbitrary boolean values needed to represent a
-  // NavigationEntry. Currently only contains HAS_POST_DATA or 0.
-  void set_type_mask(int type_mask) { type_mask_ = type_mask; }
-  int type_mask() const { return type_mask_; }
+  // Functions for testing.

[sky, 2012/09/17 14:03:36]

I don't like all the for test foo. They clutter up the class, may be
inadvertently used and may increase compile time. Instead create a class that is
only compiled when testing that is friended, takes a TabNavigation and does all
this.

[akalin, 2012/09/17 23:24:32]

Done, moved to SessionTypesTestHelper.

 
-  // The index in the NavigationController. If this is -1, it means this
-  // TabNavigation is bogus.
-  //
-  // This is used when determining the selected TabNavigation and only useful
-  // by BaseSessionService and SessionService.
-  void set_index(int index) { index_ = index; }
-  int index() const { return index_; }
+  static TabNavigation CreateForTest(const std::string& virtual_url,
+                                     const std::string& title);
 
-  // The URL that initially spawned the NavigationEntry.
-  const GURL& original_request_url() const { return original_request_url_; }
-  void set_original_request_url(const GURL& url) {
-    original_request_url_ = url;
-  }
+  const content::Referrer& GetReferrerForTest() const;
 
-  // Whether or not we're overriding the standard user agent.
-  bool is_overriding_user_agent() const { return is_overriding_user_agent_; }
-  void set_is_overriding_user_agent(bool state) {
-    is_overriding_user_agent_ = state;
-  }
+  content::PageTransition GetTransitionTypeForTest() const;
 
-  // Converts a set of TabNavigations into a set of NavigationEntrys. The
-  // caller owns the NavigationEntrys.
-  static void CreateNavigationEntriesFromTabNavigations(
-      Profile* profile,
-      const std::vector<TabNavigation>& navigations,
-      std::vector<content::NavigationEntry*>* entries);
+  bool GetHasPostDataForTest() const;
+
+  int64 GetPostIdForTest() const;
+
+  const GURL& GetOriginalRequestURLForTest() const;
+
+  bool GetIsOverridingUserAgentForTest() const;
+
+  void SetContentStateForTest(const std::string& content_state);
+
+  void SetTransitionTypeForTest(content::PageTransition transition_type);
+
+  void SetHasPostDataForTest(bool has_post_data);
+
+  void SetOriginalRequestURLForTest(const GURL& original_request_url);
+
+  void SetIsOverridingUserAgentForTest(bool is_overriding_user_agent);
 
  private:
-  friend class BaseSessionService;
+  friend struct SessionTypesTestHelper;
 
-  GURL virtual_url_;
+  // Index in the NavigationController.
+  int index_;
+
+  // Member variables corresponding to NavigationEntry fields.
+  int unique_id_;

[sky, 2012/09/17 14:03:36]

Why do we now need the unique id?

[akalin, 2012/09/17 23:24:32]

Sync needs it to disambiguate navigations (even when we have timestamps, those
won't be enough).

   content::Referrer referrer_;
+  GURL virtual_url_;
   string16 title_;
-  std::string state_;
-  content::PageTransition transition_;
-  int type_mask_;
+  std::string content_state_;
+  content::PageTransition transition_type_;
+  bool has_post_data_;
   int64 post_id_;
-
-  int index_;
   GURL original_request_url_;
   bool is_overriding_user_agent_;
+
+  // Timestamp when the navigation occurred.
+  //
+  // TODO(akalin): Add a timestamp field to NavigationEntry.
+  base::Time timestamp_;

[sky, 2012/09/17 14:03:36]

Can we add the two at the same time?

[akalin, 2012/09/17 23:24:32]

NavigationEntry already has a unique ID field (although it's read-only).

 };
 
 // SessionTab ----------------------------------------------------------------