[Autofill Assistant] Refactor WaitForDocumentReadyState

This refactors WaitForDocumentReadyState (and GetDocument
ReadyState) to take an (optional) ElementFinder::Result
instead of an optional Selector.

An optional ElementFinder::Result is here defined as having
an empty node_frame_id (which will map to the main frame).

Bug: b/2523176
Change-Id: I7db89cc0ec2ede071616b30db42c1f8de9ab2d0b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2526441
Commit-Queue: Sandro Maggi <sandromaggi@google.com>
Reviewed-by: Mathias Carlen <mcarlen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#825710}

components/autofill_assistant/browser/actions/action_delegate.h

@@ -319,19 +319,19 @@ class ActionDelegate {
       base::OnceCallback<void(bool)> on_navigation_done) = 0;
 
   // Waits for the value of Document.readyState to reach at least
-  // |min_ready_state| in |optional_frame| or, if it is empty, in the main
-  // document.
+  // |min_ready_state| in |optional_frame_element| or, if it is empty, in the
+  // main document.
   virtual void WaitForDocumentReadyState(
-      const Selector& optional_frame,
       DocumentReadyState min_ready_state,
+      const ElementFinder::Result& optional_frame_element,
       base::OnceCallback<void(const ClientStatus&,
                               DocumentReadyState,
                               base::TimeDelta)> callback) = 0;
 
-  // Gets the value of Document.readyState in |optional_frame| or, if it is
-  // empty, in the main document.
+  // Gets the value of Document.readyState in |optional_frame_element| or, if
+  // it is empty, in the main document.
   virtual void GetDocumentReadyState(
-      const Selector& optional_frame,
+      const ElementFinder::Result& optional_frame_element,
       base::OnceCallback<void(const ClientStatus&, DocumentReadyState)>
           callback) = 0;
 

components/autofill_assistant/browser/actions/mock_action_delegate.h

