Implement the basic testing infrastructure for FindRequestManager.

content/browser/find_request_manager_browsertest.cc

Index: content/browser/find_request_manager_browsertest.cc
diff --git a/content/browser/find_request_manager_browsertest.cc b/content/browser/find_request_manager_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..3d048d40dca1385636d0547a0513568a70083983
--- /dev/null
+++ b/content/browser/find_request_manager_browsertest.cc
@@ -0,0 +1,216 @@
+// 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"
+#include "content/public/test/content_browser_test_utils.h"
+#include "content/public/test/test_navigation_observer.h"
+#include "content/public/test/test_utils.h"
+#include "content/shell/browser/shell.h"
+#include "net/dns/mock_host_resolver.h"
+#include "third_party/WebKit/public/web/WebFindOptions.h"
+
+namespace content {
+
+class TestWebContentsDelegate : public WebContentsDelegate {
+ public:
+  TestWebContentsDelegate()
+      : last_finished_request_id_(kInvalidId),
+        waiting_request_id_(kInvalidId) {}
+  ~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();
+  }
+
+  // The results of a find request.
+  struct FindResults {
+    int request_id = kInvalidId;
+    int number_of_matches = 0;
+    int active_match_ordinal = 0;
+  };
+
+  // Returns the current find results.
+  FindResults GetFindResults() {
+    return current_results_;
+  }
+
+ private:
+  static const int kInvalidId = -1;
+
+  // 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;

[nasko, 2016/04/18 21:02:39]

Shouldn't we update all the parts of the result together? Otherwise it is
possible that these variables will store results from separate IPCs. Is this
intentional?

[paulmeyer, 2016/04/19 15:15:00]

This is intentional. Multiple find IPCs are used to update results
incrementally. It is documented that -1 means "no update, same as before".

[nasko, 2016/04/20 16:47:49]

Acknowledged.

+
+    if (final_update)
+      last_finished_request_id_ = request_id;
+
+    // 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();
+    }
+  }
+
+  // The lastest known results from the current find request.

[nasko, 2016/04/18 21:02:40]

nit: latest? or last?

[paulmeyer, 2016/04/19 15:15:01]

Hahaha. "lastest", meaning "the most last".

It was supposed to be "latest".

+  FindResults current_results_;
+
+  // 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_;
+
+  scoped_refptr<content::MessageLoopRunner> find_message_loop_runner_;
+};

[nasko, 2016/04/18 21:02:40]

DISALLOW_COPY_AND_ASSIGN?

[paulmeyer, 2016/04/19 15:15:01]

Done.

+
+using FindResults = TestWebContentsDelegate::FindResults;

[nasko, 2016/04/18 21:02:40]

Instead of this, why not define the FindResults struct outside of the class? You
can put it in an anonymous namespace if you are worried about name collisions.

[paulmeyer, 2016/04/19 15:15:01]

Done.

