Fix dictionary look-up for highlighted text in Mac.

chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc

Index: chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
diff --git a/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc b/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
index 384a4c0b4716f5cb291a3b841530850fed76eb09..70b818c5f4b673f3ce47304fec7cf7814fbb839c 100644
--- a/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
+++ b/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
@@ -6,15 +6,19 @@
 
 #include "base/command_line.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/chrome_content_browser_client.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/interactive_test_utils.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/browser_message_filter.h"
+#include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/content_client.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/test_utils.h"
@@ -330,7 +334,8 @@ class SitePerProcessTextInputManagerTest : public InProcessBrowserTest {
   // static
   // Adds an <input> field to a given frame by executing javascript code.
   // The input can be added as the first element or the last element of
-  // |document.body|.
+  // |document.body|. The text range defined by |selection_range| will be
+  // marked.
   static void AddInputFieldToFrame(content::RenderFrameHost* rfh,
                                    const std::string& type,
                                    const std::string& value,
@@ -346,6 +351,25 @@ class SitePerProcessTextInputManagerTest : public InProcessBrowserTest {
     EXPECT_TRUE(ExecuteScript(rfh, script));
   }
 
+  // Returns a point representing the top-left corner of the first <input>
+  // inside |rfh|.

[EhsanK, 2016/10/11 19:31:56]

Apologies. This method should have not been in the previous patch since it is
unused.

+  static gfx::Point GetTopLeftOfFirstInput(content::RenderFrameHost* rfh) {
+    int x = 0;
+    int y = 0;
+    // Get the client X value.
+    EXPECT_TRUE(content::ExecuteScriptAndExtractInt(
+        rfh,
+        "var input = document.querySelector('input');"
+        "domAutomationController.send(input.getBoundingClientRect().left);",
+        &x));
+    EXPECT_TRUE(content::ExecuteScriptAndExtractInt(
+        rfh,
+        "var input = document.querySelector('input');"
+        "domAutomationController.send(input.getBoundingClientRect().top);",
+        &y));
+    return gfx::Point(x, y);
+  }
+
   // Uses 'cross_site_iframe_factory.html'. The main frame's domain is
   // 'a.com'.
   void CreateIframePage(const std::string& structure) {
@@ -948,3 +972,173 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessTextInputManagerTest,
   EXPECT_EQ("", result);
 }
 #endif
+
+#if defined(OS_MACOSX)

[Charlie Reis, 2016/10/07 19:07:21]

We added a site_per_process_mac_browsertest.mm recently.  Would this fit better
there, or does it depend on enough things in this file that we need it here? 
(If the latter, please mention so in a comment above this line.)

[EhsanK, 2016/10/11 19:31:56]

Yes, ideally, we should stay within that file. Specifically given that I do not
use any interactive features from chrome/ and making things happen with
javascript.

I use ContentBrowserClient to setup filters here. For that, the
TestBrowserClient inherits the default ContentBrowserClient type used in the
tests (to make sure we will initialize the WebContents/RenderProcessHost
properly). Ax explained before this is to give myself a chance to add the test
filter in time. My knowledge is limited here but I think we have to inherit from
the test type or otherwise, implement a lot of methods in ContentBrowserClient
with duplicated code.

Having said that, for shell, the type would be ShellContentBrowserClient. But
there is a static variable in which is set when the first instance of
ShellContentBrowserClient is created. That prohibits further instantiations of
any test classes.

Now to solve this what I think we could do is:
1- Use ChromeContentBrowserClient which does not have the troublesome static.
2- Add a method to ShellContentBrowserClient to reset the static and perhaps set
it again when test is about to finish.
3- Implement an all new ContentBrowserClient for testing which wraps the content
one.
4- Remove the static from ShellContentBrowserClient.

Out of them all, 1) was the easiest in terms of changing pre-existing code. I
think 4) might be a good thing to do unless there is a compelling reason to make
ShellContentBrowserClient singleton even for tests. Specifically, given that
existence of this static variable and DCHECK obviates what
content::SetBrowserClientForTesting has to offer. WDYT?

[Charlie Reis, 2016/10/17 21:35:03]

Ok, sounds like ShellContentBrowserClient was non-trivial.  Let's just put a
comment explaining that the dependency on setting a ContentBrowserClient is why
this is in this file.

