Try harder to make sure that blink::FrameTree::m_uniqueName is truly unique.

third_party/WebKit/Source/core/page/FrameTree.h

Index: third_party/WebKit/Source/core/page/FrameTree.h
diff --git a/third_party/WebKit/Source/core/page/FrameTree.h b/third_party/WebKit/Source/core/page/FrameTree.h
index 2314a451403eb493881dd1b741c4d27b4f07d5fc..90155f1d65a5395d0ce79c1ad742702a3d4a562b 100644
--- a/third_party/WebKit/Source/core/page/FrameTree.h
+++ b/third_party/WebKit/Source/core/page/FrameTree.h
@@ -37,10 +37,17 @@ public:
     ~FrameTree();
 
     const AtomicString& name() const { return m_name; }
+    void setName(const AtomicString&);
+
+    // Unique name of a frame (unique per page).  Mainly used to identify the
+    // frame for session history purposes, but also used in expected results
+    // of layout tests.
+    //
+    // The value should be treated (at least outside of FrameTree.cpp) as an

[dcheng, 2016/05/31 19:57:47]

I would just condense this down to "The value should be treated as an
unstructured, opaque string."

[Łukasz Anforowicz, 2016/05/31 23:50:20]

Done.  Let me know if you think I should also remove pointer to the comment
describing internal implementation details (i.e. remove the last 2 lines of the
comment here).

[dcheng, 2016/06/01 20:58:11]

I think it makes sense to remove that part of the comment as well (it's
generally implied)

[Łukasz Anforowicz, 2016/06/01 22:31:28]

Done.

+    // unstructured, opaque string.  The implementation details, including
+    // the format description can be found in FrameTree.cpp, in a comment
+    // inside calculateUniqueNameForChildFrame method.
     const AtomicString& uniqueName() const { return m_uniqueName; }
-    // If |name| is not empty, |fallbackName| is ignored. Otherwise,
-    // |fallbackName| is used as a source of uniqueName.
-    void setName(const AtomicString& name, const AtomicString& fallbackName = nullAtom);
 
     // Directly assigns both the name and uniqueName.  Can be used when
     // |uniqueName| is already known (i.e. when it has been precalculated by
@@ -77,11 +84,39 @@ public:
 
 private:
     Frame* deepLastChild() const;
+
+    // Returns true if one of frames in the tree already has unique name equal
+    // to |uniqueNameCandidate|.
+    bool uniqueNameExists(const AtomicString& uniqueNameCandidate) const;
+
+    // Generates a hopefully-but-not-necessarily unique name based on frame's
+    // relative position in the tree and on unique names of ancestors.
+    AtomicString generateOldStyleMaybeUniqueName(bool existingChildFrame) const;

[dcheng, 2016/05/31 19:57:47]

Nit: generateUniqueNameCandidate?

[Łukasz Anforowicz, 2016/05/31 23:50:20]

Done.

+
+    // Generates a hopefully-but-not-necessarily unique suffix based on |child|
+    // absolute position in the tree.  If |child| is nullptr, calculations are
+    // made for a position that a new child of |this| would have.
+    String generateLikelyUniqueSuffix(Frame* child) const;

[dcheng, 2016/05/31 19:57:47]

Nit: generateFramePosition?

[Łukasz Anforowicz, 2016/05/31 23:50:20]

Done.

+
+    // Returns a unique name created by appending a suffix to |notYetUniqueName|.
+    // The appended suffix is based on |likelyUniqueSuffix|, but can also
+    // be extended to fulfill the uniqueness requirement.
+    AtomicString ensureUniquenessOfUniqueName(

[dcheng, 2016/05/31 19:57:47]

Nit: generateUniqueNameFromCandidate

[Łukasz Anforowicz, 2016/05/31 23:50:20]

How about
    // Concatenates |prefix|, |likelyUniqueSuffix| (and additional, internally
    // generated suffix) until the result is a unique name, that doesn't exist
    // elsewhere in the frame tree.  Returns the unique name built in this way.
    AtomicString appendUniqueSuffix(
        const AtomicString& prefix,
        const String& likelyUniqueSuffix) const;
?

+        const AtomicString& notYetUniqueName,

[dcheng, 2016/05/31 19:57:47]

Nit: candidate

[Łukasz Anforowicz, 2016/05/31 23:50:20]

How about |prefix| (we use |uniqueNameCandidate| as a variable name inside the
method's implementation, so |candidate| is a bit confusing).

+        const String& likelyUniqueSuffix) const;
+
+    // Calculates a unique name for |child| frame (which might be nullptr if the
+    // child has not yet been created - i.e. when we need unique name for a new
+    // frame).  Tries to use the |assignedName| or |fallbackName| if possible,
+    // otherwise falls back to generating a deterministic,
+    // stable-across-page-reloads string based on |child| position in the tree.
     AtomicString calculateUniqueNameForChildFrame(
-        bool existingChildFrame,
-        const AtomicString& name,
+        Frame* child,
+        const AtomicString& assignedName,
         const AtomicString& fallbackName = nullAtom) const;
-    bool uniqueNameExists(const AtomicString& name) const;
+
+    // Sets |m_uniqueName| and asserts its uniqueness.
+    void setUniqueName(const AtomicString&);
 
     Member<Frame> m_thisFrame;