[Autofill Assistant] Timeout visual state callback

previously, if there is no visual update or visual update has passed (page is stable),
then the callback will not be called until the next visual update.
In this CL, it will timeout in ~600ms.

b/119486384

Bug: 806868
Change-Id: I4df02a283dd23c30060aae9a95163b5c02e8cb52
Reviewed-on: https://chromium-review.googlesource.com/c/1340559Reviewed-by: Mathias Carlen <mcarlen@chromium.org>
Commit-Queue: Ganggui Tang <gogerald@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608976}

components/autofill_assistant/browser/web_controller.cc

@@ -142,6 +142,113 @@ const char* const kQuerySelectorAll =
     })";
 }  // namespace
 
+WebController::ElementPositionGetter::ElementPositionGetter()
+    : visual_state_updated_(false), weak_ptr_factory_(this) {}
+WebController::ElementPositionGetter::~ElementPositionGetter() = default;
+
+void WebController::ElementPositionGetter::Start(
+    content::RenderFrameHost* frame_host,
+    DevtoolsClient* devtools_client,
+    std::string element_object_id,
+    base::OnceCallback<void(int, int)> callback) {
+  callback_ = std::move(callback);
+
+  // Wait for a roundtrips through the renderer and compositor pipeline,
+  // otherwise touch event may be dropped because of missing handler.
+  // Note that mouse left button will always be send to the renderer, but it
+  // is slightly better to wait for the changes, like scroll, to be visualized
+  // in compositor as real interaction.
+  frame_host->InsertVisualStateCallback(base::BindOnce(
+      &WebController::ElementPositionGetter::OnVisualStateUpdatedCallback,
+      weak_ptr_factory_.GetWeakPtr()));
+
+  // Set 'point_x' and 'point_y' to -1 to force one round of stable check.
+  GetAndWaitBoxModelStable(devtools_client, element_object_id,
+                           /* point_x= */ -1,
+                           /* point_y= */ -1, kPeriodicBoxModelCheckRounds);
+}
+
+void WebController::ElementPositionGetter::OnVisualStateUpdatedCallback(
+    bool state) {
+  if (state) {
+    visual_state_updated_ = true;
+    return;
+  }
+
+  OnResult(-1, -1);
+}
+
+void WebController::ElementPositionGetter::GetAndWaitBoxModelStable(
+    DevtoolsClient* devtools_client,
+    std::string object_id,
+    int point_x,
+    int point_y,
+    int remaining_rounds) {
+  devtools_client->GetDOM()->GetBoxModel(
+      dom::GetBoxModelParams::Builder().SetObjectId(object_id).Build(),
+      base::BindOnce(
+          &WebController::ElementPositionGetter::OnGetBoxModelForStableCheck,
+          weak_ptr_factory_.GetWeakPtr(), devtools_client, object_id, point_x,
+          point_y, remaining_rounds));
+}
+
+void WebController::ElementPositionGetter::OnGetBoxModelForStableCheck(
+    DevtoolsClient* devtools_client,
+    std::string object_id,
+    int point_x,
+    int point_y,
+    int remaining_rounds,
+    std::unique_ptr<dom::GetBoxModelResult> result) {
+  if (!result || !result->GetModel() || !result->GetModel()->GetContent()) {
+    DLOG(ERROR) << "Failed to get box model.";
+    OnResult(-1, -1);
+    return;
+  }
+
+  // Return the center of the element.
+  const std::vector<double>* content_box = result->GetModel()->GetContent();
+  DCHECK_EQ(content_box->size(), 8u);
+  int x = round((round((*content_box)[0]) + round((*content_box)[2])) * 0.5);
+  int y = round((round((*content_box)[3]) + round((*content_box)[5])) * 0.5);
+
+  // Wait for at least three rounds (~600ms =
+  // 3*kPeriodicBoxModelCheckInterval) for visual state update callback since
+  // it might take longer time to return or never return if no updates.
+  DCHECK(kPeriodicBoxModelCheckRounds > 2 &&
+         kPeriodicBoxModelCheckRounds >= remaining_rounds);
+  if (x == point_x && y == point_y &&
+      (visual_state_updated_ ||
+       remaining_rounds + 2 < kPeriodicBoxModelCheckRounds)) {
+    // Note that there is still a chance that the element's position has been
+    // changed after the last call of GetBoxModel, however, it might be safe
+    // to assume the element's position will not be changed before issuing
+    // click or tap event after stable for kPeriodicBoxModelCheckInterval. In
+    // addition, checking again after issuing click or tap event doesn't help
+    // since the change may be expected.
+    OnResult(x, y);
+    return;
+  }
+
+  if (remaining_rounds <= 0) {
+    OnResult(-1, -1);
+    return;
+  }
+
+  base::PostDelayedTaskWithTraits(
+      FROM_HERE, {content::BrowserThread::UI},
+      base::BindOnce(
+          &WebController::ElementPositionGetter::GetAndWaitBoxModelStable,
+          weak_ptr_factory_.GetWeakPtr(), devtools_client, object_id, x, y,
+          --remaining_rounds),
+      kPeriodicBoxModelCheckInterval);
+}
+
+void WebController::ElementPositionGetter::OnResult(int x, int y) {
+  if (callback_) {
+    std::move(callback_).Run(x, y);
+  }
+}
+
 // static
 std::unique_ptr<WebController> WebController::CreateForWebContents(
     content::WebContents* web_contents) {
@@ -246,98 +353,28 @@ void WebController::OnScrollIntoView(
     return;
   }
 
-  // Wait for a roundtrips through the renderer and compositor pipeline,
-  // otherwise touch event may be dropped because of missing handler.
-  // Note that mouse left button will always be send to the renderer, but it is
-  // slightly better to wait for the changes, like scroll, to be visualized in
-  // compositor as real interaction.
-  target_element->container_frame_host->InsertVisualStateCallback(
-      base::BindOnce(&WebController::OnVisualStateCallback,
-                     weak_ptr_factory_.GetWeakPtr(), std::move(callback),
-                     target_element->object_id, is_a_click));
-}
-
-void WebController::OnVisualStateCallback(
-    base::OnceCallback<void(bool)> callback,
-    std::string object_id,
-    bool is_a_click,
-    bool state) {
-  if (!state) {
-    DLOG(ERROR) << "Wait for visual state failed unexpectedly.";
-    OnResult(false, std::move(callback));
-    return;
-  }
-
-  // Set 'point_x' and 'point_y' to -1 to force one round of stable check.
-  GetAndWaitBoxModelStable(std::move(callback), object_id, is_a_click,
-                           /* point_x= */ -1, /* point_y= */ -1,
-                           kPeriodicBoxModelCheckRounds);
-}
-
-void WebController::GetAndWaitBoxModelStable(
-    base::OnceCallback<void(bool)> callback,
-    std::string object_id,
-    bool is_a_click,
-    int point_x,
-    int point_y,
-    int remaining_rounds) {
-  devtools_client_->GetDOM()->GetBoxModel(
-      dom::GetBoxModelParams::Builder().SetObjectId(object_id).Build(),
-      base::BindOnce(&WebController::OnGetBoxModelForStableCheck,
-                     weak_ptr_factory_.GetWeakPtr(), std::move(callback),
-                     object_id, is_a_click, point_x, point_y,
-                     remaining_rounds));
+  ElementPositionGetter* element_position_checker = new ElementPositionGetter();
+  element_position_checker->Start(
+      target_element->container_frame_host, devtools_client_.get(),
+      target_element->object_id,
+      base::BindOnce(&WebController::TapOrClickOnCoordinates,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     base::WrapUnique(element_position_checker),
+                     std::move(callback), is_a_click));
 }
 