+// An observer of number of open windows.
+class WindowCountObserver {
+ public:
+  explicit WindowCountObserver(size_t lower_limit) : limit_(lower_limit) {}
+  ~WindowCountObserver() {}
+
+  // Keep polling the count of open windows until the number exceeds |limit_|.
+  void WaitForLimitOrMore() {
+    size_t current_count = content::GetOpenNSWindowsCount();
+    if (current_count >= limit_)
+      return;
+
+    message_loop_runner_ = new content::MessageLoopRunner();
+    message_loop_runner_->Run();
+    content::BrowserThread::PostTask(
+        content::BrowserThread::UI, FROM_HERE,
+        base::Bind(&WindowCountObserver::CheckWindowCount,
+                   base::Unretained(this)));
+  }
+
+ private:
+  void CheckWindowCount() {
+    size_t current_count = content::GetOpenNSWindowsCount();
+    if (current_count >= limit_) {
+      message_loop_runner_->Quit();
+      return;
+    }
+    content::BrowserThread::PostTask(
+        content::BrowserThread::UI, FROM_HERE,
+        base::Bind(&WindowCountObserver::CheckWindowCount,
+                   base::Unretained(this)));
+  }
+
+  size_t limit_;
+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(WindowCountObserver);
+};
+
+// The original TextInputClientMessageFilter is added during the initialization
+// phase of RenderProcessHost. The only chance we have to add the test filter
+// (so that it can receive the TextInputClientMac incoming IPC messages) is
+// during the call to RenderProcessWillLaunch() on ContentBrowserClient. This
+// class provides that for testing.
+class TestBrowserClient : public ChromeContentBrowserClient {
+ public:
+  TestBrowserClient() {}
+  ~TestBrowserClient() override {}
+
+  // ContentBrowserClient overrides.
+  void RenderProcessWillLaunch(
+      content::RenderProcessHost* process_host) override {
+    ChromeContentBrowserClient::RenderProcessWillLaunch(process_host);
+    filters_.push_back(
+        new content::TestTextInputClientMessageFilter(process_host));
+  }
+
+  // Retrieves the registered filter for the given RenderProcessHost. It will
+  // return false if the RenderProcessHost was initialized while a different
+  // instance of ContentBrowserClient was in action.
+  scoped_refptr<content::TestTextInputClientMessageFilter>
+  GetTextInputClientMessageFilterForProcess(
+      content::RenderProcessHost* process_host) const {
+    for (auto filter : filters_) {
+      if (filter->process() == process_host)
+        return filter;
+    }
+    return nullptr;
+  }
+
+ private:
+  std::vector<scoped_refptr<content::TestTextInputClientMessageFilter>>
+      filters_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestBrowserClient);
+};
+
+// This test verifies that requests for dictionary lookup based on selection
+// range are routed to the focused RenderWidgetHost.
+IN_PROC_BROWSER_TEST_F(SitePerProcessTextInputManagerTest,
+                       LookUpStringForRangeRoutesToFocusedWidget) {
+  // TestBrowserClient needs to replace the ChromeContenBrowserClient after most
+  // things are initialized but before the WebContents is created. Here we make
+  // that happen by creating a new WebContents in a new tab. But before the test
+  // exits, we must destroy the contents and replace the old
+  // ContentBrowserClient because the original WebContents and the new one have
+  // been initialized with the original ContentBrowserClient and the new
+  // TestBrowserClient, respectively.
+  TestBrowserClient browser_client;
+  content::ContentBrowserClient* old_browser_client =
+      content::SetBrowserClientForTesting(&browser_client);
+
+  content::WebContents* new_contents =
+      content::WebContents::Create(content::WebContents::CreateParams(
+          active_contents()->GetBrowserContext(), nullptr));
+  browser()->tab_strip_model()->InsertWebContentsAt(1, new_contents,
+                                                    TabStripModel::ADD_ACTIVE);
+  EXPECT_EQ(active_contents(), new_contents);
+
+  // Starting the test body.
+  CreateIframePage("a(b)");
+  std::vector<content::RenderFrameHost*> frames{GetFrame(IndexVector{}),
+                                                GetFrame(IndexVector{0})};
+  std::vector<content::RenderWidgetHostView*> views;
+  for (auto frame : frames)
+    views.push_back(frame->GetView());
+  std::vector<std::string> values{"main frame", "child frame"};
+
+  // Adding some field with text to each frame so that we can later query for
+  // dictionary lookup.
+  for (size_t i = 0; i < frames.size(); ++i)
+    AddInputFieldToFrame(frames[i], "text", values[i], true);
+
+  std::string result;
+  // Recording window count now so that our WindowCountObserver will detect the
+  // popup window.
+  size_t current_window_count = content::GetOpenNSWindowsCount();
+
+  // Ensure we have both focus and selected text inside the main frame.
+  EXPECT_TRUE(
+      ExecuteScript(frames[0], "document.querySelector('input').focus();"));
+
+  // Request for the dictionary lookup and intercept the word on its way back.
+  // The request is always on the tab's view which is a RenderWidgetHostViewMac.
+  content::AskForLookUpDictoinaryForRange(views[0], gfx::Range(0, 4));
+
+  // Wait until the result comes back.
+  auto root_filter = browser_client.GetTextInputClientMessageFilterForProcess(
+      frames[0]->GetProcess());
+  EXPECT_TRUE(root_filter);
+  root_filter->WaitForStringFromRange();
+
+  EXPECT_EQ("main", root_filter->string_from_range());
+
+  // Wait for the popup to appear to make sure TextInputClientMac has consumed
+  // the reply handler for the previous request.
+  WindowCountObserver(current_window_count).WaitForLimitOrMore();
+
+  // Ensure we have both focus and selected text inside the child frame.
+  EXPECT_TRUE(
+      ExecuteScript(frames[1], "document.querySelector('input').focus();"));
+
+  // Record window count again for the popup observer.
+  current_window_count = content::GetOpenNSWindowsCount();
+
+  // Make another request.
+  content::AskForLookUpDictoinaryForRange(views[0], gfx::Range(0, 5));
+
+  // Wait until the result comes back.
+  auto child_filter = browser_client.GetTextInputClientMessageFilterForProcess(
+      frames[1]->GetProcess());
+  child_filter->WaitForStringFromRange();
+
+  EXPECT_EQ("child", child_filter->string_from_range());
+
+  // Wait for the popup to appear to make sure TextInputClientMac has consumed
+  // the reply handler for the previous request.
+  WindowCountObserver(current_window_count).WaitForLimitOrMore();
+  // Test ends here. The rest is cleanup.
+
+  // Closing this WebContents while we still hold on to our TestBrowserClient.
+  EXPECT_TRUE(browser()->tab_strip_model()->CloseWebContentsAt(
+      1, TabStripModel::CLOSE_USER_GESTURE));
+
+  // For the cleanup of the original WebContents in tab index 0.
+  content::SetBrowserClientForTesting(old_browser_client);
+}
+#endif