Track TextInputState from multiple RenderWidgets in TextInputManager (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 8cd594b17a96f4c99ca44d3d979198f62d27ef55..c8eda246b549b0857fdeaf352796f6dc84b51992 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
@@ -26,9 +26,10 @@
 #include "ui/base/ime/text_input_type.h"
 #include "url/gurl.h"
 
-// TODO(ekaramad): The following tests are only active on aura platforms. After
-// fixing crbug.com/578168 for all platforms, the following tests should be
-// activated for other platforms, e.g., Mac and Android (crbug.com/602723).
+// TODO(ekaramad): The following tests should be active on all platforms. After
+// fixing https://crbug.com/578168, this test file should be built for android
+// as well and most of the following tests should be enabled for all platforms
+//(https://crbug.com/602723).
 
 ///////////////////////////////////////////////////////////////////////////////
 // TextInputManager and IME Tests
@@ -536,6 +537,9 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessTextInputManagerTest,
   reset_state_observer.Wait();
 }
 
+// TODO(ekaramad): Enable the following tests on other platforms when the
+// corresponding feature is implemented (http://crbug.com/578168).
+#if defined(USE_AURA)

[EhsanK, 2016/07/27 16:46:32]

I guess all of the following will eventually be active run on Mac as well. But
some, if not all, will not run on Android as the selection related state is
updated differently on Android. So we will have to deal with some #ifdefs for
life.

 // 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
@@ -646,11 +650,13 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessTextInputManagerTest,
   for (size_t i = 0; i < frames.size(); ++i)
     send_tab_and_wait_for_selection_change(frames[i], input_text[i].size());
 }
+#endif  // USE_AURA
 
 // TODO(ekaramad): The following tests are specifically written for Aura and are
 // based on InputMethodObserver. Write similar tests for Mac/Android/Mus
 // (crbug.com/602723).

[Charlie Reis, 2016/07/27 16:14:18]

This part confuses me a bit.  Is there a meaningful difference between the
Aura-only block above and the Aura-only block below?  Both seem to have TODOs to
make them work on other platforms, so I'm wondering if it's simpler to combine
them.

(Or do the InputMethodObserver-specific tests deserve a block of their own?  I'm
ok with that if so.)

[EhsanK, 2016/07/27 16:46:32]

Sorry for the confusion. The block above is Aura only because I have not
implemented the features. I expect them to move to Mac + Aura 1by1 as I finish
IME for Mac.

The ones below were originally intended to be InputMethodObserver tests as you
suggest. But as of now I have the only one below for aura which is actually a
unit test. I guess I should be able to move it to unit tests and get rid of this
block for good.

It will be ideal though to have proper end-to-end tests for IME which will only
use IME extension (which only works on ChromeOS for now). I will try to revisit
this issue after I investigate the IME api in more depths. Although, the IME API
has its own tests for ChromeOS.

 
+#if defined(USE_AURA)
 // Observes current input method for state changes.
 class InputMethodObserverBase {
  public:
@@ -756,3 +762,4 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessTextInputManagerTest,
   sender.SetType(ui::TEXT_INPUT_TYPE_NONE);
   EXPECT_FALSE(send_and_check_show_ime());
 }
+#endif