[Autofill Assistant] Separate check for element existence or visibility.

Before this change, WebController::ElementExists always checked for both
existence and visibility.

With this change, WebController::ElementExists checks for existence, and
WebController::ElementVisible for visibility. Element existence is used
for preconditions, visibility is used for all other cases, such as when
waiting for dom or waiting for an element to act on.

This change also simplifies BatchElementChecker a bit by keeping element
existence, visibility and field checks completely separate.

Bug: 806868
Change-Id: Ibd232c317aff4ba4640e5c47a0a93a2b4637ec76
Reviewed-on: https://chromium-review.googlesource.com/c/1301783
Commit-Queue: Stephane Zermatten <szermatt@chromium.org>
Reviewed-by: Ganggui Tang <gogerald@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603556}

components/autofill_assistant/browser/actions/wait_for_dom_action.cc

@@ -29,8 +29,9 @@ void WaitForDomAction::InternalProcessAction(ActionDelegate* delegate,
                                              ProcessActionCallback callback) {
   DCHECK_GT(proto_.wait_for_dom().selectors_size(), 0);
   batch_element_checker_ = delegate->CreateBatchElementChecker();
-  batch_element_checker_->AddElementExistenceCheck(
-      ExtractSelectors(proto_.wait_for_dom().selectors()), base::DoNothing());
+  batch_element_checker_->AddElementCheck(
+      kVisibilityCheck, ExtractSelectors(proto_.wait_for_dom().selectors()),
+      base::DoNothing());
 
   base::TimeDelta duration = kDefaultCheckDuration;
   int timeout_ms = proto_.wait_for_dom().timeout_ms();

components/autofill_assistant/browser/batch_element_checker.cc

@@ -21,7 +21,7 @@ static constexpr base::TimeDelta kCheckPeriod =
 
 BatchElementChecker::BatchElementChecker(WebController* web_controller)
     : web_controller_(web_controller),
-      pending_preconditions_to_check_count_(0),
+      pending_checks_count_(0),
       all_found_(false),
       stopped_(false),
       weak_ptr_factory_(this) {
@@ -30,22 +30,22 @@ BatchElementChecker::BatchElementChecker(WebController* web_controller)
 
 BatchElementChecker::~BatchElementChecker() {}
 
-void BatchElementChecker::AddElementExistenceCheck(
+void BatchElementChecker::AddElementCheck(
+    ElementCheckType check_type,
     const std::vector<std::string>& selectors,
-    base::OnceCallback<void(bool)> callback) {
+    ElementCheckCallback callback) {
   DCHECK(!try_done_callback_);
 
-  element_callback_map_[selectors].element_exists_callbacks.emplace_back(
+  element_check_callbacks_[std::make_pair(check_type, selectors)].emplace_back(
       std::move(callback));
 }
 
 void BatchElementChecker::AddFieldValueCheck(
     const std::vector<std::string>& selectors,
-    base::OnceCallback<void(bool, const std::string&)> callback) {
+    GetFieldValueCallback callback) {
   DCHECK(!try_done_callback_);
 
-  element_callback_map_[selectors].get_field_value_callbacks.emplace_back(
-      std::move(callback));
+  get_field_value_callbacks_[selectors].emplace_back(std::move(callback));
 }
 
 void BatchElementChecker::Run(const base::TimeDelta& duration,
@@ -62,9 +62,6 @@ void BatchElementChecker::Run(const base::TimeDelta& duration,
       base::Unretained(this), try_count, try_done, std::move(all_done)));
 }
 
-BatchElementChecker::ElementCallbacks::ElementCallbacks() {}
-BatchElementChecker::ElementCallbacks::~ElementCallbacks() {}
-
 void BatchElementChecker::Try(base::OnceCallback<void()> try_done_callback) {
   DCHECK(!try_done_callback_);
 
@@ -74,43 +71,53 @@ void BatchElementChecker::Try(base::OnceCallback<void()> try_done_callback) {
   }
   try_done_callback_ = std::move(try_done_callback);
 
-  DCHECK_EQ(pending_preconditions_to_check_count_, 0);
-  pending_preconditions_to_check_count_ = element_callback_map_.size();
-  // Done the check if there is nothing to check.
-  if (!pending_preconditions_to_check_count_) {
-    CheckTryDone();
-    return;
+  DCHECK_EQ(pending_checks_count_, 0);
+  pending_checks_count_ =
+      element_check_callbacks_.size() + get_field_value_callbacks_.size() + 1;
+
+  for (auto& entry : element_check_callbacks_) {
+    if (entry.second.empty()) {
+      pending_checks_count_--;
+      continue;
+    }
+
+    const auto& call_arguments = entry.first;
+    web_controller_->ElementCheck(
+        call_arguments.first, call_arguments.second,
+        base::BindOnce(
+            &BatchElementChecker::OnElementChecked,
+            weak_ptr_factory_.GetWeakPtr(),
+            // Guaranteed to exist for the lifetime of this instance, because
+            // the map isn't modified after Run has been called.
+            base::Unretained(&entry.second)));
   }
 
-  size_t total_number_of_loops = pending_preconditions_to_check_count_;
-  for (auto iter = element_callback_map_.begin();
-       iter != element_callback_map_.end(); iter++) {
-    if (!iter->second.get_field_value_callbacks.empty()) {
-      web_controller_->GetFieldValue(
-          iter->first, base::BindOnce(&BatchElementChecker::OnGetFieldValue,
-                                      weak_ptr_factory_.GetWeakPtr(), iter));
-    } else if (!iter->second.element_exists_callbacks.empty()) {
-      web_controller_->ElementExists(
-          iter->first, base::BindOnce(&BatchElementChecker::OnElementExists,
-                                      weak_ptr_factory_.GetWeakPtr(), iter));
-    } else {
-      // Uninteresting entries
-      pending_preconditions_to_check_count_--;
-      CheckTryDone();
+  for (auto& entry : get_field_value_callbacks_) {
+    if (entry.second.empty()) {
+      pending_checks_count_--;
+      continue;
     }
 
-    // This is an ugly workaround for unit tests. 'all_done' callback will be
-    // called synchronously since mocked web_controller is synchronous. And we
-    // can not simply make it asynchronous since a lot of unit tests rely on
-    // synchronous callback. During the callback consumer may destruct this
-    // class which causes crash since 'element_callback_map_' is deleted.
-    //
-    // TODO(crbug.com/806868): make sure 'all_done' callback is called
-    // asynchronously and fix unit tests accordingly.
-    total_number_of_loops--;
-    if (!total_number_of_loops)
-      return;
+    web_controller_->GetFieldValue(
+        entry.first,
+        base::BindOnce(
+            &BatchElementChecker::OnGetFieldValue,
+            weak_ptr_factory_.GetWeakPtr(),
+            // Guaranteed to exist for the lifetime of this instance, because
+            // the map isn't modified after Run has been called.
+            base::Unretained(&entry.second)));
   }
+
+  // The extra +1 of pending_check_count and this check happening last
+  // guarantees that all_done cannot be called before the end of this function.
+  // Without this, callbacks could be called synchronously by the web
+  // controller, the call all_done, which could delete this instance and all its
+  // datastructures while the function is still going through them.
+  //
+  // TODO(crbug.com/806868): make sure 'all_done' callback is called
+  // asynchronously and fix unit tests accordingly.
+  pending_checks_count_--;
+  CheckTryDone();
 }
 
 void BatchElementChecker::OnTryDone(int64_t remaining_attempts,
@@ -144,68 +151,71 @@ void BatchElementChecker::OnTryDone(int64_t remaining_attempts,
 }
 
 void BatchElementChecker::GiveUp() {
-  for (auto& entry : element_callback_map_) {
-    ElementCallbacks* callbacks = &entry.second;
-    RunElementExistsCallbacks(callbacks, false);
-    RunGetFieldValueCallbacks(callbacks, false, "");
+  for (auto& entry : element_check_callbacks_) {
+    RunCallbacks(&entry.second, false);
+  }
+  for (auto& entry : get_field_value_callbacks_) {
+    RunCallbacks(&entry.second, false, "");
   }
 }
 
-void BatchElementChecker::OnElementExists(ElementCallbackMap::iterator iter,
-                                          bool exists) {
-  pending_preconditions_to_check_count_--;
+void BatchElementChecker::OnElementChecked(
+    std::vector<ElementCheckCallback>* callbacks,
+    bool exists) {
+  pending_checks_count_--;
   if (exists)
-    RunElementExistsCallbacks(&iter->second, true);
+    RunCallbacks(callbacks, true);
 
   CheckTryDone();
 }
 
-void BatchElementChecker::OnGetFieldValue(ElementCallbackMap::iterator iter,
-                                          bool exists,
-                                          const std::string& value) {
-  pending_preconditions_to_check_count_--;
-  if (exists) {
-    RunElementExistsCallbacks(&iter->second, exists);
-    RunGetFieldValueCallbacks(&iter->second, exists, value);
-  }
+void BatchElementChecker::OnGetFieldValue(
+    std::vector<GetFieldValueCallback>* callbacks,
+    bool exists,
+    const std::string& value) {
+  pending_checks_count_--;
+  if (exists)
+    RunCallbacks(callbacks, exists, value);
 
   CheckTryDone();
 }
 
 void BatchElementChecker::CheckTryDone() {
-  DCHECK_GE(pending_preconditions_to_check_count_, 0);
-  if (pending_preconditions_to_check_count_ <= 0 && try_done_callback_) {
+  DCHECK_GE(pending_checks_count_, 0);
+  if (pending_checks_count_ <= 0 && try_done_callback_) {
     all_found_ = !HasMoreChecksToRun();
     std::move(try_done_callback_).Run();
   }
 }
 
-bool BatchElementChecker::HasMoreChecksToRun() {
-  for (const auto& entry : element_callback_map_) {
-    if (!entry.second.element_exists_callbacks.empty())
-      return true;
-
-    if (!entry.second.get_field_value_callbacks.empty())
-      return true;
-  }
-  return false;
-}
-
-void BatchElementChecker::RunElementExistsCallbacks(ElementCallbacks* callbacks,
-                                                    bool result) {
-  for (auto& callback : callbacks->element_exists_callbacks) {
+void BatchElementChecker::RunCallbacks(
+    std::vector<ElementCheckCallback>* callbacks,
+    bool result) {
+  for (auto& callback : *callbacks) {
     std::move(callback).Run(result);
   }
-  callbacks->element_exists_callbacks.clear();
+  callbacks->clear();
 }
 
-void BatchElementChecker::RunGetFieldValueCallbacks(ElementCallbacks* callbacks,
-                                                    bool exists,
-                                                    const std::string& value) {
-  for (auto& callback : callbacks->get_field_value_callbacks) {
+void BatchElementChecker::RunCallbacks(
+    std::vector<GetFieldValueCallback>* callbacks,
+    bool exists,
+    const std::string& value) {
+  for (auto& callback : *callbacks) {
     std::move(callback).Run(exists, value);
   }
-  callbacks->get_field_value_callbacks.clear();
+  callbacks->clear();
 }
 
+bool BatchElementChecker::HasMoreChecksToRun() {
+  for (const auto& entry : element_check_callbacks_) {
+    if (!entry.second.empty())
+      return true;
+  }
+  for (const auto& entry : get_field_value_callbacks_) {
+    if (!entry.second.empty())
+      return true;
+  }
+  return false;
+}
 }  // namespace autofill_assistant

components/autofill_assistant/browser/batch_element_checker.h

@@ -19,6 +19,9 @@
 namespace autofill_assistant {
 class WebController;
 
+// Types of element checks.
+enum ElementCheckType { kExistenceCheck, kVisibilityCheck };
+
 // Helper for checking a set of elements at the same time. It avoids duplicate
 // checks and supports retries.
 //
@@ -27,12 +30,12 @@ class WebController;
 // The simplest way of using a BatchElementChecker is to:
 // - create an instance, using WebController::CreateBatchElementChecker or
 //   ActionDelegate::CreateBatchElementChecker
-// - call AddElementExistenceCheck() and AddFieldValueCheck()
+// - call AddElementCheck() and AddFieldValueCheck()
 // - call Run() with duration set to 0.
 //
 // The result of the checks is reported to the callbacks passed to
-// AddElementExistenceCheck() and AddFieldValueCheck(), then the callback passed
-// to Run() is called, to report the end of the a run.
+// AddElementCheck(kExistenceCheck, ) and AddFieldValueCheck(), then the
+// callback passed to Run() is called, to report the end of the a run.
 //
 // Check with retries:
 //
@@ -46,19 +49,34 @@ class BatchElementChecker {
   explicit BatchElementChecker(WebController* web_controller);
   virtual ~BatchElementChecker();
 
-  // Checks whether the element given by |selectors| exists on the web page.
+  // Callback for AddElementCheck. Argument is true if the check passed.
+  using ElementCheckCallback = base::OnceCallback<void(bool)>;
+
+  // Callback for AddFieldValueCheck. Argument is true is the element exists.
+  // The string contains the field value, or an empty string if accessing the
+  // value failed.
+  using GetFieldValueCallback =
+      base::OnceCallback<void(bool, const std::string&)>;
+
+  // Checks an an element.
+  //
+  // kElementCheck checks whether at least one element given by |selectors|
+  // exists on the web page.
+  //
+  // kVisibilityCheck checks whether at least one element given by |selectors|
+  // is visible on the page.
   //
-  // New element existence checks cannot be added once Run has been called.
-  void AddElementExistenceCheck(const std::vector<std::string>& selectors,
-                                base::OnceCallback<void(bool)> callback);
+  // New element checks cannot be added once Run has been called.
+  void AddElementCheck(ElementCheckType check_type,
+                       const std::vector<std::string>& selectors,
+                       ElementCheckCallback callback);
 
   // Gets the value of |selectors| and return the result through |callback|. The
   // returned value will be the empty string in case of error or empty value.
   //
   // New field checks cannot be added once Run has been called.
-  void AddFieldValueCheck(
-      const std::vector<std::string>& selectors,
-      base::OnceCallback<void(bool, const std::string&)> callback);
+  void AddFieldValueCheck(const std::vector<std::string>& selectors,
+                          GetFieldValueCallback callback);
 
   // Runs the checks until all elements exist or for |duration|, whichever one
   // comes first. Elements found are reported as soon as they're founds.
@@ -78,58 +96,52 @@ class BatchElementChecker {
   bool all_found() { return all_found_; }
 
  private:
-  // Checks to run on a specific element, and the callbacks to report them to.
-  struct ElementCallbacks {
-    ElementCallbacks();
-    ~ElementCallbacks();
-
-    // All the callbacks registered for that element by separate calls to
-    // AddElementExistenceCheck with the same selector.
-    std::vector<base::OnceCallback<void(bool)>> element_exists_callbacks;
-
-    // All the callbacks registered for that field by separate calls to
-    // AddFieldValueCheck with the same selector.
-    std::vector<base::OnceCallback<void(bool, const std::string&)>>
-        get_field_value_callbacks;
-  };
-
-  using ElementCallbackMap =
-      std::map<std::vector<std::string>, ElementCallbacks>;
-
   // Tries running the checks, reporting only successes.
   //
   // Calls |try_done_callback| at the end of the run.
   void Try(base::OnceCallback<void()> try_done_callback);
 
+  void OnTryDone(int64_t remaining_attempts,
+                 base::RepeatingCallback<void()> try_done,
+                 base::OnceCallback<void()> all_done);
+
   // If there are still callbacks not called by a previous call to Try, call
   // them now. When this method returns, all callbacks are guaranteed to have
   // been run.
   void GiveUp();
 
-  void OnTryDone(int64_t remaining_attempts,
-                 base::RepeatingCallback<void()> try_done,
-                 base::OnceCallback<void()> all_done);
-  bool HasMoreChecksToRun();
-  void OnElementExists(ElementCallbackMap::iterator iter, bool exists);
-  void OnGetFieldValue(ElementCallbackMap::iterator iter,
+  void OnElementChecked(std::vector<ElementCheckCallback>* callbacks,
+                        bool exists);
+  void OnGetFieldValue(std::vector<GetFieldValueCallback>* callbacks,
                        bool exists,
                        const std::string& value);
   void CheckTryDone();
-  void RunElementExistsCallbacks(ElementCallbacks* element, bool result);
-  void RunGetFieldValueCallbacks(ElementCallbacks* element,
-                                 bool exists,
-                                 const std::string& value);
+  void RunCallbacks(std::vector<ElementCheckCallback>* callbacks, bool result);
+  void RunCallbacks(std::vector<GetFieldValueCallback>* callbacks,
+                    bool result,
+                    const std::string& value);
+  bool HasMoreChecksToRun();
 
   WebController* const web_controller_;
-  ElementCallbackMap element_callback_map_;
-  int pending_preconditions_to_check_count_;
+
+  // A map of ElementCheck arguments (check_type, selectors) to callbacks that
+  // take the result of the check.
+  std::map<std::pair<ElementCheckType, std::vector<std::string>>,
+           std::vector<ElementCheckCallback>>
+      element_check_callbacks_;
+
+  // A map of GetFieldValue arguments (selectors) to callbacks that take the
+  // field value.
+  std::map<std::vector<std::string>, std::vector<GetFieldValueCallback>>
+      get_field_value_callbacks_;
+  int pending_checks_count_;
   bool all_found_;
   bool stopped_;
 
-  // The callback built for Try(). It is kept around while going through
-  // |element_callback_map_| and called once all selectors in that map have been
-  // looked up once, whether that lookup was successful or not. Also used to
-  // guarantee that only one Try runs at a time.
+  // The callback built for Try(). It is kept around while going through the
+  // maps and called once all selectors in that map have been looked up once,
+  // whether that lookup was successful or not. Also used to guarantee that only
+  // one Try runs at a time.
   base::OnceCallback<void()> try_done_callback_;
 
   base::WeakPtrFactory<BatchElementChecker> weak_ptr_factory_;

components/autofill_assistant/browser/mock_web_controller.h

@@ -41,12 +41,14 @@ class MockWebController : public WebController {
                void(const std::vector<std::string>& selectors,
                     base::OnceCallback<void(bool)>& callback));
 
-  void ElementExists(const std::vector<std::string>& selectors,
-                     base::OnceCallback<void(bool)> callback) override {
-    OnElementExists(selectors, callback);
+  void ElementCheck(ElementCheckType check_type,
+                    const std::vector<std::string>& selectors,
+                    base::OnceCallback<void(bool)> callback) override {
+    OnElementCheck(check_type, selectors, callback);
   }
-  MOCK_METHOD2(OnElementExists,
-               void(const std::vector<std::string>& selectors,
+  MOCK_METHOD3(OnElementCheck,
+               void(ElementCheckType check_type,
+                    const std::vector<std::string>& selectors,
                     base::OnceCallback<void(bool)>& callback));
 
   void GetFieldValue(

components/autofill_assistant/browser/script_executor.cc

@@ -64,7 +64,7 @@ ScriptExecutor::CreateBatchElementChecker() {
 void ScriptExecutor::WaitForElement(const std::vector<std::string>& selectors,
                                     base::OnceCallback<void(bool)> callback) {
   std::unique_ptr<BatchElementChecker> checker = CreateBatchElementChecker();
-  checker->AddElementExistenceCheck(selectors, base::DoNothing());
+  checker->AddElementCheck(kVisibilityCheck, selectors, base::DoNothing());
   checker->Run(kWaitForSelectorDeadline,
                /* try_done= */ base::DoNothing(),
                /* all_done= */

components/autofill_assistant/browser/script_executor_unittest.cc

@@ -43,8 +43,8 @@ class ScriptExecutorTest : public testing::Test,
 
     // In this test, "tell" actions always succeed and "click" actions always
     // fail. The following makes a click action fail immediately
-    ON_CALL(mock_web_controller_, OnElementExists(_, _))
-        .WillByDefault(RunOnceCallback<1>(true));
+    ON_CALL(mock_web_controller_, OnElementCheck(_, _, _))
+        .WillByDefault(RunOnceCallback<2>(true));
     ON_CALL(mock_web_controller_, OnClickElement(_, _))
         .WillByDefault(RunOnceCallback<1>(false));
     ON_CALL(mock_web_controller_, OnFocusElement(_, _))

components/autofill_assistant/browser/script_precondition.cc

@@ -98,7 +98,8 @@ void ScriptPrecondition::Check(
     base::OnceCallback<void(bool)> callback =
         base::BindOnce(&ScriptPrecondition::OnCheckElementExists,
                        weak_ptr_factory_.GetWeakPtr());
-    batch_checks->AddElementExistenceCheck(selector, std::move(callback));
+    batch_checks->AddElementCheck(kExistenceCheck, selector,
+                                  std::move(callback));
   }
   for (const auto& value_match : form_value_match_) {
     DCHECK(!value_match.element().selectors().empty());

components/autofill_assistant/browser/script_precondition_unittest.cc

@@ -55,11 +55,12 @@ class DirectCallback {
 class ScriptPreconditionTest : public testing::Test {
  public:
   void SetUp() override {
-    ON_CALL(mock_web_controller_, OnElementExists(ElementsAre("exists"), _))
-        .WillByDefault(RunOnceCallback<1>(true));
     ON_CALL(mock_web_controller_,
-            OnElementExists(ElementsAre("does_not_exist"), _))
-        .WillByDefault(RunOnceCallback<1>(false));
+            OnElementCheck(kExistenceCheck, ElementsAre("exists"), _))
+        .WillByDefault(RunOnceCallback<2>(true));
+    ON_CALL(mock_web_controller_,
+            OnElementCheck(kExistenceCheck, ElementsAre("does_not_exist"), _))
+        .WillByDefault(RunOnceCallback<2>(false));
 
     SetUrl("http://www.example.com/path");
     ON_CALL(mock_web_controller_, OnGetFieldValue(ElementsAre("exists"), _))
@@ -177,8 +178,9 @@ TEST_F(ScriptPreconditionTest, BadPathPattern) {
 }
 
 TEST_F(ScriptPreconditionTest, IgnoreEmptyElementsExist) {
-  EXPECT_CALL(mock_web_controller_, OnElementExists(ElementsAre("exists"), _))
-      .WillOnce(RunOnceCallback<1>(true));
+  EXPECT_CALL(mock_web_controller_,
+              OnElementCheck(kExistenceCheck, ElementsAre("exists"), _))
+      .WillOnce(RunOnceCallback<2>(true));
 
   ScriptPreconditionProto proto;
   proto.add_elements_exist()->add_selectors("exists");

components/autofill_assistant/browser/script_tracker_unittest.cc

@@ -32,11 +32,12 @@ class ScriptTrackerTest : public testing::Test,
                           public ScriptExecutorDelegate {
  public:
   void SetUp() override {
-    ON_CALL(mock_web_controller_, OnElementExists(ElementsAre("exists"), _))
-        .WillByDefault(RunOnceCallback<1>(true));
     ON_CALL(mock_web_controller_,
-            OnElementExists(ElementsAre("does_not_exist"), _))
-        .WillByDefault(RunOnceCallback<1>(false));
+            OnElementCheck(kExistenceCheck, ElementsAre("exists"), _))
+        .WillByDefault(RunOnceCallback<2>(true));
+    ON_CALL(mock_web_controller_,
+            OnElementCheck(kExistenceCheck, ElementsAre("does_not_exist"), _))
+        .WillByDefault(RunOnceCallback<2>(false));
     ON_CALL(mock_web_controller_, GetUrl()).WillByDefault(ReturnRef(url_));
 
     // Scripts run, but have no actions.
@@ -250,8 +251,8 @@ TEST_F(ScriptTrackerTest, CheckScriptsAgainAfterScriptEnd) {
 
 TEST_F(ScriptTrackerTest, CheckScriptsAfterDOMChange) {
   EXPECT_CALL(mock_web_controller_,
-              OnElementExists(ElementsAre("maybe_exists"), _))
-      .WillOnce(RunOnceCallback<1>(false));
+              OnElementCheck(kExistenceCheck, ElementsAre("maybe_exists"), _))
+      .WillOnce(RunOnceCallback<2>(false));
 
   SupportsScriptResponseProto scripts;
   AddScript(&scripts, "script name", "script path", "maybe_exists");
@@ -262,8 +263,8 @@ TEST_F(ScriptTrackerTest, CheckScriptsAfterDOMChange) {
 
   // DOM has changed; OnElementExists now returns true.
   EXPECT_CALL(mock_web_controller_,
-              OnElementExists(ElementsAre("maybe_exists"), _))
-      .WillOnce(RunOnceCallback<1>(true));
+              OnElementCheck(kExistenceCheck, ElementsAre("maybe_exists"), _))
+      .WillOnce(RunOnceCallback<2>(true));
   tracker_.CheckScripts(base::TimeDelta::FromSeconds(0));
 
   // The script can now run
@@ -275,9 +276,10 @@ TEST_F(ScriptTrackerTest, DuplicateCheckCalls) {
   AddScript(&scripts, "runnable name", "runnable path", "exists");
 
   base::OnceCallback<void(bool)> captured_callback;
-  EXPECT_CALL(mock_web_controller_, OnElementExists(ElementsAre("exists"), _))
-      .WillOnce(CaptureOnceCallback<1>(&captured_callback))
-      .WillOnce(RunOnceCallback<1>(false));
+  EXPECT_CALL(mock_web_controller_,
+              OnElementCheck(kExistenceCheck, ElementsAre("exists"), _))
+      .WillOnce(CaptureOnceCallback<2>(&captured_callback))
+      .WillOnce(RunOnceCallback<2>(false));
   SetAndCheckScripts(scripts);
 
   // At this point, since the callback hasn't been run, there's still a check in

components/autofill_assistant/browser/web_controller.cc

@@ -238,32 +238,39 @@ void WebController::OnDispatchReleaseMouseEvent(
   OnResult(true, std::move(callback));
 }
 
-void WebController::ElementExists(const std::vector<std::string>& selectors,
-                                  base::OnceCallback<void(bool)> callback) {
+void WebController::ElementCheck(ElementCheckType check_type,
+                                 const std::vector<std::string>& selectors,
+                                 base::OnceCallback<void(bool)> callback) {
   DCHECK(!selectors.empty());
-  // We don't use strict_mode because we only check for the existence of an
-  // element and we don't act on it.
-  FindElement(
-      selectors, /* strict_mode= */ false,
-      base::BindOnce(&WebController::OnFindElementForExist,
-                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+  // We don't use strict_mode because we only check for the existence of at
+  // least one such element and we don't act on it.
+  FindElement(selectors, /* strict_mode= */ false,
+              base::BindOnce(&WebController::OnFindElementForCheck,
+                             weak_ptr_factory_.GetWeakPtr(), check_type,
+                             std::move(callback)));
 }
 
-void WebController::OnFindElementForExist(
+void WebController::OnFindElementForCheck(
+    ElementCheckType check_type,
     base::OnceCallback<void(bool)> callback,
     std::unique_ptr<FindElementResult> result) {
   if (result->object_id.empty()) {
     OnResult(false, std::move(callback));
     return;
   }
+  if (check_type == kExistenceCheck) {
+    OnResult(true, std::move(callback));
+    return;
+  }
+  DCHECK_EQ(check_type, kVisibilityCheck);
 
   devtools_client_->GetDOM()->GetBoxModel(
       dom::GetBoxModelParams::Builder().SetObjectId(result->object_id).Build(),
-      base::BindOnce(&WebController::OnGetBoxModelForExist,
+      base::BindOnce(&WebController::OnGetBoxModelForVisible,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
 
-void WebController::OnGetBoxModelForExist(
+void WebController::OnGetBoxModelForVisible(
     base::OnceCallback<void(bool)> callback,
     std::unique_ptr<dom::GetBoxModelResult> result) {
   OnResult(result && result->GetModel() && result->GetModel()->GetContent(),

components/autofill_assistant/browser/web_controller.h

@@ -115,13 +115,18 @@ class WebController {
  protected:
   friend class BatchElementChecker;
 
-  // Check whether at least one element given by |selectors| exists on the web
-  // page. The element must have a valid BoxModel (i.e. it must be visible and
-  // have a size greater than 0).
+  // Checks an element for:
+  //
+  // kExistenceCheck: Checks whether at least one element given by |selectors|
+  // exists on the web page.
+  //
+  // kVisibilityCheck: Checks whether at least on element given by |selectors|
+  // is visible on the web page.
   //
   // Normally done through BatchElementChecker.
-  virtual void ElementExists(const std::vector<std::string>& selectors,
-                             base::OnceCallback<void(bool)> callback);
+  virtual void ElementCheck(ElementCheckType type,
+                            const std::vector<std::string>& selectors,
+                            base::OnceCallback<void(bool)> callback);
 
   // Get the value of |selectors| and return the result through |callback|. The
   // returned value might be false, if the element cannot be found, true and the
@@ -183,10 +188,11 @@ class WebController {
   void OnDispatchReleaseMouseEvent(
       base::OnceCallback<void(bool)> callback,
       std::unique_ptr<input::DispatchMouseEventResult> result);
-  void OnFindElementForExist(base::OnceCallback<void(bool)> callback,
+  void OnFindElementForCheck(ElementCheckType check_type,
+                             base::OnceCallback<void(bool)> callback,
                              std::unique_ptr<FindElementResult> result);
-  void OnGetBoxModelForExist(base::OnceCallback<void(bool)> callback,
-                             std::unique_ptr<dom::GetBoxModelResult> result);
+  void OnGetBoxModelForVisible(base::OnceCallback<void(bool)> callback,
+                               std::unique_ptr<dom::GetBoxModelResult> result);
 
   // Find the element given by |selectors|. If multiple elements match
   // |selectors| and if |strict_mode| is false, return the first one that is

components/autofill_assistant/browser/web_controller_browsertest.cc

@@ -34,25 +34,26 @@ class WebControllerBrowserTest : public content::ContentBrowserTest {
         WebController::CreateForWebContents(shell()->web_contents());
   }
 
-  void AreElementsExist(const std::vector<std::vector<std::string>>& selectors,
+  void RunElementChecks(ElementCheckType check_type,
+                        const std::vector<std::vector<std::string>>& selectors,
                         const std::vector<bool> results) {
     base::RunLoop run_loop;
     ASSERT_EQ(selectors.size(), results.size());
     size_t pending_number_of_checks = selectors.size();
     for (size_t i = 0; i < selectors.size(); i++) {
-      web_controller_->ElementExists(
-          selectors[i],
-          base::BindOnce(&WebControllerBrowserTest::CheckElementExistCallback,
+      web_controller_->ElementCheck(
+          check_type, selectors[i],
+          base::BindOnce(&WebControllerBrowserTest::CheckElementVisibleCallback,
                          base::Unretained(this), run_loop.QuitClosure(),
                          &pending_number_of_checks, results[i]));
     }
     run_loop.Run();
   }
 
-  void CheckElementExistCallback(const base::Closure& done_callback,
-                                 size_t* pending_number_of_checks_output,
-                                 bool expected_result,
-                                 bool result) {
+  void CheckElementVisibleCallback(const base::Closure& done_callback,
+                                   size_t* pending_number_of_checks_output,
+                                   bool expected_result,
+                                   bool result) {
     ASSERT_EQ(expected_result, result);
     *pending_number_of_checks_output -= 1;
     if (*pending_number_of_checks_output == 0) {
@@ -77,8 +78,8 @@ class WebControllerBrowserTest : public content::ContentBrowserTest {
 
   void WaitForElementRemove(const std::vector<std::string>& selectors) {
     base::RunLoop run_loop;
-    web_controller_->ElementExists(
-        selectors,
+    web_controller_->ElementCheck(
+        kExistenceCheck, selectors,
         base::BindOnce(&WebControllerBrowserTest::OnWaitForElementRemove,
                        base::Unretained(this), run_loop.QuitClosure(),
                        selectors));
@@ -255,7 +256,7 @@ class WebControllerBrowserTest : public content::ContentBrowserTest {
   DISALLOW_COPY_AND_ASSIGN(WebControllerBrowserTest);
 };
 
-IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest, ConcurrentElementsExist) {
+IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest, ConcurrentElementsVisible) {
   std::vector<std::vector<std::string>> selectors;
   std::vector<bool> results;
 
@@ -315,7 +316,33 @@ IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest, ConcurrentElementsExist) {
   selectors.emplace_back(a_selector);
   results.emplace_back(false);
 
-  AreElementsExist(selectors, results);
+  RunElementChecks(kVisibilityCheck, selectors, results);
+}
+
+IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest, ElementExists) {
+  std::vector<std::vector<std::string>> selectors;
+  std::vector<bool> results;
+
+  std::vector<std::string> a_selector;
+
+  // A visible element
+  a_selector.emplace_back("#button");
+  selectors.emplace_back(a_selector);
+  results.emplace_back(true);
+
+  // A hidden element.
+  a_selector.clear();
+  a_selector.emplace_back("#hidden");
+  selectors.emplace_back(a_selector);
+  results.emplace_back(true);
+
+  // A nonexistent element.
+  a_selector.clear();
+  a_selector.emplace_back("#doesnotexist");
+  selectors.emplace_back(a_selector);
+  results.emplace_back(false);
+
+  RunElementChecks(kExistenceCheck, selectors, results);
 }
 
 IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest, ClickElement) {