OOPIF: Add frame_unique_name to FrameNavigationEntry.

content/browser/frame_host/frame_navigation_entry.h

Index: content/browser/frame_host/frame_navigation_entry.h
diff --git a/content/browser/frame_host/frame_navigation_entry.h b/content/browser/frame_host/frame_navigation_entry.h
index d92a924fc855bd1fe64f1567b1cc661420a6dd81..6990cd727fbfcc9fbd10e35b870dd75804a21499 100644
--- a/content/browser/frame_host/frame_navigation_entry.h
+++ b/content/browser/frame_host/frame_navigation_entry.h
@@ -19,17 +19,16 @@ namespace content {
 // For now, it is owned by a single NavigationEntry and only tracks the main
 // frame.
 //
-// TODO(creis): In --site-per-process, fill in a tree of FrameNavigationEntries
-// in each NavigationEntry, one per frame.  FrameNavigationEntries may be shared
-// across NavigationEntries if the frame hasn't changed.
+// If SiteIsolationPolicy::UseSubframeNavigationEntries is true, there will be a
+// tree of FrameNavigationEntries in each NavigationEntry, one per frame.
+// TODO(creis): Share these FrameNavigationEntries across NavigationEntries if
+// the frame hasn't changed.
 class CONTENT_EXPORT FrameNavigationEntry
     : public base::RefCounted<FrameNavigationEntry> {
  public:
-  // TODO(creis): We should not use FTN IDs here, since they will change if you
-  // leave a page and come back later.  We should evaluate whether Blink's
-  // unique names would work instead, similar to HistoryNode.
   explicit FrameNavigationEntry(int frame_tree_node_id);
   FrameNavigationEntry(int frame_tree_node_id,
+                       std::string target,
                        int64 item_sequence_number,
                        int64 document_sequence_number,
                        SiteInstanceImpl* site_instance,
@@ -41,7 +40,8 @@ class CONTENT_EXPORT FrameNavigationEntry
   FrameNavigationEntry* Clone() const;
 
   // Updates all the members of this entry.
-  void UpdateEntry(int64 item_sequence_number,
+  void UpdateEntry(std::string target,
+                   int64 item_sequence_number,
                    int64 document_sequence_number,
                    SiteInstanceImpl* site_instance,
                    const GURL& url,
@@ -51,9 +51,17 @@ class CONTENT_EXPORT FrameNavigationEntry
   // The ID of the FrameTreeNode this entry is for.  -1 for the main frame,
   // since we don't always know the FrameTreeNode ID when creating the overall
   // NavigationEntry.
-  // TODO(creis): Replace with frame sequence number or unique name.
+  // TODO(creis): Consider removing |frame_tree_node_id| in favor of |target|,
+  // if we can move unique name computation to the browser process.
   int frame_tree_node_id() const { return frame_tree_node_id_; }
 
+  // The unique name of the frame this entry is for.  This is a stable name for
+  // the frame based on its position in the tree and relation to other named
+  // frames, which does not change after cross-process navigations or restores.
+  // Only the main frame can have an empty name.

[Avi (use Gerrit), 2015/11/09 20:58:26]

If this is a "unique name", why is this called "target" and not called "unique
name" or something like that?

[Charlie Reis, 2015/11/09 21:28:48]

The risk with putting a "unique name" on FrameNavigationEntry is that multiple
FrameNavigationEntries will have the same unique name, which could be confusing.
 Target was presumably chosen on blink::HistoryItem because it indicates which
frame the item is meant for.  I agree that "target" doesn't convey what approach
we're using to specify the frame, though.

I suppose we could call it unique_frame_name() to avoid implying that each FNE
has its own unique name.  Work for you?  If so, I'll upload a new patch.

[Avi (use Gerrit), 2015/11/09 21:30:58]

That sounds like it doesn't avoid what you were worried about. The "unique" in
the name... what's the universe of uniqueness? Is the name unique within the
page?

[Charlie Reis, 2015/11/09 21:38:41]

I think it does.  I was worried that it would be odd for two sequential FNEs for
the same frame to have the same "unique_name," but it makes sense for them to
have the same "unique_frame_name" since they target the same frame.
Yes, it's unique within a page (i.e., WebContents).  I can clarify that in the
comment.

Are you separately concerned that it implies the name is unique everywhere?  I'm
open to suggestions, but it'd be nice to avoid diverging too far from the
terminology in the renderer process.

[Avi (use Gerrit), 2015/11/09 21:44:39]

If you're OK with "unique frame name" then I'm OK with it. "Target" is so
meaningless IMO that it's not helpful at all. A comment about how it's unique is
good.

I know we don't want to diverge far, but "target" is so bad we should change it
in Blink too...

[Charlie Reis, 2015/11/09 21:49:24]

Nate, what would you think about changing blink::HistoryItem::target() to
something like uniqueFrameName()?  I agree with Avi that target() doesn't really
convey what it holds, and it takes some digging to find out.

We can update Blink separately, but we should choose a name here that could work
for both.

[Nate Chapin, 2015/11/09 21:52:23]

I'd prefer frameUniqueName() to uniqueFrameName(), because it is the
uniqueName() of the relevant frame, and I think putting frame in the middle
doesn't make that as clear. I could go either way though.

The name target() on HistoryItem is definitely dated, feel free to rename it
here or later.

[Charlie Reis, 2015/11/09 22:19:41]

Thanks.  I went with that, and we can update the other uses in Blink later.

+  std::string target() const { return target_; }
+  void set_target(std::string target) { target_ = target; }
+
   // Keeps track of where this entry belongs in the frame's session history.
   // The item sequence number identifies each stop in the back/forward history
   // and is globally unique.  The document sequence number increments for each
@@ -91,13 +99,14 @@ class CONTENT_EXPORT FrameNavigationEntry
   virtual ~FrameNavigationEntry();
 
   // WARNING WARNING WARNING WARNING WARNING WARNING WARNING WARNING WARNING
-  // For all new fields, update |Clone|.
+  // Add all new fields to |UpdateEntry|.
   // TODO(creis): These fields have implications for session restore.  This is
   // currently managed by NavigationEntry, but the logic will move here.
   // WARNING WARNING WARNING WARNING WARNING WARNING WARNING WARNING WARNING
 
   // See the accessors above for descriptions.
   int frame_tree_node_id_;
+  std::string target_;
   int64 item_sequence_number_;
   int64 document_sequence_number_;
   scoped_refptr<SiteInstanceImpl> site_instance_;