Making Mac Dictionary better for OOPIFs and <webview>

chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc

Index: chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc
diff --git a/chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc b/chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc
index 2a7094e3c82898e2e33b0e7db4cb8c9f83352e14..17cdb418bfa3ed43d16549ac82413c5523fcecb1 100644
--- a/chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc
+++ b/chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc
@@ -9,6 +9,7 @@
 #include "base/single_thread_task_runner.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/test_timeouts.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "chrome/app/chrome_command_ids.h"
@@ -1362,6 +1363,76 @@ IN_PROC_BROWSER_TEST_P(WebViewInteractiveTest, TextSelection) {
   ASSERT_TRUE(selected_text.size() >= 10u);
   ASSERT_EQ("AAAAAAAAAA", selected_text.substr(0, 10));
 }
+
+// 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 {

[lazyboy, 2017/04/13 16:40:45]

nit: rename class too less generic and more specific for text input client.
Also can we put this around the top of the file where other classes are?

[EhsanK, 2017/04/13 17:56:15]

Done.

+ 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);
+};
+
+// Verifies that asking for a word lookup from a guest's RWHV will open the
+// dictionary popup window.

[lazyboy, 2017/04/13 16:40:45]

I couldn't figure out which part is checking the popup window. Can you explain a
bit or update the description? I could only find that the test waits for IPC in
line 1429 below.

[EhsanK, 2017/04/13 17:56:15]

Yes sorry about the bad comment. That is what the test does. Getting the IPC
means RWHVCocoa will call showDefinitionForAttributedString which is supposed to
popup the dict.

+IN_PROC_BROWSER_TEST_P(WebViewInteractiveTest, WordLookUp) {

[lazyboy, 2017/04/13 16:40:45]

nit: WordLookup (small u) or LookUpWord

[EhsanK, 2017/04/13 17:56:15]

Done.

+  // TestBrowserClient needs to replace the ChromeContentBrowserClient after
+  // most things are initialized but before the WebContents is created.
+  TestBrowserClient browser_client;
+  content::ContentBrowserClient* old_browser_client =
+      content::SetBrowserClientForTesting(&browser_client);
+
+  SetupTest("web_view/text_selection",
+            "/extensions/platform_apps/web_view/text_selection/guest.html");
+  ASSERT_TRUE(guest_web_contents());
+  ASSERT_TRUE(ui_test_utils::ShowAndFocusNativeWindow(GetPlatformAppWindow()));
+
+  auto guest_message_filter =
+      browser_client.GetTextInputClientMessageFilterForProcess(
+          guest_web_contents()->GetRenderProcessHost());
+  EXPECT_TRUE(guest_message_filter);

[lazyboy, 2017/04/13 16:40:45]

Since we need this to be non-null in line 1429, we should ASSERT_TRUE

[EhsanK, 2017/04/13 17:56:15]

Done. Thanks!

+
+  // Lookup some string through context menu.
+  ContextMenuNotificationObserver menu_observer(IDC_CONTENT_CONTEXT_LOOK_UP);
+  SimulateRWHMouseClick(guest_web_contents()->GetRenderViewHost()->GetWidget(),

[lazyboy, 2017/04/13 16:40:45]

Can you add a quick note about what the significance of (20,20) is? (Of course
that's my fault for not putting the note in the original test).

[EhsanK, 2017/04/13 17:56:15]

Sure. I guess that is where the text is (to be highlighted).

+                        blink::WebMouseEvent::Button::kRight, 20, 20);
+  // Wait for the response form the guest renderer.
+  guest_message_filter->WaitForStringFromRange();
+
+  // Sanity check.
+  ASSERT_EQ("AAAA", guest_message_filter->string_from_range().substr(0, 4));
+
+  content::SetBrowserClientForTesting(old_browser_client);

[lazyboy, 2017/04/13 16:40:45]

It's better to use a separate class that sets the new client in its constructor
and reset it back in the destructor. That will ensure we always reset the client
regardless of ASSERTs above:
ScopedBrowserClient {
  ScopedBrowserClient(browser_client) { old_ =
SetBrowserClientForTesting(browser_client); }
  ~ScopedBrowserClient() { SetBrowserClientForTesting(old_); }
};

[EhsanK, 2017/04/13 17:56:15]

Thanks for the suggestion. I implemented something similar.

+}
 #endif
 
 IN_PROC_BROWSER_TEST_P(WebViewFocusInteractiveTest, FocusAndVisibility) {