Fix a recent regression in IME inside OOPIFs

content/renderer/render_widget.cc

 // Copyright (c) 2012 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 "content/renderer/render_widget.h"
 
 #include <memory>
 #include <utility>
 
 #include "base/auto_reset.h"
@@ skipping 641 matching lines @@
 
   return RenderThread::Get()->Send(message);
 }
 
 void RenderWidget::SendOrCrash(IPC::Message* message) {
   bool result = Send(message);
   CHECK(closing_ || result) << "Failed to send message";
 }
 
 bool RenderWidget::ShouldHandleImeEvents() const {
-  return GetWebWidget()->isWebFrameWidget() && has_focus_;
+  // TODO(ekaramad): We do not track page focus for OOPIFs so for now we will
+  // assume this method always returns true.
+  return GetWebWidget()->isWebFrameWidget() && (has_focus_ || for_oopif_);

[alexmos, 2017/02/06 23:47:46]

I was a bit surprised that has_focus_ isn't tracking page focus for OOPIFs,
because I thought I fixed that in https://codereview.chromium.org/1394383002 so
that it's replicated across all processes involved in a page.  But digging in a
bit, it seems these replicated has_focus_ bits are only kept in the RenderView
in each process, but the RenderWidgets corresponding to OOPIF WebFrameWidgets
indeed aren't tracking it.  So is it possible to fix this by looking up the
state kept in the RV instead?  We could also consider copying the page focus to
all RenderWidgets on the page, although that would be duplicating the page focus
state (ideally, it'd be on RenderPage, but that doesn't exist yet :) ).

[EhsanK, 2017/02/07 07:02:13]

Yes. I did notice this issue (and explained the problem I had briefly in the bug
associated with the CL).  I think it is best we somehow fix the page focus in
OOPIFs. That being said, the only use cases I found are:
1- WebFrameWidget::setFocus() call.
2- BrowserPlugin
3- IME
4- Plugins in OOPIFs.

(1) might or might not be important (probably is). Applications are finishing an
ongoing composition (IME) and some autofill client thing I don't know.
(2) shouldn't be since it only relates to BP focus inside OOPIF (which is PDF
for now), but, will eventually go away with BP itself.
(3) Used to be noticeably important since I remember not considering page focus
(accepting IPCs regardless of it) would lead to double commits when switching
tabs during IME compositions. However, with this CL which basically always
allows the IPC call to go through, I can no longer reproduce the double commit
issue. So it seems like this one might not be that important.
(4) I guess this one is also important since it sets the focus area for a pepper
plugin.
I cannot find an easy solution as we are not exposing RenderView/RenderFrame to
RenderWidget. The only link I know is the owner delegate, to which we are not
supposed to add methods (I believe). 
I guess here are our viable options:

1- Copy the state/call to all RenderWidgets. As you suggested,
page->focusController()->setFocus is called for  as many render widgets as we
have on page (at least twice I guess).

2- Only call it on the focused RenderWidget (and possibly avoid calling it on
swapped out RenderView). This was also suggested in your design doc referenced
in the CL you mentioned.

3- Maybe, create a RenderWidgetDelegate class (lfg@ suggested this once before
to help get rid of the pointer to focused pepper plugin we have in render
widget) and use it to query the state. I am not sure if this would be the same
as the RenderPage you mentioned. FWIW, it can be a set of method calls to page
level stuff we need from RenderWidget which can then be added to RenderPage
instead.

WDYT?

[alexmos, 2017/02/08 01:42:20]

Yeah, this is tricky.  I've also chatted with Daniel about this, and we think
it's ok to go with this temporary fix for now, and investigate the right fix in
a followup.  Let's update the comment to reference the bug # and perhaps instead
of saying "We do not track page focus for OOPIFs", you could say that we do
track it, but it's in RenderView and we need to figure out how to access it from
here.  (I'll also review the rest of this and post any other comments
separately.)

As for options to fix this for real, I'm still not too thrilled about option (1)
(it feels wrong to call FocusController::setFocused multiple times, for
example).  (2) might break other dependencies that currently look up
RenderView::has_focus_?  (3) would work; I guess we already have
RenderWidgetOwnerDelegate, so we could expose a similar interface, but actually,
if we're going there, Nasko/Daniel/I thought it might not be so bad to just make
RenderWidget know about its RenderView and have a getter for it.

It might also be possible to get to the RenderView without adding anything by
going through RenderWidget -> WebFrameWidget -> localRoot() -> convert to
RenderFrameImpl -> GetRenderView().  Pretty horrible, but might work. :)

Another option is to expose page focus from the Blink public API (e.g., expose
FocusController::isFocused() from WedWidget::isFocused()).  Currently, we keep
track of page focus both in content and in blink, which is sad, and this might
let us remove RV::has_focus_ completely and just look this up in Blink instead. 
I'm not sure this will work for all call sites (e.g., for plugins), but it seems
like a good approach if it does let us remove page focus tracking from content/.

RenderPage is probably a longer-term thing that probably won't come to life
until we're ready to start removing RenderView.

[EhsanK, 2017/02/08 02:35:50]

Thanks Alex.

Two comments:
1- By not finding an easy way to use RenderView I also meant (I was under the
impression that) we do not reference to RenderView and RenderFrame because they
are not supposed to be exposed. But if exposing RenderView inside RenderWidget
is fine, I could definitely use that (keeping a reference to RenderView looks
very tempting to me).

2- I also agree we should land this and solve page focus in a separate CL. We
still need to either remove WebFrameWidgetImpl::setFocus, or if we do have to
run some of the logic there, propagate the calls from WebView/RenderView. 
but that would at least require changes to the function to avoid issues like
calling focusController()->setFocused multiple times.

3- Also isFocused() could be exposed in WebView (assuming that RenderWidget gets
to have a reference to RenderView).

 }
 
 void RenderWidget::SetWindowRectSynchronously(
     const gfx::Rect& new_window_rect) {
   ResizeParams params;
   params.screen_info = screen_info_;
   params.new_size = new_window_rect.size();
   params.physical_backing_size =
       gfx::ScaleToCeiledSize(new_window_rect.size(), device_scale_factor_);
   params.visible_viewport_size = new_window_rect.size();
@@ skipping 912 matching lines @@
 #endif
   if (replacement_range.IsValid()) {
     GetWebWidget()->applyReplacementRange(
         WebRange(replacement_range.start(), replacement_range.length()));
   }
 
   if (!GetWebWidget())
     return;
   ImeEventGuard guard(this);
   input_handler_->set_handling_input_event(true);
-  if (auto* controller = GetInputMethodController())
+  if (auto* controller = GetInputMethodController()) {
     controller->commitText(WebString::fromUTF16(text),
                            WebVector<WebCompositionUnderline>(underlines),
                            relative_cursor_pos);
+  }

[EhsanK, 2017/02/06 17:46:41]

Just a formatting fix (irrelevant to the actual issue).

   input_handler_->set_handling_input_event(false);
   UpdateCompositionInfo(false /* not an immediate request */);
 }
 
 void RenderWidget::OnImeFinishComposingText(bool keep_selection) {
   if (!ShouldHandleImeEvents())
     return;
 
 #if BUILDFLAG(ENABLE_PLUGINS)
   if (focused_pepper_plugin_) {
@@ skipping 672 matching lines @@
     // browser side (https://crbug.com/669219).
     // If there is no WebFrameWidget, then there will be no
     // InputMethodControllers for a WebLocalFrame.
     return nullptr;
   }
   return static_cast<blink::WebFrameWidget*>(GetWebWidget())
       ->getActiveWebInputMethodController();
 }
 
 }  // namespace content