Refactor UniqueNameHelper to use an adapter pattern for code sharing.

content/common/unique_name_helper.cc

Index: content/common/unique_name_helper.cc
diff --git a/content/renderer/unique_name_helper.cc b/content/common/unique_name_helper.cc
similarity index 39%
rename from content/renderer/unique_name_helper.cc
rename to content/common/unique_name_helper.cc
index 481645cec9edaa003f35295ab7469ae81b47c1a8..7ae33829a53341169a59ce8e291e15b5550515fc 100644
--- a/content/renderer/unique_name_helper.cc
+++ b/content/common/unique_name_helper.cc
@@ -2,106 +2,87 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "content/renderer/unique_name_helper.h"
+#include "content/common/unique_name_helper.h"
 
-#include <vector>
-
-#include "base/containers/adapters.h"
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
-#include "base/strings/string_piece.h"
-#include "content/renderer/render_frame_impl.h"
-#include "content/renderer/render_frame_proxy.h"
-#include "third_party/WebKit/public/web/WebLocalFrame.h"
+#include "base/strings/string_util.h"
 
 namespace content {
 
 namespace {
 
-const std::string& UniqueNameForFrame(blink::WebFrame* frame) {
-  return frame->IsWebLocalFrame()
-             ? RenderFrameImpl::FromWebFrame(frame)->unique_name()
-             : RenderFrameProxy::FromWebFrame(frame)->unique_name();
-}
-
-bool UniqueNameExists(blink::WebFrame* top, const std::string& candidate) {
-  // This method is currently O(N), where N = number of frames in the tree.
+using FrameAdapter = UniqueNameHelper::FrameAdapter;
 
-  // Before recalculating or checking unique name, we set m_uniqueName
-  // to an empty string (so the soon-to-be-removed name does not count
-  // as a collision).  This means that uniqueNameExists would return
-  // false positives when called with an empty |name|.
-  DCHECK(!candidate.empty());
+class PendingChildFrameAdapter : public UniqueNameHelper::FrameAdapter {
+ public:
+  explicit PendingChildFrameAdapter(FrameAdapter* parent) : parent_(parent) {}
 
-  for (blink::WebFrame* frame = top; frame; frame = frame->TraverseNext()) {
-    if (UniqueNameForFrame(frame) == candidate)
-      return true;
+  // FrameAdapter overrides:
+  bool IsMainFrame() const override { return false; }
+  bool IsCandidateUnique(const std::string& name) const override {
+    return parent_->IsCandidateUnique(name);
+  }
+  int GetSiblingCount() const override { return parent_->GetChildCount(); }
+  int GetChildCount() const override {
+    NOTREACHED();
+    return 0;
+  }
+  std::vector<base::StringPiece> CollectAncestorNames(
+      BeginPoint begin_point,
+      bool (*should_stop)(base::StringPiece)) const override {
+    DCHECK_EQ(BeginPoint::kParentFrame, begin_point);
+    return parent_->CollectAncestorNames(BeginPoint::kThisFrame, should_stop);
+  }
+  std::vector<int> GetFramePosition(BeginPoint begin_point) const override {
+    DCHECK_EQ(BeginPoint::kParentFrame, begin_point);
+    return parent_->GetFramePosition(BeginPoint::kThisFrame);
   }
 
-  return false;
-}
-
-std::string GenerateCandidate(blink::WebFrame* parent, blink::WebFrame* child) {
-  constexpr char kFramePathPrefix[] = "<!--framePath ";
-  constexpr int kFramePathPrefixLength = 14;
-  constexpr int kFramePathSuffixLength = 3;
-
-  std::string new_name(kFramePathPrefix);
+ private:
+  FrameAdapter* const parent_;
+};
 
-  // Find the nearest parent that has a frame with a path in it.
-  std::vector<blink::WebFrame*> chain;
-  for (blink::WebFrame* frame = parent; frame; frame = frame->Parent()) {
-    base::StringPiece name = UniqueNameForFrame(frame);
-    if (name.starts_with(kFramePathPrefix) && name.ends_with("-->") &&
-        kFramePathPrefixLength + kFramePathSuffixLength < name.size()) {
-      name.substr(kFramePathPrefixLength,
-                  name.size() - kFramePathPrefixLength - kFramePathSuffixLength)
-          .AppendToString(&new_name);
-      break;
-    }
-    chain.push_back(frame);
-  }
+constexpr char kFramePathPrefix[] = "<!--framePath ";
+constexpr int kFramePathPrefixLength = 14;
+constexpr int kFramePathSuffixLength = 3;
 
-  for (auto* frame : base::Reversed(chain)) {
-    new_name += '/';
-    new_name += UniqueNameForFrame(frame);
-  }
+bool IsNameWithFramePath(base::StringPiece name) {
+  return name.starts_with(kFramePathPrefix) && name.ends_with("-->") &&
+         (kFramePathPrefixLength + kFramePathSuffixLength) < name.size();
+}
 
-  int child_count = 0;
-  for (blink::WebFrame* frame = parent->FirstChild(); frame;
-       frame = frame->NextSibling()) {
-    ++child_count;
+std::string GenerateCandidate(const FrameAdapter* frame) {
+  std::string new_name(kFramePathPrefix);
+  new_name += "/";
+  std::vector<base::StringPiece> ancestor_names = frame->CollectAncestorNames(
+      FrameAdapter::BeginPoint::kParentFrame, &IsNameWithFramePath);
+  // Note: This checks ancestor_names[0] twice, but it's nicer to do the name
+  // extraction here rather than passing another function pointer to
+  // CollectAncestorNames().
+  if (!ancestor_names.empty() && IsNameWithFramePath(ancestor_names[0])) {
+    ancestor_names[0] = ancestor_names[0].substr(kFramePathPrefixLength,
+                                                 ancestor_names[0].size() -
+                                                     kFramePathPrefixLength -
+                                                     kFramePathSuffixLength);
   }
+  new_name += base::JoinString(ancestor_names, "/");
 
-  // When generating a candidate, a null |child| means that this is the
-  // candidate name for a child frame that's not yet attached.
   new_name += "/<!--frame";
-  new_name += base::IntToString(child_count - (child ? 1 : 0));
+  new_name += base::IntToString(frame->GetSiblingCount());
   new_name += "-->-->";
 
   // NOTE: This name might not be unique - see http://crbug.com/588800.
   return new_name;
 }
 
-std::string GenerateFramePosition(blink::WebFrame* parent,
-                                  blink::WebFrame* child) {
-  // This method is currently O(N), where N = number of frames in the tree.
-
+std::string GenerateFramePosition(const FrameAdapter* frame) {
   std::string position_string("<!--framePosition");
-
-  while (parent) {
-    int position_in_parent = 0;
-    blink::WebFrame* sibling = parent->FirstChild();
-    while (sibling != child) {
-      sibling = sibling->NextSibling();
-      ++position_in_parent;
-    }
-
+  std::vector<int> positions =
+      frame->GetFramePosition(FrameAdapter::BeginPoint::kParentFrame);
+  for (int position : positions) {
     position_string += '-';
-    position_string += base::IntToString(position_in_parent);
-
-    child = parent;
-    parent = parent->Parent();
+    position_string += base::IntToString(position);
   }
 
   // NOTE: The generated string is not guaranteed to be unique, but should
@@ -113,12 +94,12 @@ std::string GenerateFramePosition(blink::WebFrame* parent,
   return position_string;
 }
 
-std::string AppendUniqueSuffix(blink::WebFrame* top,
+std::string AppendUniqueSuffix(const FrameAdapter* frame,
                                const std::string& prefix,
                                const std::string& likely_unique_suffix) {
   // This should only be called if the |prefix| isn't unique, as this is
   // otherwise pointless work.
-  DCHECK(UniqueNameExists(top, prefix));
+  DCHECK(!frame->IsCandidateUnique(prefix));
 
   // We want unique name to be stable across page reloads - this is why
   // we use a deterministic |number_of_tries| rather than a random number
@@ -135,53 +116,48 @@ std::string AppendUniqueSuffix(blink::WebFrame* top,
     size_t current_length = candidate.size();
     candidate += base::IntToString(number_of_retries++);
     candidate += "-->";
-    if (!UniqueNameExists(top, candidate))
+    if (frame->IsCandidateUnique(candidate))
       break;
     candidate.resize(current_length);
   }
   return candidate;
 }
 
-std::string CalculateNewName(blink::WebFrame* parent,
-                             blink::WebFrame* child,
+std::string CalculateNewName(const FrameAdapter* frame,
                              const std::string& name) {
-  blink::WebFrame* top = parent->Top();
-  if (!name.empty() && !UniqueNameExists(top, name) && name != "_blank")
+  if (!name.empty() && frame->IsCandidateUnique(name) && name != "_blank")
     return name;
 
-  std::string candidate = GenerateCandidate(parent, child);
-  if (!UniqueNameExists(top, candidate))
+  std::string candidate = GenerateCandidate(frame);
+  if (frame->IsCandidateUnique(candidate))
     return candidate;
 
-  std::string likely_unique_suffix = GenerateFramePosition(parent, child);
-  return AppendUniqueSuffix(top, candidate, likely_unique_suffix);
+  std::string likely_unique_suffix = GenerateFramePosition(frame);
+  return AppendUniqueSuffix(frame, candidate, likely_unique_suffix);
 }
 
 }  // namespace
 
-UniqueNameHelper::UniqueNameHelper(RenderFrameImpl* render_frame)
-    : render_frame_(render_frame) {}
+UniqueNameHelper::FrameAdapter::~FrameAdapter() {}
+
+UniqueNameHelper::UniqueNameHelper(FrameAdapter* frame) : frame_(frame) {}
 
 UniqueNameHelper::~UniqueNameHelper() {}
 
 std::string UniqueNameHelper::GenerateNameForNewChildFrame(
-    blink::WebFrame* parent,
-    const std::string& name) {
-  return CalculateNewName(parent, nullptr, name);
+    const std::string& name) const {
+  PendingChildFrameAdapter adapter(frame_);
+  return CalculateNewName(&adapter, name);
 }
 
 void UniqueNameHelper::UpdateName(const std::string& name) {
   // The unique name of the main frame is always the empty string.
-  if (!GetWebFrame()->Parent())
+  if (frame_->IsMainFrame())
     return;
   // It's important to clear this before calculating a new name, as the
   // calculation checks for collisions with existing unique names.
   unique_name_.clear();

[Łukasz Anforowicz, 2017/05/25 18:33:18]

I thought you said (*) that we still need to clear the old name.  Is the
deletion of the 3 lines above intentional?

(*)
https://codereview.chromium.org/2902253003/diff/20001/content/renderer/render...

[dcheng, 2017/05/25 18:40:41]

Doh, forgot to revert the unstaged changes when I used git commit -p. Thanks for
catching this.

-  unique_name_ = CalculateNewName(GetWebFrame()->Parent(), GetWebFrame(), name);
-}
-
-blink::WebLocalFrame* UniqueNameHelper::GetWebFrame() const {
-  return render_frame_->GetWebFrame();
+  unique_name_ = CalculateNewName(frame_, name);
 }
 
 }  // namespace content