Fix a recent regression in IME inside OOPIFs

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 dc71dce007b1342a5dcc0cff1e41e7e4e749ce3b..36173dffdac7706ed41e0eaf65f62c354365a5ed 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
@@ -792,6 +792,55 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessTextInputManagerTest,
   }
 }
 
+// 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).