[Autofill Assistant] Proposed Refactor of ElementFinder::Result

This changes the |ElementFinder::Result| to be split into different
data chunks.

This refactoring is done for the ElementStore, which is supposed
to only store the |ResultData| part (instead of the full |Result|).

Bug: b/172328014
Change-Id: I9e0f6aea5b5619feb6e326f6262e0ceaf8176d74
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2519458
Commit-Queue: Sandro Maggi <sandromaggi@google.com>
Reviewed-by: Clemens Arbesser <arbesser@google.com>
Reviewed-by: Stephane Zermatten <szermatt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827249}

components/autofill_assistant/browser/BUILD.gn

@@ -211,6 +211,8 @@ static_library("browser") {
     "wait_for_document_operation.h",
     "web/check_on_top_worker.cc",
     "web/check_on_top_worker.h",
+    "web/element.cc",
+    "web/element.h",
     "web/element_finder.cc",
     "web/element_finder.h",
     "web/element_position_getter.cc",

components/autofill_assistant/browser/actions/action_test_utils.cc

@@ -32,12 +32,14 @@ ElementFinder::Result MockFindElement(MockActionDelegate& delegate,
       .Times(times)
       .WillRepeatedly(WithArgs<1>([&selector](auto&& callback) {
         auto element_result = std::make_unique<ElementFinder::Result>();
-        element_result->object_id = selector.proto.filters(0).css_selector();
+        element_result->dom_object.object_data.object_id =
+            selector.proto.filters(0).css_selector();
         std::move(callback).Run(OkClientStatus(), std::move(element_result));
       }));
 
   ElementFinder::Result expected_result;
-  expected_result.object_id = selector.proto.filters(0).css_selector();
+  expected_result.dom_object.object_data.object_id =
+      selector.proto.filters(0).css_selector();
   return expected_result;
 }
 
@@ -56,12 +58,14 @@ ElementFinder::Result MockFindElement(MockWebController& web_controller,
       .Times(times)
       .WillRepeatedly(WithArgs<1>([&selector](auto&& callback) {
         auto element_result = std::make_unique<ElementFinder::Result>();
-        element_result->object_id = selector.proto.filters(0).css_selector();
+        element_result->dom_object.object_data.object_id =
+            selector.proto.filters(0).css_selector();
         std::move(callback).Run(OkClientStatus(), std::move(element_result));
       }));
 
   ElementFinder::Result expected_result;
-  expected_result.object_id = selector.proto.filters(0).css_selector();
+  expected_result.dom_object.object_data.object_id =
+      selector.proto.filters(0).css_selector();
   return expected_result;
 }
 

components/autofill_assistant/browser/actions/action_test_utils.h

@@ -14,7 +14,7 @@
 namespace autofill_assistant {
 
 MATCHER_P(EqualsElement, element, "") {
-  return arg.object_id == element.object_id;
+  return arg.object_id() == element.object_id();
 }
 
 MATCHER_P(EqualsStatus, status, "") {

components/autofill_assistant/browser/actions/upload_dom_action_unittest.cc

@@ -140,12 +140,12 @@ TEST_F(UploadDomActionTest, MultipleDomUpload) {
   EXPECT_CALL(mock_action_delegate_, FindAllElements(selector, _))
       .WillOnce(testing::WithArgs<1>([](auto&& callback) {
         auto element_result = std::make_unique<ElementFinder::Result>();
-        element_result->object_id = "fake_object_id";
+        element_result->dom_object.object_data.object_id = "fake_object_id";
         std::move(callback).Run(OkClientStatus(), std::move(element_result));
       }));
 
   ElementFinder::Result expected_result;
-  expected_result.object_id = "fake_object_id";
+  expected_result.dom_object.object_data.object_id = "fake_object_id";
 
   std::vector<std::string> fake_htmls{"<div></div>", "<span></span>"};
   EXPECT_CALL(mock_action_delegate_,

components/autofill_assistant/browser/web/check_on_top_worker.cc

@@ -31,11 +31,11 @@ void CheckOnTopWorker::Start(const ElementFinder::Result& element,
   js_snippet.AddLine("}");
   std::string function = js_snippet.ToString();
 
-  pending_result_count_ = element.frame_stack.size() + 1;
-  for (const auto& frame : element.frame_stack) {
+  pending_result_count_ = element.frame_stack().size() + 1;
+  for (const auto& frame : element.frame_stack()) {
     CallFunctionOn(function, frame.node_frame_id, frame.object_id);
   }
-  CallFunctionOn(function, element.node_frame_id, element.object_id);
+  CallFunctionOn(function, element.node_frame_id(), element.object_id());
 }
 
 void CheckOnTopWorker::CallFunctionOn(const std::string& function,

components/autofill_assistant/browser/web/element.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/autofill_assistant/browser/web/element.h"
+
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/web_contents.h"
+
+namespace autofill_assistant {
+
+DomObjectFrameStack::DomObjectFrameStack() = default;
+
+DomObjectFrameStack::~DomObjectFrameStack() = default;
+
+DomObjectFrameStack::DomObjectFrameStack(const DomObjectFrameStack&) = default;
+
+content::RenderFrameHost* FindCorrespondingRenderFrameHost(
+    const std::string& frame_id,
+    content::WebContents* web_contents) {
+  const auto& all_frames = web_contents->GetAllFrames();
+  const auto& it = std::find_if(
+      all_frames.begin(), all_frames.end(), [&](const auto& frame) {
+        return frame->GetDevToolsFrameToken().ToString() == frame_id;
+      });
+  if (it == all_frames.end()) {
+    return nullptr;
+  }
+  return *it;
+}
+
+}  // namespace autofill_assistant

components/autofill_assistant/browser/web/element.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_WEB_ELEMENT_H_
+#define COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_WEB_ELEMENT_H_
+
+#include <string>
+#include <vector>
+
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/web_contents.h"
+
+namespace autofill_assistant {
+
+// JsObjectIdentifier contains all data required to address an object in a
+// JavaScript context via Devtools.
+struct JsObjectIdentifier {
+  // The object id in the JavaScript context. This can be a JavaScript node
+  // instance or an array of such.
+  std::string object_id;
+
+  // The frame id to use to execute devtools Javascript calls within the
+  // context of the frame. Might be empty if no frame id needs to be
+  // specified.
+  std::string node_frame_id;
+};
+
+// DomObjectFrameStack contains all data required to use an object including
+// its nesting in frames information.
+struct DomObjectFrameStack {
+  DomObjectFrameStack();
+  ~DomObjectFrameStack();
+  DomObjectFrameStack(const DomObjectFrameStack&);
+
+  // The data for the final object.
+  JsObjectIdentifier object_data;
+
+  // This holds the information of all the frames that were traversed until
+  // the final element was reached.
+  std::vector<JsObjectIdentifier> frame_stack;
+};
+
+// Find the frame host in the set of known frames matching the |frame_id|. This
+// returns nullptr if no frame is found.
+content::RenderFrameHost* FindCorrespondingRenderFrameHost(
+    const std::string& frame_id,
+    content::WebContents* web_contents);
+
+}  // namespace autofill_assistant
+
+#endif  // COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_WEB_ELEMENT_H_

components/autofill_assistant/browser/web/element_finder.cc

@@ -6,6 +6,7 @@
 
 #include "components/autofill_assistant/browser/devtools/devtools_client.h"
 #include "components/autofill_assistant/browser/service.pb.h"
+#include "components/autofill_assistant/browser/web/element.h"
 #include "components/autofill_assistant/browser/web/web_controller_util.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
@@ -324,15 +325,15 @@ void ElementFinder::SendSuccessResult(const std::string& object_id) {
   // Fill in result and return
   std::unique_ptr<Result> result =
       std::make_unique<Result>(BuildResult(object_id));
-  result->frame_stack = frame_stack_;
+  result->dom_object.frame_stack = frame_stack_;
   std::move(callback_).Run(OkClientStatus(), std::move(result));
 }
 
 ElementFinder::Result ElementFinder::BuildResult(const std::string& object_id) {
   Result result;
   result.container_frame_host = current_frame_;
-  result.object_id = object_id;
-  result.node_frame_id = current_frame_id_;
+  result.dom_object.object_data.object_id = object_id;
+  result.dom_object.object_data.node_frame_id = current_frame_id_;
   return result;
 }
 
@@ -735,9 +736,10 @@ void ElementFinder::OnDescribeNodeForFrame(
   if (node->GetNodeName() == "IFRAME") {
     DCHECK(node->HasFrameId());  // Ensure all frames have an id.
 
-    frame_stack_.push_back(BuildResult(object_id));
+    frame_stack_.push_back({object_id, current_frame_id_});
 
-    auto* frame = FindCorrespondingRenderFrameHost(node->GetFrameId());
+    auto* frame =
+        FindCorrespondingRenderFrameHost(node->GetFrameId(), web_contents_);
     if (!frame) {
       VLOG(1) << __func__ << " Failed to find corresponding owner frame.";
       SendResult(ClientStatus(FRAME_HOST_NOT_FOUND));
@@ -801,17 +803,6 @@ void ElementFinder::OnResolveNode(
   ExecuteNextTask();
 }
 
-content::RenderFrameHost* ElementFinder::FindCorrespondingRenderFrameHost(
-    std::string frame_id) {
-  for (auto* frame : web_contents_->GetAllFrames()) {
-    if (frame->GetDevToolsFrameToken().ToString() == frame_id) {
-      return frame;
-    }
-  }
-
-  return nullptr;
-}
-
 void ElementFinder::ApplyProximityFilter(int filter_index,
                                          const std::string& array_object_id) {
   Selector target_selector;
@@ -922,7 +913,7 @@ void ElementFinder::OnProximityFilterTarget(int filter_index,
 })");
 
   std::vector<std::unique_ptr<runtime::CallArgument>> arguments;
-  AddRuntimeCallArgumentObjectId(result->object_id, &arguments);
+  AddRuntimeCallArgumentObjectId(result->object_id(), &arguments);
   AddRuntimeCallArgument(filter.max_pairs(), &arguments);
 
   devtools_client_->GetRuntime()->CallFunctionOn(

components/autofill_assistant/browser/web/element_finder.h

@@ -17,6 +17,7 @@
 #include "components/autofill_assistant/browser/devtools/devtools/domains/types_runtime.h"
 #include "components/autofill_assistant/browser/devtools/devtools_client.h"
 #include "components/autofill_assistant/browser/selector.h"
+#include "components/autofill_assistant/browser/web/element.h"
 #include "components/autofill_assistant/browser/web/js_snippets.h"
 #include "components/autofill_assistant/browser/web/web_controller_worker.h"
 
@@ -48,23 +49,29 @@ class ElementFinder : public WebControllerWorker {
     kMatchArray,
   };
 
+  // Result is the fully resolved element that can be used without limitations.
+  // This means that |container_frame_host| has been found and is not nullptr.
   struct Result {
     Result();
     ~Result();
     Result(const Result&);
 
+    DomObjectFrameStack dom_object;
+
     // The render frame host contains the element.
     content::RenderFrameHost* container_frame_host = nullptr;
 
-    // The object id of the element.
-    std::string object_id;
+    const std::string& object_id() const {
+      return dom_object.object_data.object_id;
+    }
 
-    // The frame id to use to execute devtools Javascript calls within the
-    // context of the frame. Might be empty if no frame id needs to be
-    // specified.
-    std::string node_frame_id;
+    const std::string& node_frame_id() const {
+      return dom_object.object_data.node_frame_id;
+    }
 
-    std::vector<Result> frame_stack;
+    const std::vector<JsObjectIdentifier>& frame_stack() const {
+      return dom_object.frame_stack;
+    }
   };
 
   // |web_contents| and |devtools_client| must be valid for the lifetime of the
@@ -294,8 +301,6 @@ class ElementFinder : public WebControllerWorker {
                               std::unique_ptr<dom::DescribeNodeResult> result);
   void OnResolveNode(const DevtoolsClient::ReplyStatus& reply_status,
                      std::unique_ptr<dom::ResolveNodeResult> result);
-  content::RenderFrameHost* FindCorrespondingRenderFrameHost(
-      std::string frame_id);
 
   // Handle TaskType::PROXIMITY
   void ApplyProximityFilter(int filter_index,
@@ -354,7 +359,7 @@ class ElementFinder : public WebControllerWorker {
   // elements matched by task i, or nullptr if the task is still running.
   std::vector<std::unique_ptr<std::vector<std::string>>> tasks_results_;
 
-  std::vector<Result> frame_stack_;
+  std::vector<JsObjectIdentifier> frame_stack_;
 
   // Finder for the target of the current proximity filter.
   std::unique_ptr<ElementFinder> proximity_target_filter_;

components/autofill_assistant/browser/web/element_rect_getter.cc

@@ -46,12 +46,12 @@ void ElementRectGetter::GetBoundingClientRect(
     ElementRectCallback callback) {
   std::string object_id;
   std::string node_frame_id;
-  if (index < element->frame_stack.size()) {
-    object_id = element->frame_stack[index].object_id;
-    node_frame_id = element->frame_stack[index].node_frame_id;
+  if (index < element->frame_stack().size()) {
+    object_id = element->frame_stack()[index].object_id;
+    node_frame_id = element->frame_stack()[index].node_frame_id;
   } else {
-    object_id = element->object_id;
-    node_frame_id = element->node_frame_id;
+    object_id = element->object_id();
+    node_frame_id = element->node_frame_id();
   }
 
   std::vector<std::unique_ptr<runtime::CallArgument>> arguments;
@@ -109,7 +109,7 @@ void ElementRectGetter::OnGetClientRectResult(
     rect.bottom = std::min(stacked_rect.bottom, stacked_rect.top + rect.bottom);
   }
 
-  if (index >= element->frame_stack.size()) {
+  if (index >= element->frame_stack().size()) {
     std::move(callback).Run(OkClientStatus(), rect);
     return;
   }

components/autofill_assistant/browser/web/element_rect_getter.h

@@ -67,4 +67,4 @@ class ElementRectGetter : public WebControllerWorker {
 
 }  // namespace autofill_assistant
 
-#endif  // COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_WEB_ELEMENT_POSITION_GETTER_H_
+#endif  // COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_WEB_ELEMENT_RECT_GETTER_H_

components/autofill_assistant/browser/web/web_controller_browsertest.cc

@@ -524,7 +524,7 @@ class WebControllerBrowserTest : public content::ContentBrowserTest,
     ElementFinder::Result result;
     FindElement(selector, &status, &result);
     EXPECT_EQ(ELEMENT_RESOLUTION_FAILED, status.proto_status());
-    EXPECT_THAT(result.object_id, IsEmpty());
+    EXPECT_THAT(result.object_id(), IsEmpty());
   }
 
   void CheckFindElementResult(const ElementFinder::Result& result,
@@ -532,13 +532,13 @@ class WebControllerBrowserTest : public content::ContentBrowserTest,
     if (is_main_frame) {
       EXPECT_EQ(shell()->web_contents()->GetMainFrame(),
                 result.container_frame_host);
-      EXPECT_EQ(result.frame_stack.size(), 0u);
+      EXPECT_EQ(result.frame_stack().size(), 0u);
     } else {
       EXPECT_NE(shell()->web_contents()->GetMainFrame(),
                 result.container_frame_host);
-      EXPECT_GE(result.frame_stack.size(), 1u);
+      EXPECT_GE(result.frame_stack().size(), 1u);
     }
-    EXPECT_FALSE(result.object_id.empty());
+    EXPECT_FALSE(result.object_id().empty());
   }
 
   ClientStatus GetStringAttribute(const Selector& selector,