Route IME Events to Focused RenderWidgets (Android)

content/browser/renderer_host/ime_adapter_android.cc

Index: content/browser/renderer_host/ime_adapter_android.cc
diff --git a/content/browser/renderer_host/ime_adapter_android.cc b/content/browser/renderer_host/ime_adapter_android.cc
index 3a7d18b94da7ce41b8a4981e98e7d87b3eda70cc..5815f81465a54eb9a57dd2eeb338a515569d02f0 100644
--- a/content/browser/renderer_host/ime_adapter_android.cc
+++ b/content/browser/renderer_host/ime_adapter_android.cc
@@ -142,7 +142,7 @@ void ImeAdapterAndroid::SetComposingText(JNIEnv* env,
                                          const JavaParamRef<jobject>& text,
                                          const JavaParamRef<jstring>& text_str,
                                          int relative_cursor_pos) {
-  RenderWidgetHostImpl* rwhi = GetRenderWidgetHostImpl();
+  RenderWidgetHostImpl* rwhi = GetFocusedWidget();
   if (!rwhi)
     return;
 
@@ -172,7 +172,7 @@ void ImeAdapterAndroid::CommitText(JNIEnv* env,
                                    const JavaParamRef<jobject>& text,
                                    const JavaParamRef<jstring>& text_str,
                                    int relative_cursor_pos) {
-  RenderWidgetHostImpl* rwhi = GetRenderWidgetHostImpl();
+  RenderWidgetHostImpl* rwhi = GetFocusedWidget();
   if (!rwhi)
     return;
 
@@ -195,7 +195,7 @@ void ImeAdapterAndroid::CommitText(JNIEnv* env,
 
 void ImeAdapterAndroid::FinishComposingText(JNIEnv* env,
                                             const JavaParamRef<jobject>&) {
-  RenderWidgetHostImpl* rwhi = GetRenderWidgetHostImpl();
+  RenderWidgetHostImpl* rwhi = GetFocusedWidget();
   if (!rwhi)
     return;
 
@@ -288,7 +288,7 @@ void ImeAdapterAndroid::DeleteSurroundingText(JNIEnv*,
 bool ImeAdapterAndroid::RequestTextInputStateUpdate(
     JNIEnv* env,
     const JavaParamRef<jobject>&) {
-  RenderWidgetHostImpl* rwhi = GetRenderWidgetHostImpl();
+  RenderWidgetHostImpl* rwhi = GetFocusedWidget();
   if (!rwhi)
     return false;
   rwhi->Send(new InputMsg_RequestTextInputStateUpdate(rwhi->GetRoutingID()));
@@ -300,7 +300,7 @@ void ImeAdapterAndroid::RequestCursorUpdate(
     const base::android::JavaParamRef<jobject>& obj,
     bool immediate_request,
     bool monitor_request) {
-  RenderWidgetHostImpl* rwhi = GetRenderWidgetHostImpl();
+  RenderWidgetHostImpl* rwhi = GetFocusedWidget();
   if (!rwhi)
     return;
   rwhi->Send(new InputMsg_RequestCompositionUpdate(
@@ -312,7 +312,7 @@ void ImeAdapterAndroid::ResetImeAdapter(JNIEnv* env,
   java_ime_adapter_.reset();
 }
 
-RenderWidgetHostImpl* ImeAdapterAndroid::GetRenderWidgetHostImpl() {
+RenderWidgetHostImpl* ImeAdapterAndroid::GetPageRenderWidgetHostImpl() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(rwhva_);
   RenderWidgetHost* rwh = rwhva_->GetRenderWidgetHost();

[Charlie Reis, 2017/01/25 22:00:22]

Isn't this the RWH associated with RenderWidgetHostViewAndroid?  For an OOPIF,
wouldn't that be a subframe widget rather than a "page" (main frame?) widget? 
I'm confused about calling this "page" level.

[EhsanK, 2017/01/26 19:16:29]

I think |rwhva_| is always a RenderWidgetHostViewAndroid (that is the type as
defined in the header); and that should always be the page's view? Also
RenderWidgetHostViewAndroid is the only thing which creates the
ImeAdapterAndroid (AFAIK) and passes its own reference as a RWHVA.

[Charlie Reis, 2017/01/27 01:07:22]

Ah yes, I was confused about how RWHVs work (with platform views at the top and
ChildFrame views for OOPIFs).

[EhsanK, 2017/01/27 16:16:46]

Acknowledged.

@@ -322,8 +322,14 @@ RenderWidgetHostImpl* ImeAdapterAndroid::GetRenderWidgetHostImpl() {
   return RenderWidgetHostImpl::From(rwh);
 }
 
+RenderWidgetHostImpl* ImeAdapterAndroid::GetFocusedWidget() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);

[EhsanK, 2017/01/26 19:16:29]

If it wasn't for this threading DCHECK, I could remove GetFocusedWidget() and
use rwhva_->GetFocusedWidget() instead.

[Charlie Reis, 2017/01/27 01:07:22]

That DCHECK isn't strictly necessary, but given the amount of thought we're
putting in to which method is the right one to call, it's probably worth having
this helper method for it.

[EhsanK, 2017/01/27 16:16:46]

Acknowledged.

+  DCHECK(rwhva_);

[EhsanK, 2017/01/25 19:27:47]

This DCHECK here and everywhere else in this file seems unnecessary.  I only
added it to conform to the way code was before (specifically with
GetPageRenderWidgetHostImpl). It is also inconsistently used since in
SendKeyEvent we do not DCHECK.

This class is created by RWHVAndroid which passes a reference to itself in the
CTOR of this class. I would suggest we remove all such DCHECKs (either in this
CL or in another) and if needed, add one DCHECK to the CTOR. WDYT?

[Charlie Reis, 2017/01/25 22:00:22]

Looks like the only previous use of it is in GetPageRenderWidgetHostImpl, and I
agree that it's unnecessary there or here.  Moving it to the constructor in this
CL sounds good.

[EhsanK, 2017/01/26 19:16:29]

Acknowledged.

+  return rwhva_->GetFocusedWidget();

[Charlie Reis, 2017/01/25 22:00:22]

I'm getting confused looking at RenderWidgetHostViewBase::GetFocusedWidget(),
where this is implemented.  We pass the RWHV's widget into
WebContentsImpl::GetFocusedRenderWidgetHost, which returns that same widget if
it's not equal to the main frame's current widget (with a comment about popup
menus).

Wouldn't this be broken for OOPIFs?  In other words, if an OOPIF's RWHV calls
GetFocusedWidget, it will always get back its own RWH.  Am I reading that
incorrectly?

[EhsanK, 2017/01/26 19:16:29]

Since |rwhva_| is a RenderWidgetHostViewAndroid and is for the page, I believe
what we are doing here is correct. This is supposed to return the widget inside
the page (or under the tab's view) which takes keyboard focus. This is also the
same thing we have been doing for other platforms.

[Charlie Reis, 2017/01/27 01:07:22]

Thanks-- that was the same mixup I had above.

[EhsanK, 2017/01/27 16:16:47]

Acknowledged.

+}
+
 RenderFrameHost* ImeAdapterAndroid::GetFocusedFrame() {
-  RenderWidgetHostImpl* rwh = GetRenderWidgetHostImpl();
+  RenderWidgetHostImpl* rwh = GetPageRenderWidgetHostImpl();

[Charlie Reis, 2017/01/25 22:00:22]

What's the reason for GetFocusedFrame() use GetPageRenderWidgetHostImpl()
instead of GetFocusedWidget()?  Is there a case that GetFocusedWidget() would
return null while there's a focused frame, or some other case that they differ?

[EhsanK, 2017/01/26 19:16:29]

I don't think we can get to RenderFrameHostImpl from RenderWidgetHostImpl at all
(can we?).
Yes, I believe there is. In WebContentsImpl::GetFocusedRenderWidgetHost() we
will get nullptr if the focused widget's view is nullptr. Therefore,
GetFocusedFrame()->GetRenderWidgetHost() could be non-nullptr when
GetFocusedWidget() returns null. I think in those cases we should not proceed
with keyboard input IPCs.

[Charlie Reis, 2017/01/27 01:07:21]

Ok.

[EhsanK, 2017/01/27 16:16:47]

Acknowledged.

   if (!rwh)
     return nullptr;
   RenderViewHost* rvh = RenderViewHost::From(rwh);
@@ -338,7 +344,7 @@ RenderFrameHost* ImeAdapterAndroid::GetFocusedFrame() {
 }
 
 WebContents* ImeAdapterAndroid::GetWebContents() {
-  RenderWidgetHostImpl* rwh = GetRenderWidgetHostImpl();
+  RenderWidgetHostImpl* rwh = GetPageRenderWidgetHostImpl();

[Charlie Reis, 2017/01/25 22:00:22]

Again, just wondering whether we need GetPageRenderWidgetHostImpl at all.  It's
ok if so.

[EhsanK, 2017/01/26 19:16:29]

We actually don't since
RenderWidgetHostImpl::From(rwhva_->GetRenderWidgetHost()) could replace that. I
also used rwh->delegate()->GetAsWebContents() here since it will need only one
cast from RenderWidgetHostDelegate to WebContents. The way it is now casts
RenderWidgetHostOwnerDelegate to RenderViewHost and then RenderViewHostDelegate
to WebContents. WDYT?

[Charlie Reis, 2017/01/27 01:07:21]

Sure.

[EhsanK, 2017/01/27 16:16:46]

Acknowledged.

   if (!rwh)
     return nullptr;
   return WebContents::FromRenderViewHost(RenderViewHost::From(rwh));