+
+class FindRequestManagerTest : public ContentBrowserTest {
+ public:
+  FindRequestManagerTest()
+      : normal_delegate_(nullptr),
+        last_request_id_(0) {}
+  ~FindRequestManagerTest() override {}
+
+  void SetUpOnMainThread() override {
+    host_resolver()->AddRule("*", "127.0.0.1");
+    ASSERT_TRUE(embedded_test_server()->Start());
+
+    // Swap the WebContents's delegate for our test delegate.
+    normal_delegate_ = contents()->GetDelegate();
+    contents()->SetDelegate(new TestWebContentsDelegate());
+  }
+
+  void TearDownOnMainThread() override {
+    // Swap the WebContents's delegate back to its usual delegate.
+    contents()->SetDelegate(normal_delegate_);
+  }
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    IsolateAllSitesForTesting(command_line);
+  }
+
+ protected:
+  // Navigate to |url| and wait for it to finish loading.
+  void LoadAndWait(const std::string& url) {
+    TestNavigationObserver navigation_observer(contents());
+    NavigateToURL(shell(), embedded_test_server()->GetURL("a.com", url));
+  }
+
+  WebContents* contents() {
+    return shell()->web_contents();
+  }
+
+  TestWebContentsDelegate* delegate() {
+    return static_cast<TestWebContentsDelegate*>(contents()->GetDelegate());
+  }
+
+ private:
+  TestWebContentsDelegate test_delegate_;
+  WebContentsDelegate* normal_delegate_;
+
+  int last_request_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(FindRequestManagerTest);
+};
+
+// Test basic find-in-page functionality (such as searching forward and
+// backward) and check for correct results at each step.
+IN_PROC_BROWSER_TEST_F(FindRequestManagerTest, Basic) {
+  LoadAndWait("/find_in_page.html");
+
+  blink::WebFindOptions options;
+  contents()->Find(1, base::UTF8ToUTF16("result"), options);
+  delegate()->WaitForFinalReply(1);
+
+  FindResults results = delegate()->GetFindResults();
+  EXPECT_EQ(1, results.request_id);
+  EXPECT_EQ(19, results.number_of_matches);

[nasko, 2016/04/18 21:02:39]

Why is the total number of matches only 19? There are strings "result XX" across
both frames, where XX is 0-19, which is total of 20.

[paulmeyer, 2016/04/19 15:15:01]

This is because "Result 19" does not actually match (and is not supposed to).
There is a comment in find_in_page_frame.html about it, but I think it's a
rather perplexing comment (it was already there in the file I took from the
existing Blink find tests, so I just left it). I'll change it to hopefully be
more clear.

[nasko, 2016/04/20 16:47:49]

Acknowledged.

+  EXPECT_EQ(1, results.active_match_ordinal);
+
+  options.findNext = true;
+  for (int i = 2; i <= 10; ++i) {
+    contents()->Find(i, base::UTF8ToUTF16("result"), options);
+    delegate()->WaitForFinalReply(i);
+    results = delegate()->GetFindResults();
+
+    EXPECT_EQ(i, results.request_id);
+    EXPECT_EQ(19, results.number_of_matches);
+    EXPECT_EQ(i, results.active_match_ordinal);
+  }
+
+  options.forward = false;
+  for (int i = 11; i <= 15; ++i) {
+    contents()->Find(i, base::UTF8ToUTF16("result"), options);
+    delegate()->WaitForFinalReply(i);
+    results = delegate()->GetFindResults();
+
+    EXPECT_EQ(i, results.request_id);
+    EXPECT_EQ(19, results.number_of_matches);
+    EXPECT_EQ(20 - 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_F(FindRequestManagerTest, CharacterByCharacter) {
+  LoadAndWait("/find_in_page.html");
+
+  blink::WebFindOptions default_options;
+  contents()->Find(1, base::UTF8ToUTF16("r"), default_options);

[nasko, 2016/04/18 21:02:40]

minor nit: Instead of using hardcoded numbers, why not declare a local variable
and increment it, then use the same to wait for reply?

[paulmeyer, 2016/04/19 15:15:01]

Okay, I've done something like that. I'd prefer to use a member on the parent
class to automate the incrementation.

[nasko, 2016/04/20 16:47:50]

Thanks! This looks much cleaner.

+  contents()->Find(2, base::UTF8ToUTF16("re"), default_options);
+  contents()->Find(3, base::UTF8ToUTF16("res"), default_options);
+  contents()->Find(4, base::UTF8ToUTF16("resu"), default_options);
+  contents()->Find(5, base::UTF8ToUTF16("resul"), default_options);
+  contents()->Find(6, base::UTF8ToUTF16("result"), default_options);
+  delegate()->WaitForFinalReply(6);
+
+  FindResults results = delegate()->GetFindResults();
+  EXPECT_EQ(6, results.request_id);
+  EXPECT_EQ(19, results.number_of_matches);
+  EXPECT_EQ(1, results.active_match_ordinal);
+}
+
+// Test sending a large number of find requests subsequently.
+IN_PROC_BROWSER_TEST_F(FindRequestManagerTest, RapidFire) {
+  LoadAndWait("/find_in_page.html");
+
+  blink::WebFindOptions options;
+  contents()->Find(1, base::UTF8ToUTF16("result"), options);
+
+  options.findNext = true;
+  for (int i = 2; i <= 1000; ++i)
+    contents()->Find(i, base::UTF8ToUTF16("result"), options);
+  delegate()->WaitForFinalReply(1000);
+  FindResults results = delegate()->GetFindResults();
+
+  EXPECT_EQ(1000, results.request_id);
+  EXPECT_EQ(19, results.number_of_matches);
+  EXPECT_EQ(1000 % results.number_of_matches, results.active_match_ordinal);
+}
+
+}  // namespace content