Fix a recent regression in IME inside OOPIFs

chrome/browser/renderer_host/site_per_process_text_input_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 <vector>
 
 #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"
@@ skipping 774 matching lines @@
   for (size_t i = 0; i < views.size(); ++i) {
     // First focus the <input>.
     send_tab_and_wait_for_value(values[i]);
 
     // Send a sequence of |count| 'E' keys and wait until the view receives a
     // selection change update for a text of the corresponding size, |count|.
     send_keys_select_all_wait_for_selection_change(views[i], count++);
   }
 }
 
+// This test verifies that committing text works as expected for all the frames
+// on the page. Specifically, the test sends an IPC to the RenderWidget
+// corresponding to a focused frame with a focused <input> to commit some text.
+// Then, it verifies that the <input>'s value matches the committed text
+// (https://crbug.com/688842).
+IN_PROC_BROWSER_TEST_F(SitePerProcessTextInputManagerTest,
+                       ImeCommitTextForAllFrames) {
+  CreateIframePage("a(b,c(a))");
+  std::vector<content::RenderFrameHost*> frames{
+      GetFrame(IndexVector{}), GetFrame(IndexVector{0}),
+      GetFrame(IndexVector{1}), GetFrame(IndexVector{1, 0})};
+  for (size_t i = 0; i < frames.size(); ++i)

[sky, 2017/02/08 16:36:53]

Do you need an assertion that the RenderFrameHosts are non-null?

[EhsanK, 2017/02/09 00:50:34]

I believe not since it should be inferred from line 802 (each letter will create
one frame). This uses a template html file used in almost all site per process
(browser) tests.

For instance here:
https://cs.chromium.org/chromium/src/chrome/browser/site_per_process_interact...

We don't assert frame sizes in any of those tests. I will add the assert if you
believe it is necessary.

+    AddInputFieldToFrame(frames[i], "text", "", true);
+
+  auto focus_input_in_frame = [](content::RenderFrameHost* frame) {

[sky, 2017/02/08 16:36:53]

Why are you using closures here and 814? Closures are great when you have a
function that takes a closure, but that isn't the case here. For example, this
closure is used in one place, line 828. Changing that line to:

  EXPECT_TRUE(ExecuteScript(...));

Is a lot more readable.

[EhsanK, 2017/02/09 00:50:34]

Thanks and Done. I usually use lambdas to avoid repeating the same code, but in
this case, I was hoping it will enhance readability (which I guess it did not).
I am using the scripts with some comments now.

+    EXPECT_TRUE(ExecuteScript(
+        frame, "window.focus(); document.querySelector('input').focus();"));
+  };
+
+  auto get_input_value_from_frame = [](content::RenderFrameHost* frame) {
+    std::string result;
+    EXPECT_TRUE(ExecuteScriptAndExtractString(frame,
+                                              "window.domAutomationController."
+                                              "send(document.querySelector('"
+                                              "input').value);",
+                                              &result));
+    return result;
+  };
+
+  std::string sample_text[4U] = {"main", "child_b", "child_c", "child_a"};
+

[sky, 2017/02/08 16:36:53]

Assert the size of sample_text is the same as frames?

[EhsanK, 2017/02/09 00:50:34]

Done.

+  for (size_t index = 0; index < frames.size(); ++index) {
+    // Focus the frame and the <input> inside it.
+    focus_input_in_frame(frames[index]);

[sky, 2017/02/08 16:36:53]

If this fails, is it really worth continuing? I'm basically wondering if the
EXPECT here should be an ASSERT. Also, be sure to redirect the index to all
these so if there is a failure you know which frame it was.

[EhsanK, 2017/02/09 00:50:34]

Done. We should not continue if any of these scripts are not executed. Thanks!

+
+    // Commit some text.
+    content::SendImeCommitTextToWidget(
+        frames[index]->GetView()->GetRenderWidgetHost(),
+        base::UTF8ToUTF16(sample_text[index]),
+        std::vector<ui::CompositionUnderline>(), gfx::Range(), 0);
+
+    // Running a NOP js code to make sure the <input> text is updated.

[sky, 2017/02/08 16:36:53]

How do you know the SendImeCommitTextToWidget is completed by the time the no-op
runs?

[EhsanK, 2017/02/09 00:50:34]

Thanks for pointing this out. Without this line the test would fail due to
<input> being empty most of the time. But this is also not the correct fix as it
turns out.

Running JS code and setting composition both translate into sending an IPC to
the renderer (through the same channel) and handled on the same thread.

Removing the NOP JS code, for each frame, we send IPCs in this order:
1- JS code for focusing input
2- Commit text
3- JS code to retrieve the input value

So I expected these IPCs to be sent and received in the same exact order.

I put some LOG(INFO) inside both RenderProcessHostImpl::Send and RenderFrameImpl
and RenderWidget handlers and noticed the handling sometimes becomes out of
order, as in we do
1- JS code for focusing input
2- JS code to retrieve the input value
3- Commit text.

I don't think the order is messed up because of how LOG dumps text given that
all the LOGs in the renderer are in the same process and same thread (or does
it?). This order makes sense in scenarios that the test fails. That being said,
I don't know how the ordering could be messed up at the child process or render
thread.

So in conclusion, I removed this and reworked the testing method to now rely on
an ACK (through text selection update) from the renderer when the input event
occurs.

+    EXPECT_TRUE(ExecuteScript(frames[index], "var nop = true;"));
+
+    // Query <input>.value and make sure it matches the text we sent to commit.
+    EXPECT_EQ(sample_text[index], get_input_value_from_frame(frames[index]));
+  }
+}
+
 // TODO(ekaramad): Some of the following tests should be active on Android as
 // well. Enable them when the corresponding feature is implemented for Android
 // (https://crbug.com/602723).
 #if !defined(OS_ANDROID)
 // This test creates a page with multiple child frames and adds an <input> to
 // each frame. Then, sequentially, each <input> is focused by sending a tab key.
 // Then, after |TextInputState.type| for a view is changed to text, another key
 // is pressed (a character) and then the test verifies that TextInputManager
 // receives the corresponding update on the change in selection bounds on the
 // browser side.
@@ skipping 549 matching lines @@
 
   // 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  //  defined(MAC_OSX)
 #endif  // !defined(OS_ANDROID)