Some changes to the testing infrastructure in find_request_manager_browsertest.cc.

content/browser/find_request_manager_browsertest.cc

 // Copyright 2016 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 "base/command_line.h"
 #include "base/strings/utf_string_conversions.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/content_browser_test.h"
@@ skipping 16 matching lines @@
   int request_id = kInvalidId;
   int number_of_matches = 0;
   int active_match_ordinal = 0;
 };
 
 }  // namespace
 
 class TestWebContentsDelegate : public WebContentsDelegate {
  public:
   TestWebContentsDelegate()
-      : last_finished_request_id_(kInvalidId),
-        waiting_request_id_(kInvalidId) {}
+      : last_request_id_(kInvalidId),
+        last_finished_request_id_(kInvalidId),
+        next_reply_received_(false),
+        waiting_for_(NOTHING) {}
   ~TestWebContentsDelegate() override {}
 
-  // Waits for the final reply to the find request with ID |request_id|.
-  void WaitForFinalReply(int request_id) {
-    if (last_finished_request_id_ >= request_id)
-      return;
-
-    waiting_request_id_ = request_id;
-    find_message_loop_runner_ = new content::MessageLoopRunner;
-    find_message_loop_runner_->Run();
-  }
-
   // Returns the current find results.
   FindResults GetFindResults() {
     return current_results_;
   }
 
-#if defined(OS_ANDROID)
-  // Waits for the next find reply. This is useful for waiting for a single
-  // match to be activated, which results in a single find reply (without a
-  // unique request ID).
-  void WaitForNextReply() {
-    waiting_request_id_ = 0;
-    find_message_loop_runner_ = new content::MessageLoopRunner;
-    find_message_loop_runner_->Run();
+  // Waits for all pending replies to be received.
+  void WaitForFinalReply() {
+    if (last_finished_request_id_ >= last_request_id_)
+      return;
+
+    WaitFor(FINAL_REPLY);
   }
 
+  // Waits for the next find reply. This is useful for waiting for a single
+  // match to be activated, or for a new frame to be searched.
+  void WaitForNextReply() {
+    if (next_reply_received_)
+      return;
+
+    WaitFor(NEXT_REPLY);
+  }
+
+  // Indicates that the next find reply from this point will be the one to wait
+  // for when WaitForNextReply() is called. It may be the case that the reply
+  // comes before the call to WaitForNextReply(), in which case it will return
+  // immediately.
+  void MarkNextReply() {
+    next_reply_received_ = false;
+  }
+
+  // Called when a new find request is issued, so the delegate knows the last
+  // request ID.
+  void UpdateLastRequest(int request_id) {
+    last_request_id_ = request_id;
+  }
+
+#if defined(OS_ANDROID)
   // Waits for all of the find match rects to be received.
   void WaitForMatchRects() {
-    match_rects_message_loop_runner_ = new content::MessageLoopRunner;
-    match_rects_message_loop_runner_->Run();
+    WaitFor(MATCH_RECTS);
   }
 
   const std::vector<gfx::RectF>& find_match_rects() const {
     return find_match_rects_;
   }
 
   const gfx::RectF& active_match_rect() const {
     return active_match_rect_;
   }
 #endif
 
  private:
+  enum WaitingFor {
+    NOTHING,
+    FINAL_REPLY,
+    NEXT_REPLY,
+#if defined(OS_ANDROID)
+    MATCH_RECTS
+#endif
+  };
+
   // WebContentsDelegate override.
   void FindReply(WebContents* web_contents,
                  int request_id,
                  int number_of_matches,
                  const gfx::Rect& selection_rect,
                  int active_match_ordinal,
                  bool final_update) override {
     // Update the current results.
     if (request_id > current_results_.request_id)
       current_results_.request_id = request_id;
     if (number_of_matches != -1)
       current_results_.number_of_matches = number_of_matches;
     if (active_match_ordinal != -1)
       current_results_.active_match_ordinal = active_match_ordinal;
 
-    if (final_update)
+    if (!final_update)
+      return;
+
+    if (request_id > last_finished_request_id_)
       last_finished_request_id_ = request_id;
+    next_reply_received_ = true;
 
-    // If we are waiting for a final reply and this is it, stop waiting.
-    if (find_message_loop_runner_.get() &&
-        last_finished_request_id_ >= waiting_request_id_) {
-      find_message_loop_runner_->Quit();
+    // If we are waiting for this find reply, stop waiting.
+    if (waiting_for_ == NEXT_REPLY ||
+        (waiting_for_ == FINAL_REPLY &&
+         last_finished_request_id_ >= last_request_id_)) {
+      WaitFor(NOTHING);
     }
   }
 
+  // Uses |message_loop_runner_| to wait for various things.
+  void WaitFor(WaitingFor wait_for) {
+    waiting_for_ = wait_for;
+
+    if (message_loop_runner_.get())
+      message_loop_runner_->Quit();
+
+    if (wait_for == NOTHING)
+      return;
+
+    message_loop_runner_ = new content::MessageLoopRunner;

[lfg, 2016/08/10 03:19:16]

This is a bit strange. You just called Quit() on the message_loop, and you
destroy it right after? Or am I missing something and there's something else
holding the refptr to the old message loop?

Also, it seems that if WaitFor() is called from inside the message loop (with
something other than NOTHING) this will run a nested message loop which is
really bad.

[paulmeyer, 2016/08/10 15:43:12]

You're right, it is strange. Based on how the function is used, these problems
don't come up, but it doesn't explicitly guarantee that, and it looks weird.
I've cleaned it up by splitting the stop waiting part into its own function. 

+    message_loop_runner_->Run();
+  }
+
 #if defined(OS_ANDROID)
+  // WebContentsDelegate override.
   void FindMatchRectsReply(WebContents* web_contents,
                            int version,
                            const std::vector<gfx::RectF>& rects,
                            const gfx::RectF& active_rect) override {
     // Update the current rects.
     find_match_rects_ = rects;
     active_match_rect_ = active_rect;
 
     // If we are waiting for match rects, stop waiting.
-    if (match_rects_message_loop_runner_.get())
-      match_rects_message_loop_runner_->Quit();
+    if (waiting_for_ == MATCH_RECTS)
+      WaitFor(NOTHING);
   }
 
   std::vector<gfx::RectF> find_match_rects_;
 
   gfx::RectF active_match_rect_;
 #endif
 
   // The latest known results from the current find request.
   FindResults current_results_;
 
+  // The ID of the last find request issued.
+  int last_request_id_;
+
   // The ID of the last find request to finish (all replies received).
   int last_finished_request_id_;
 
-  // If waiting using |find_message_loop_runner_|, this is the ID of the find
-  // request being waited for.
-  int waiting_request_id_;
+  // Indicates whether the next reply after MarkNextReply() has been received.
+  bool next_reply_received_;
 
-  scoped_refptr<content::MessageLoopRunner> find_message_loop_runner_;
-  scoped_refptr<content::MessageLoopRunner> match_rects_message_loop_runner_;
+  // Indicates what |message_loop_runner_| is waiting for, if anything.
+  WaitingFor waiting_for_;
+
+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(TestWebContentsDelegate);
 };
 
 class FindRequestManagerTest : public ContentBrowserTest,
                                public testing::WithParamInterface<bool> {
  public:
   FindRequestManagerTest()
       : normal_delegate_(nullptr),
         last_request_id_(0) {}
@@ skipping 51 matching lines @@
     FrameTreeNode* child = root->child_at(0);
     GURL url(embedded_test_server()->GetURL("b.com",
                                             child->current_url().path()));
     NavigateFrameToURL(child, url);
     EXPECT_EQ(url, observer.last_navigation_url());
     EXPECT_TRUE(observer.last_navigation_succeeded());
   }
 
   void Find(const std::string& search_text,
             const blink::WebFindOptions& options) {
-    contents()->Find(++last_request_id_,
+    delegate()->UpdateLastRequest(++last_request_id_);
+    contents()->Find(last_request_id_,
                      base::UTF8ToUTF16(search_text),
                      options);
   }
 
-  void WaitForFinalReply() const {
-    delegate()->WaitForFinalReply(last_request_id_);
-  }
-
   WebContents* contents() const {
     return shell()->web_contents();
   }
 
   TestWebContentsDelegate* delegate() const {
     return static_cast<TestWebContentsDelegate*>(contents()->GetDelegate());
   }
 
   int last_request_id() const {
     return last_request_id_;
@@ skipping 57 matching lines @@
 
 // Tests basic find-in-page functionality (such as searching forward and
 // backward) and check for correct results at each step.
 IN_PROC_BROWSER_TEST_P(FindRequestManagerTest, MAYBE(Basic)) {
   LoadAndWait("/find_in_page.html");
   if (GetParam())
     MakeChildFrameCrossProcess();
 
   blink::WebFindOptions options;
   Find("result", options);
-  WaitForFinalReply();
+  delegate()->WaitForFinalReply();
 
   FindResults results = delegate()->GetFindResults();
   EXPECT_EQ(last_request_id(), results.request_id);
   EXPECT_EQ(19, results.number_of_matches);
   EXPECT_EQ(1, results.active_match_ordinal);
 
   options.findNext = true;
   for (int i = 2; i <= 10; ++i) {
     Find("result", options);
-    WaitForFinalReply();
+    delegate()->WaitForFinalReply();
 
     results = delegate()->GetFindResults();
     EXPECT_EQ(last_request_id(), results.request_id);
     EXPECT_EQ(19, results.number_of_matches);
     EXPECT_EQ(i, results.active_match_ordinal);
   }
 
   options.forward = false;
   for (int i = 9; i >= 5; --i) {
     Find("result", options);
-    WaitForFinalReply();
+    delegate()->WaitForFinalReply();
 
     results = delegate()->GetFindResults();
     EXPECT_EQ(last_request_id(), results.request_id);
     EXPECT_EQ(19, results.number_of_matches);
     EXPECT_EQ(i, results.active_match_ordinal);
   }
 }
 
 // Tests searching for a word character-by-character, as would typically be done
 // by a user typing into the find bar.
 IN_PROC_BROWSER_TEST_P(FindRequestManagerTest, MAYBE(CharacterByCharacter)) {
   LoadAndWait("/find_in_page.html");
   if (GetParam())
     MakeChildFrameCrossProcess();
 
   blink::WebFindOptions default_options;
   Find("r", default_options);
   Find("re", default_options);
   Find("res", default_options);
   Find("resu", default_options);
   Find("resul", default_options);
   Find("result", default_options);
-  WaitForFinalReply();
+  delegate()->WaitForFinalReply();
 
   FindResults results = delegate()->GetFindResults();
   EXPECT_EQ(last_request_id(), results.request_id);
   EXPECT_EQ(19, results.number_of_matches);
   EXPECT_EQ(1, results.active_match_ordinal);
 }
 
 // Tests sending a large number of find requests subsequently.
 IN_PROC_BROWSER_TEST_P(FindRequestManagerTest, MAYBE(RapidFire)) {
   LoadAndWait("/find_in_page.html");
   if (GetParam())
     MakeChildFrameCrossProcess();
 
   blink::WebFindOptions options;
   Find("result", options);
 
   options.findNext = true;
   for (int i = 2; i <= 1000; ++i)
     Find("result", options);
-  WaitForFinalReply();
+  delegate()->WaitForFinalReply();
 
   FindResults results = delegate()->GetFindResults();
   EXPECT_EQ(last_request_id(), results.request_id);
   EXPECT_EQ(19, results.number_of_matches);
   EXPECT_EQ(last_request_id() % results.number_of_matches,
             results.active_match_ordinal);
 }
 
 // Tests removing a frame during a find session.
 IN_PROC_BROWSER_TEST_P(FindRequestManagerTest, MAYBE(RemoveFrame)) {
   LoadMultiFramePage(2 /* height */, GetParam() /* cross_process */);
 
   blink::WebFindOptions options;
   Find("result", options);
   options.findNext = true;
   options.forward = false;
   Find("result", options);
   Find("result", options);
   Find("result", options);
   Find("result", options);
   Find("result", options);
-  WaitForFinalReply();
+  delegate()->WaitForFinalReply();
 
   FindResults results = delegate()->GetFindResults();
   EXPECT_EQ(last_request_id(), results.request_id);
   EXPECT_EQ(21, results.number_of_matches);
   EXPECT_EQ(17, results.active_match_ordinal);
 
   // Remove a frame.
   FrameTreeNode* root =
       static_cast<WebContentsImpl*>(shell()->web_contents())->
       GetFrameTree()->root();
   root->RemoveChild(root->child_at(0));
 
   // The number of matches and active match ordinal should update automatically
   // to exclude the matches from the removed frame.
   results = delegate()->GetFindResults();
-  EXPECT_EQ(last_request_id(), results.request_id);
   EXPECT_EQ(12, results.number_of_matches);
   EXPECT_EQ(8, results.active_match_ordinal);
 
   // TODO(paulemeyer): Once adding frames mid-session is handled, test that too.
 }
 
 // Tests Searching in a hidden frame. Matches in the hidden frame should be
 // ignored.
 IN_PROC_BROWSER_TEST_F(FindRequestManagerTest, MAYBE(HiddenFrame)) {
   LoadAndWait("/find_in_hidden_frame.html");
 
   blink::WebFindOptions default_options;
   Find("hello", default_options);
-  WaitForFinalReply();
+  delegate()->WaitForFinalReply();
   FindResults results = delegate()->GetFindResults();
 
   EXPECT_EQ(last_request_id(), results.request_id);
   EXPECT_EQ(1, results.number_of_matches);
   EXPECT_EQ(1, results.active_match_ordinal);
 }
 
 #if defined(OS_ANDROID)
 // Tests requesting find match rects.
 IN_PROC_BROWSER_TEST_F(FindRequestManagerTest, MAYBE(FindMatchRects)) {
   LoadAndWait("/find_in_page.html");
 
   blink::WebFindOptions default_options;
   Find("result", default_options);
-  WaitForFinalReply();
+  delegate()->WaitForFinalReply();
   EXPECT_EQ(19, delegate()->GetFindResults().number_of_matches);
 
   // Request the find match rects.
   contents()->RequestFindMatchRects(-1);
   delegate()->WaitForMatchRects();
   const std::vector<gfx::RectF>& rects = delegate()->find_match_rects();
 
   // The first match should be active.
   EXPECT_EQ(rects[0], delegate()->active_match_rect());
 
@@ skipping 61 matching lines @@
   EXPECT_GT(rects[17].y(), rects[18].y());
 }
 
 // Tests activating the find match nearest to a given point.
 IN_PROC_BROWSER_TEST_F(FindRequestManagerTest,
                        MAYBE(ActivateNearestFindMatch)) {
   LoadAndWait("/find_in_page.html");
 
   blink::WebFindOptions default_options;
   Find("result", default_options);
-  WaitForFinalReply();
+  delegate()->WaitForFinalReply();
   EXPECT_EQ(19, delegate()->GetFindResults().number_of_matches);
 
   // Get the find match rects.
   contents()->RequestFindMatchRects(-1);
   delegate()->WaitForMatchRects();
   const std::vector<gfx::RectF>& rects = delegate()->find_match_rects();
 
   // Activate matches via points inside each of the find match rects, in an
   // arbitrary order. Check that the correct match becomes active after each
   // activation.
   int order[19] =
       {11, 13, 2, 0, 16, 5, 7, 10, 6, 1, 15, 14, 9, 17, 18, 3, 8, 12, 4};
   for (int i = 0; i < 19; ++i) {
+    delegate()->MarkNextReply();
     contents()->ActivateNearestFindResult(
         rects[order[i]].CenterPoint().x(), rects[order[i]].CenterPoint().y());
     delegate()->WaitForNextReply();
     EXPECT_EQ(order[i] + 1, delegate()->GetFindResults().active_match_ordinal);
   }
 }
 #endif  // defined(OS_ANDROID)
 
 }  // namespace content