Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(74)

Unified Diff: content/common/unique_name_helper.cc

Issue 2902253003: Refactor UniqueNameHelper to use an adapter pattern for code sharing. (Closed)
Patch Set: Fix off by one bug Created 3 years, 7 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « content/common/unique_name_helper.h ('k') | content/renderer/BUILD.gn » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
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..a6a135b7cd1f4efab070e27298811e2768d3aa9b 100644
--- a/content/renderer/unique_name_helper.cc
+++ b/content/common/unique_name_helper.cc
@@ -2,106 +2,94 @@
// 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 <algorithm>
-#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 {
+ // Note: no adjustment is required here: since this adapter is an internal
+ // helper, the parent FrameAdapter it delegates to won't know about this
+ // child to include it in the count.
+ 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 = 15;
+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);
+ std::vector<base::StringPiece> ancestor_names = frame->CollectAncestorNames(
+ FrameAdapter::BeginPoint::kParentFrame, &IsNameWithFramePath);
+ std::reverse(ancestor_names.begin(), ancestor_names.end());
+ // 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 +101,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 +123,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();
- unique_name_ = CalculateNewName(GetWebFrame()->Parent(), GetWebFrame(), name);
-}
-
-blink::WebLocalFrame* UniqueNameHelper::GetWebFrame() const {
- return render_frame_->GetWebFrame();
+ unique_name_ = CalculateNewName(frame_, name);
}
} // namespace content
« no previous file with comments | « content/common/unique_name_helper.h ('k') | content/renderer/BUILD.gn » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698