@@ -298,30 +298,31 @@ class MockActionDelegate : public ActionDelegate {
 
   MOCK_METHOD2(
       OnGetDocumentReadyState,
-      void(const Selector&,
+      void(const ElementFinder::Result&,
            base::OnceCallback<void(const ClientStatus&, DocumentReadyState)>&));
 
   void GetDocumentReadyState(
-      const Selector& frame,
+      const ElementFinder::Result& optional_frame_element,
       base::OnceCallback<void(const ClientStatus&, DocumentReadyState)>
           callback) override {
-    OnGetDocumentReadyState(frame, callback);
+    OnGetDocumentReadyState(optional_frame_element, callback);
   }
 
   MOCK_METHOD3(OnWaitForDocumentReadyState,
-               void(const Selector&,
-                    DocumentReadyState min_ready_state,
+               void(DocumentReadyState,
+                    const ElementFinder::Result&,
                     base::OnceCallback<void(const ClientStatus&,
                                             DocumentReadyState,
                                             base::TimeDelta)>&));
 
   void WaitForDocumentReadyState(
-      const Selector& frame,
       DocumentReadyState min_ready_state,
+      const ElementFinder::Result& optional_frame_element,
       base::OnceCallback<void(const ClientStatus&,
                               DocumentReadyState,
                               base::TimeDelta)> callback) override {
-    OnWaitForDocumentReadyState(frame, min_ready_state, callback);
+    OnWaitForDocumentReadyState(min_ready_state, optional_frame_element,
+                                callback);
   }
 
   MOCK_METHOD0(RequireUI, void());

components/autofill_assistant/browser/actions/wait_for_document_action.cc

@@ -6,6 +6,7 @@
 
 #include "components/autofill_assistant/browser/actions/action_delegate.h"
 #include "components/autofill_assistant/browser/client_status.h"
+#include "components/autofill_assistant/browser/web/element_finder.h"
 
 namespace autofill_assistant {
 
@@ -21,29 +22,50 @@ void WaitForDocumentAction::InternalProcessAction(
     ProcessActionCallback callback) {
   callback_ = std::move(callback);
 
-  Selector selector(proto_.wait_for_document().frame());
-  if (selector.empty()) {
+  Selector frame_selector(proto_.wait_for_document().frame());
+  if (frame_selector.empty()) {
     // No element to wait for.
-    OnShortWaitForElement(ClientStatus(ACTION_APPLIED));
+    WaitForReadyState();
     return;
   }
   delegate_->ShortWaitForElement(
-      selector,
+      frame_selector,
       base::BindOnce(
           &WaitForDocumentAction::OnWaitForElementTimed,
           weak_ptr_factory_.GetWeakPtr(),
           base::BindOnce(&WaitForDocumentAction::OnShortWaitForElement,
-                         weak_ptr_factory_.GetWeakPtr())));
+                         weak_ptr_factory_.GetWeakPtr(), frame_selector)));
 }
 
 void WaitForDocumentAction::OnShortWaitForElement(
+    const Selector& frame_selector,
     const ClientStatus& element_status) {
   if (!element_status.ok()) {
     SendResult(element_status, DOCUMENT_UNKNOWN_READY_STATE);
     return;
   }
+
+  delegate_->FindElement(frame_selector,
+                         base::BindOnce(&WaitForDocumentAction::OnFindElement,
+                                        weak_ptr_factory_.GetWeakPtr()));
+}
+
+void WaitForDocumentAction::OnFindElement(
+    const ClientStatus& status,
+    std::unique_ptr<ElementFinder::Result> element) {
+  if (!status.ok()) {
+    SendResult(status, DOCUMENT_UNKNOWN_READY_STATE);
+    return;
+  }
+
+  optional_frame_element_ = std::move(element);
+  WaitForReadyState();
+}
+
+void WaitForDocumentAction::WaitForReadyState() {
   delegate_->GetDocumentReadyState(
-      Selector(proto_.wait_for_document().frame()),
+      optional_frame_element_ ? *optional_frame_element_
+                              : ElementFinder::Result(),
       base::BindOnce(&WaitForDocumentAction::OnGetStartState,
                      weak_ptr_factory_.GetWeakPtr()));
 }
@@ -75,8 +97,9 @@ void WaitForDocumentAction::OnGetStartState(const ClientStatus& status,
       base::BindOnce(&WaitForDocumentAction::OnTimeout,
                      weak_ptr_factory_.GetWeakPtr(), base::TimeTicks::Now()));
   delegate_->WaitForDocumentReadyState(
-      Selector(proto_.wait_for_document().frame()),
       proto_.wait_for_document().min_ready_state(),
+      optional_frame_element_ ? *optional_frame_element_
+                              : ElementFinder::Result(),
       base::BindOnce(&WaitForDocumentAction::OnWaitForStartState,
                      weak_ptr_factory_.GetWeakPtr()));
 }
@@ -97,7 +120,8 @@ void WaitForDocumentAction::OnTimeout(base::TimeTicks wait_time_start) {
   action_stopwatch_.TransferToWaitTime(base::TimeTicks::Now() -
                                        wait_time_start);
   delegate_->GetDocumentReadyState(
-      Selector(proto_.wait_for_document().frame()),
+      optional_frame_element_ ? *optional_frame_element_
+                              : ElementFinder::Result(),
       base::BindOnce(&WaitForDocumentAction::OnTimeoutInState,
                      weak_ptr_factory_.GetWeakPtr()));
 }

components/autofill_assistant/browser/actions/wait_for_document_action.h

@@ -9,6 +9,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/timer/timer.h"
 #include "components/autofill_assistant/browser/actions/action.h"
+#include "components/autofill_assistant/browser/web/element_finder.h"
 
 namespace autofill_assistant {
 
@@ -22,7 +23,11 @@ class WaitForDocumentAction : public Action {
   // Overrides Action:
   void InternalProcessAction(ProcessActionCallback callback) override;
 
-  void OnShortWaitForElement(const ClientStatus& status);
+  void OnShortWaitForElement(const Selector& frame_selector,
+                             const ClientStatus& status);
+  void OnFindElement(const ClientStatus& status,
+                     std::unique_ptr<ElementFinder::Result> element);
+  void WaitForReadyState();
 
   void OnGetStartState(const ClientStatus& status,
                        DocumentReadyState start_state);
@@ -36,6 +41,7 @@ class WaitForDocumentAction : public Action {
 
   ProcessActionCallback callback_;
   base::OneShotTimer timer_;
+  std::unique_ptr<ElementFinder::Result> optional_frame_element_;
   base::WeakPtrFactory<WaitForDocumentAction> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(WaitForDocumentAction);

components/autofill_assistant/browser/actions/wait_for_document_action_unittest.cc

@@ -12,6 +12,7 @@
 #include "base/test/gmock_callback_support.h"
 #include "base/test/mock_callback.h"
 #include "base/test/task_environment.h"
+#include "components/autofill_assistant/browser/actions/action_test_utils.h"
 #include "components/autofill_assistant/browser/actions/mock_action_delegate.h"
 #include "components/autofill_assistant/browser/client_status.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -136,7 +137,7 @@ TEST_F(WaitForDocumentActionTest, WaitForDocumentInteractive) {
   EXPECT_CALL(mock_action_delegate_, OnGetDocumentReadyState(_, _))
       .WillOnce(RunOnceCallback<1>(OkClientStatus(), DOCUMENT_LOADING));
   EXPECT_CALL(mock_action_delegate_,
-              OnWaitForDocumentReadyState(_, DOCUMENT_INTERACTIVE, _))
+              OnWaitForDocumentReadyState(DOCUMENT_INTERACTIVE, _, _))
       .WillOnce(RunOnceCallback<2>(OkClientStatus(), DOCUMENT_INTERACTIVE,
                                    base::TimeDelta::FromSeconds(0)));
   proto_.set_timeout_ms(1000);
@@ -158,10 +159,11 @@ TEST_F(WaitForDocumentActionTest, WaitForDocumentInteractiveTimesOut) {
                           base::TimeDelta)>
       captured_callback;
   EXPECT_CALL(mock_action_delegate_,
-              OnWaitForDocumentReadyState(_, DOCUMENT_COMPLETE, _))
+              OnWaitForDocumentReadyState(DOCUMENT_COMPLETE, _, _))
       .WillOnce(Invoke(
           [&captured_callback](
-              const Selector& frame, DocumentReadyState min_ready_state,
+              DocumentReadyState min_ready_state,
+              const ElementFinder::Result& optional_frame_element,
               base::OnceCallback<void(const ClientStatus&, DocumentReadyState,
                                       base::TimeDelta)>& callback) {
             captured_callback = std::move(callback);
@@ -190,13 +192,16 @@ TEST_F(WaitForDocumentActionTest, WaitForDocumentInteractiveTimesOut) {
 }
 
 TEST_F(WaitForDocumentActionTest, CheckDocumentInFrame) {
+  Selector expected_frame_selector({"#frame"});
   EXPECT_CALL(mock_action_delegate_,
-              OnShortWaitForElement(Selector({"#frame"}), _))
+              OnShortWaitForElement(expected_frame_selector, _))
       .WillRepeatedly(RunOnceCallback<1>(OkClientStatus(),
                                          base::TimeDelta::FromSeconds(0)));
-
   EXPECT_CALL(mock_action_delegate_,
-              OnGetDocumentReadyState(Selector({"#frame"}), _))
+              OnGetDocumentReadyState(
+                  EqualsElement(test_util::MockFindElement(
+                      mock_action_delegate_, expected_frame_selector)),
+                  _))
       .WillOnce(RunOnceCallback<1>(OkClientStatus(), DOCUMENT_COMPLETE));
 
   proto_.set_timeout_ms(0);

components/autofill_assistant/browser/script_executor.cc

@@ -692,21 +692,21 @@ bool ScriptExecutor::WaitForNavigation(
 }
 
 void ScriptExecutor::GetDocumentReadyState(
-    const Selector& optional_frame,
+    const ElementFinder::Result& optional_frame_element,
     base::OnceCallback<void(const ClientStatus&, DocumentReadyState)>
         callback) {
-  delegate_->GetWebController()->GetDocumentReadyState(optional_frame,
+  delegate_->GetWebController()->GetDocumentReadyState(optional_frame_element,
                                                        std::move(callback));
 }
 
 void ScriptExecutor::WaitForDocumentReadyState(
-    const Selector& optional_frame,
     DocumentReadyState min_ready_state,
+    const ElementFinder::Result& optional_frame_element,
     base::OnceCallback<void(const ClientStatus&,
                             DocumentReadyState,
                             base::TimeDelta)> callback) {
   delegate_->GetWebController()->WaitForDocumentReadyState(
-      optional_frame, min_ready_state, std::move(callback));
+      optional_frame_element, min_ready_state, std::move(callback));
 }
 
 void ScriptExecutor::LoadURL(const GURL& url) {

components/autofill_assistant/browser/script_executor.h

@@ -230,12 +230,12 @@ class ScriptExecutor : public ActionDelegate,
   bool ExpectedNavigationHasStarted() override;
   bool WaitForNavigation(base::OnceCallback<void(bool)> callback) override;
   void GetDocumentReadyState(
-      const Selector& optional_frame,
+      const ElementFinder::Result& optional_frame_element,
       base::OnceCallback<void(const ClientStatus&, DocumentReadyState)>
           callback) override;
   void WaitForDocumentReadyState(
-      const Selector& optional_frame,
       DocumentReadyState min_ready_state,
+      const ElementFinder::Result& optional_frame_element,
       base::OnceCallback<void(const ClientStatus&,
                               DocumentReadyState,
                               base::TimeDelta)> callback) override;

components/autofill_assistant/browser/web/mock_web_controller.h

@@ -130,30 +130,31 @@ class MockWebController : public WebController {
 
   MOCK_METHOD2(
       OnGetDocumentReadyState,
-      void(const Selector&,
+      void(const ElementFinder::Result&,
            base::OnceCallback<void(const ClientStatus&, DocumentReadyState)>&));
 
   void GetDocumentReadyState(
-      const Selector& frame,
+      const ElementFinder::Result& optional_frame_element,
       base::OnceCallback<void(const ClientStatus&, DocumentReadyState)>
           callback) override {
-    OnGetDocumentReadyState(frame, callback);
+    OnGetDocumentReadyState(optional_frame_element, callback);
   }
 
   MOCK_METHOD3(OnWaitForDocumentReadyState,
-               void(const Selector&,
-                    DocumentReadyState min_ready_state,
+               void(const ElementFinder::Result&,
+                    DocumentReadyState,
                     base::OnceCallback<void(const ClientStatus&,
                                             DocumentReadyState,
                                             base::TimeDelta)>&));
 
   void WaitForDocumentReadyState(
-      const Selector& frame,
+      const ElementFinder::Result& optional_frame_element,
       DocumentReadyState min_ready_state,
       base::OnceCallback<void(const ClientStatus&,
                               DocumentReadyState,
                               base::TimeDelta)> callback) override {
-    OnWaitForDocumentReadyState(frame, min_ready_state, callback);
+    OnWaitForDocumentReadyState(optional_frame_element, min_ready_state,
+                                callback);
   }
 
   base::WeakPtr<WebController> GetWeakPtr() const override {

components/autofill_assistant/browser/web/web_controller.cc

@@ -716,45 +716,22 @@ void WebController::OnWaitForWindowHeightChange(
 }
 
 void WebController::GetDocumentReadyState(
-    const Selector& optional_frame,
+    const ElementFinder::Result& optional_frame_element,
     base::OnceCallback<void(const ClientStatus&, DocumentReadyState)>
         callback) {
   WaitForDocumentReadyState(
-      optional_frame, DOCUMENT_UNKNOWN_READY_STATE,
+      optional_frame_element, DOCUMENT_UNKNOWN_READY_STATE,
       base::BindOnce(&WrapCallbackNoWait, std::move(callback)));
 }
 
 void WebController::WaitForDocumentReadyState(
-    const Selector& optional_frame,
+    const ElementFinder::Result& optional_frame_element,
     DocumentReadyState min_ready_state,
     base::OnceCallback<void(const ClientStatus&,
                             DocumentReadyState,
                             base::TimeDelta)> callback) {
-  if (optional_frame.empty()) {
-    OnFindElementForWaitForDocumentReadyState(
-        min_ready_state, std::move(callback), OkClientStatus(), nullptr);
-    return;
-  }
-  FindElement(
-      optional_frame, /* strict= */ false,
-      base::BindOnce(&WebController::OnFindElementForWaitForDocumentReadyState,
-                     weak_ptr_factory_.GetWeakPtr(), min_ready_state,
-                     std::move(callback)));
-}
-
-void WebController::OnFindElementForWaitForDocumentReadyState(
-    DocumentReadyState min_ready_state,
-    base::OnceCallback<void(const ClientStatus&,
-                            DocumentReadyState,
-                            base::TimeDelta)> callback,
-    const ClientStatus& status,
-    std::unique_ptr<ElementFinder::Result> element) {
-  if (!status.ok()) {
-    std::move(callback).Run(status, DOCUMENT_UNKNOWN_READY_STATE,
-                            base::TimeDelta::FromSeconds(0));
-    return;
-  }
-
+  // Note: An optional frame element will have an empty node_frame_id which
+  // will be considered as operating in the main frame.
   std::string expression;
   AppendWaitForDocumentReadyStateFunction(min_ready_state, &expression);
   devtools_client_->GetRuntime()->Evaluate(
@@ -763,7 +740,7 @@ void WebController::OnFindElementForWaitForDocumentReadyState(
           .SetReturnByValue(true)
           .SetAwaitPromise(true)
           .Build(),
-      /* node_frame_id= */ element ? element->node_frame_id : std::string(),
+      optional_frame_element.node_frame_id,
       base::BindOnce(&WebController::OnWaitForDocumentReadyState,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback),
                      base::TimeTicks::Now()));

components/autofill_assistant/browser/web/web_controller.h

@@ -240,17 +240,17 @@ class WebController {
   virtual void WaitForWindowHeightChange(
       base::OnceCallback<void(const ClientStatus&)> callback);
 
-  // Gets the value of document.readyState for |optional_frame| or, if it is
-  // empty, in the main document.
+  // Gets the value of document.readyState for |optional_frame_element| or, if
+  // it is empty, in the main document.
   virtual void GetDocumentReadyState(
-      const Selector& optional_frame,
+      const ElementFinder::Result& optional_frame_element,
       base::OnceCallback<void(const ClientStatus&, DocumentReadyState)>
           callback);
 
   // Waits for the value of Document.readyState to satisfy |min_ready_state| in
-  // |optional_frame| or, if it is empty, in the main document.
+  // |optional_frame_element| or, if it is empty, in the main document.
   virtual void WaitForDocumentReadyState(
-      const Selector& optional_frame,
+      const ElementFinder::Result& optional_frame_element,
       DocumentReadyState min_ready_state,
       base::OnceCallback<void(const ClientStatus&,
                               DocumentReadyState,
@@ -424,13 +424,6 @@ class WebController {
       base::OnceCallback<void(bool)> callback,
       const DevtoolsClient::ReplyStatus& reply_status,
       std::unique_ptr<runtime::CallFunctionOnResult> result);
-  void OnFindElementForWaitForDocumentReadyState(
-      DocumentReadyState min_ready_state,
-      base::OnceCallback<void(const ClientStatus&,
-                              DocumentReadyState,
-                              base::TimeDelta)> callback,
-      const ClientStatus& status,
-      std::unique_ptr<ElementFinder::Result> element);
   void OnWaitForDocumentReadyState(
       base::OnceCallback<void(const ClientStatus&,
                               DocumentReadyState,

components/autofill_assistant/browser/web/web_controller_browsertest.cc

@@ -2027,7 +2027,7 @@ IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest,
   DocumentReadyState end_state;
   base::RunLoop run_loop;
   web_controller_->WaitForDocumentReadyState(
-      Selector(), DOCUMENT_INTERACTIVE,
+      ElementFinder::Result(), DOCUMENT_INTERACTIVE,
       base::BindOnce(&WebControllerBrowserTest::OnClientStatusAndReadyState,
                      base::Unretained(this), run_loop.QuitClosure(), &status,
                      &end_state));
@@ -2043,7 +2043,7 @@ IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest,
   DocumentReadyState end_state;
   base::RunLoop run_loop;
   web_controller_->WaitForDocumentReadyState(
-      Selector(), DOCUMENT_COMPLETE,
+      ElementFinder::Result(), DOCUMENT_COMPLETE,
       base::BindOnce(&WebControllerBrowserTest::OnClientStatusAndReadyState,
                      base::Unretained(this), run_loop.QuitClosure(), &status,
                      &end_state));
@@ -2056,10 +2056,15 @@ IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest,
 IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest,
                        WaitFrameDocumentReadyStateComplete) {
   ClientStatus status;
+
+  ElementFinder::Result iframe_element;
+  FindElement(Selector({"#iframe"}), &status, &iframe_element);
+  ASSERT_EQ(ACTION_APPLIED, status.proto_status());
+
   DocumentReadyState end_state;
   base::RunLoop run_loop;
   web_controller_->WaitForDocumentReadyState(
-      Selector({"#iframe"}), DOCUMENT_COMPLETE,
+      iframe_element, DOCUMENT_COMPLETE,
       base::BindOnce(&WebControllerBrowserTest::OnClientStatusAndReadyState,
                      base::Unretained(this), run_loop.QuitClosure(), &status,
                      &end_state));
@@ -2072,10 +2077,15 @@ IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest,
 IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest,
                        WaitExternalFrameDocumentReadyStateComplete) {
   ClientStatus status;
+
+  ElementFinder::Result iframe_element;
+  FindElement(Selector({"#iframeExternal"}), &status, &iframe_element);
+  ASSERT_EQ(ACTION_APPLIED, status.proto_status());
+
   DocumentReadyState end_state;
   base::RunLoop run_loop;
   web_controller_->WaitForDocumentReadyState(
-      Selector({"#iframeExternal"}), DOCUMENT_COMPLETE,
+      iframe_element, DOCUMENT_COMPLETE,
       base::BindOnce(&WebControllerBrowserTest::OnClientStatusAndReadyState,
                      base::Unretained(this), run_loop.QuitClosure(), &status,
                      &end_state));