-void WebController::OnGetBoxModelForStableCheck(
+void WebController::TapOrClickOnCoordinates(
+    std::unique_ptr<ElementPositionGetter> element_position_getter,
     base::OnceCallback<void(bool)> callback,
-    std::string object_id,
     bool is_a_click,
-    int point_x,
-    int point_y,
-    int remaining_rounds,
-    std::unique_ptr<dom::GetBoxModelResult> result) {
-  if (!result || !result->GetModel() || !result->GetModel()->GetContent()) {
-    DLOG(ERROR) << "Failed to get box model.";
-    OnResult(false, std::move(callback));
-    return;
-  }
-
-  // Click or tap at the center of the element.
-  const std::vector<double>* content_box = result->GetModel()->GetContent();
-  DCHECK_EQ(content_box->size(), 8u);
-  int x = round((round((*content_box)[0]) + round((*content_box)[2])) * 0.5);
-  int y = round((round((*content_box)[3]) + round((*content_box)[5])) * 0.5);
-
-  if (x == point_x && y == point_y) {
-    // Note that there is still a chance that the element's position has been
-    // changed after the last call of GetBoxModel, however, it might be safe to
-    // assume the element's position will not be changed before issuing click or
-    // tap event after stable for kPeriodicBoxModelCheckInterval. In addition,
-    // checking again after issuing click or tap event doesn't help since the
-    // change may be expected.
-    TapOrClickOnCoordinates(std::move(callback), is_a_click, x, y);
-    return;
-  }
-
-  if (remaining_rounds <= 0) {
+    int x,
+    int y) {
+  if (x < 0 || y < 0) {
+    DLOG(ERROR) << "Failed to get element position.";
     OnResult(false, std::move(callback));
     return;
   }
 
-  base::PostDelayedTaskWithTraits(
-      FROM_HERE, {content::BrowserThread::UI},
-      base::BindOnce(&WebController::GetAndWaitBoxModelStable,
-                     weak_ptr_factory_.GetWeakPtr(), std::move(callback),
-                     object_id, is_a_click, x, y, --remaining_rounds),
-      kPeriodicBoxModelCheckInterval);
-}
-
-void WebController::TapOrClickOnCoordinates(
-    base::OnceCallback<void(bool)> callback,
-    bool is_a_click,
-    int x,
-    int y) {
   if (is_a_click) {
     devtools_client_->GetInput()->DispatchMouseEvent(
         input::DispatchMouseEventParams::Builder()

components/autofill_assistant/browser/web_controller.h

@@ -165,6 +165,43 @@ class WebController {
  private:
   friend class WebControllerBrowserTest;
 
+  // Helper class to get element's position when is stable and the frame it
+  // belongs finished visual update.
+  class ElementPositionGetter {
+   public:
+    ElementPositionGetter();
+    ~ElementPositionGetter();
+
+    // |devtools_client| must outlive this class which is guarantteed by the
+    // owner of this class.
+    void Start(content::RenderFrameHost* frame_host,
+               DevtoolsClient* devtools_client,
+               std::string element_object_id,
+               base::OnceCallback<void(int, int)> callback);
+
+   private:
+    void OnVisualStateUpdatedCallback(bool state);
+    void GetAndWaitBoxModelStable(DevtoolsClient* devtools_client,
+                                  std::string object_id,
+                                  int point_x,
+                                  int point_y,
+                                  int remaining_rounds);
+    void OnGetBoxModelForStableCheck(
+        DevtoolsClient* devtools_client,
+        std::string object_id,
+        int point_x,
+        int point_y,
+        int remaining_rounds,
+        std::unique_ptr<dom::GetBoxModelResult> result);
+    void OnResult(int x, int y);
+
+    base::OnceCallback<void(int, int)> callback_;
+    bool visual_state_updated_;
+
+    base::WeakPtrFactory<ElementPositionGetter> weak_ptr_factory_;
+    DISALLOW_COPY_AND_ASSIGN(ElementPositionGetter);
+  };
+
   // Perform a mouse left button click on the element given by |selectors| and
   // return the result through callback.
   // CSS selectors in |selectors| are ordered from top frame to the frame
@@ -221,28 +258,12 @@ class WebController {
                         base::OnceCallback<void(bool)> callback,
                         bool is_a_click,
                         std::unique_ptr<runtime::CallFunctionOnResult> result);
-  void OnVisualStateCallback(base::OnceCallback<void(bool)> callback,
-                             std::string object_id,
-                             bool is_a_click,
-                             bool state);
-  void GetAndWaitBoxModelStable(base::OnceCallback<void(bool)> callback,
-                                std::string object_id,
-                                bool is_a_click,
-                                int point_x,
-                                int point_y,
-                                int remaining_rounds);
-  void OnGetBoxModelForStableCheck(
+  void TapOrClickOnCoordinates(
+      std::unique_ptr<ElementPositionGetter> element_position_getter,
       base::OnceCallback<void(bool)> callback,
-      std::string object_id,
       bool is_a_click,
-      int point_x,
-      int point_y,
-      int remaining_rounds,
-      std::unique_ptr<dom::GetBoxModelResult> result);
-  void TapOrClickOnCoordinates(base::OnceCallback<void(bool)> callback,
-                               bool is_a_click,
-                               int x,
-                               int y);
+      int x,
+      int y);
   void OnDispatchPressMouseEvent(
       base::OnceCallback<void(bool)> callback,
       int x,

components/autofill_assistant/browser/web_controller_browsertest.cc

@@ -16,7 +16,8 @@ namespace autofill_assistant {
 
 const char* kTargetWebsitePath = "/autofill_assistant_target_website.html";
 
-class WebControllerBrowserTest : public content::ContentBrowserTest {
+class WebControllerBrowserTest : public content::ContentBrowserTest,
+                                 public content::WebContentsObserver {
  public:
   WebControllerBrowserTest() {}
   ~WebControllerBrowserTest() override {}
@@ -32,6 +33,36 @@ class WebControllerBrowserTest : public content::ContentBrowserTest {
         NavigateToURL(shell(), http_server_->GetURL(kTargetWebsitePath)));
     web_controller_ =
         WebController::CreateForWebContents(shell()->web_contents());
+    Observe(shell()->web_contents());
+  }
+
+  void DidCommitAndDrawCompositorFrame() override {
+    paint_occurred_during_last_loop_ = true;
+  }
+
+  void WaitTillPageIsIdle(base::TimeDelta continuous_paint_timeout) {
+    base::TimeTicks finished_load_time = base::TimeTicks::Now();
+    bool page_is_loading = false;
+    do {
+      paint_occurred_during_last_loop_ = false;
+      base::RunLoop heart_beat;
+      base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+          FROM_HERE, heart_beat.QuitClosure(), base::TimeDelta::FromSeconds(3));
+      heart_beat.Run();
+      page_is_loading =
+          web_contents()->IsWaitingForResponse() || web_contents()->IsLoading();
+      if (page_is_loading) {
+        finished_load_time = base::TimeTicks::Now();
+      } else if ((base::TimeTicks::Now() - finished_load_time) >
+                 continuous_paint_timeout) {
+        // |continuous_paint_timeout| has expired since Chrome loaded the page.
+        // During this period of time, Chrome has been continuously painting
+        // the page. In this case, the page is probably idle, but a bug, a
+        // blinking caret or a persistent animation is making Chrome paint at
+        // regular intervals. Exit.
+        break;
+      }
+    } while (page_is_loading || paint_occurred_during_last_loop_);
   }
 
   void RunElementChecks(ElementCheckType check_type,
@@ -287,6 +318,7 @@ class WebControllerBrowserTest : public content::ContentBrowserTest {
 
  private:
   std::unique_ptr<net::EmbeddedTestServer> http_server_;
+  bool paint_occurred_during_last_loop_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(WebControllerBrowserTest);
 };
@@ -404,7 +436,23 @@ IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest,
 
 IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest, TapElement) {
   std::vector<std::string> selectors;
-  selectors.emplace_back("#touch_area");
+  selectors.emplace_back("#touch_area_two");
+  TapElement(selectors);
+  WaitForElementRemove(selectors);
+
+  selectors.clear();
+  selectors.emplace_back("#touch_area_one");
+  TapElement(selectors);
+  WaitForElementRemove(selectors);
+}
+
+IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest, TapElementAfterPageIsIdle) {
+  // Set a very long timeout to make sure either the page is idle or the test
+  // timeout.
+  WaitTillPageIsIdle(base::TimeDelta::FromHours(1));
+
+  std::vector<std::string> selectors;
+  selectors.emplace_back("#touch_area_one");
   TapElement(selectors);
 
   WaitForElementRemove(selectors);

components/test/data/autofill_assistant/autofill_assistant_target_website.html

@@ -17,13 +17,13 @@ found in the LICENSE file.
         button.parentNode.removeChild(button);
       }
 
-      var removeTouchArea = function() {
-        var touch_area = document.getElementById("touch_area");
+      var removeTouchArea = function(element) {
+        var touch_area = document.getElementById(element);
         touch_area.parentNode.removeChild(touch_area);
       }
 
-      var moveTouchArea = function() {
-        var touch_area = document.getElementById("touch_area");
+      var moveTouchAreaTwo = function() {
+        var touch_area = document.getElementById("touch_area_two");
         setTimeout(function(){touch_area.style.left = "100px";}, 100);
         setTimeout(function(){touch_area.style.left = "200px";}, 200);
         setTimeout(function(){touch_area.style.left = "300px";}, 300);
@@ -40,7 +40,14 @@ found in the LICENSE file.
     </style>
   </head>
 
-  <body onload="moveTouchArea()">
+  <body onload="moveTouchAreaTwo()">
+    <!-- Touch area can be accessed without needing scroll.-->
+    <div>
+      <p id="touch_area_one" ontouchend="removeTouchArea('touch_area_one')">
+        Touchable Area One</p>
+      <br>
+    </div>
+
     <!--
     Intentionally make this section has the full height of the window
     to force scroll when operating on the elements below not in this
@@ -51,13 +58,14 @@ found in the LICENSE file.
     </div>
 
     <div>
-      <button id="button" type="button" onclick=
-          "removeButton()">Test Button</button>
+      <p id="touch_area_two" ontouchend="removeTouchArea('touch_area_two')">
+        Touchable Area Two</p>
       <br>
     </div>
 
     <div>
-      <p id="touch_area" ontouchend="removeTouchArea()">Touchable Area</p>
+      <button id="button" type="button" onclick=
+          "removeButton()">Test Button</button>
       <br>
     </div>