Fix a recent regression in IME inside OOPIFs

Fix a recent regression in IME inside OOPIFs

IME events are only processed at the renderer if there is widget/page
focus. This used to be done by checking the variabel m_imeAcceptEvents
inside WebFrameWidgetImpl which technically tracks the page focus.

After a recent refactoring, the logic for verifying page focus was moved
to RenderWidget which saves the unnecessary call to
WebInputMethodController IME methods when page is not focused. But
unfortunately this broke IME because page focus
(RenderWidget::has_focus_) does not get updated for RenderWidgets
corresponding to OOPIFs. The only reason it was working before was that
m_imeAcceptEvents is initialized to |true| and stay because we never
update page focus for WebFrameWidgetImpl.

This CL will temporarily allow handling all IME events by checking if
the RenderWidget is for an OOPIF. This will revert the behvaior to that
of prior to the refactoring.

This CL also adds an interactive browser test which catches regressions
of this sort on Mac and Aura platforms.

BUG=688842, 602723
Review-Url: https://codereview.chromium.org/2681473002
Cr-Commit-Position: refs/heads/master@{#449521}
Committed: https://chromium.googlesource.com/chromium/src/+/b6483a05119a0ebb57d3968783371c119a83d49c

[EhsanK, 2017-02-06 16:58:43]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[EhsanK, 2017-02-06 16:59:03]

Description was changed from

==========
Fix recent regression in IME inside OOPIFs

IME events are only processed at the renderer if there is widget/page
focus. This used to be done by checking the variabel m_imeAcceptEvents
inside WebFrameWidgetImpl which technically tracks the page focus.

After a recent refactoring, the logic for verifying page focus was moved
to RenderWidget which saves the unnecessary call to
WebInputMethodController IME methods when page is not focused. But
unfortunately this broke IME because page focus
(RenderWidget::has_focus_) does not get updated for RenderWidgets
corresponding to OOPIFs. The only reason it was working before was that
m_imeAcceptEvents is initialized to |true| and stay because we never
update page focus for WebFrameWidgetImpl.

This CL will temporarily allow handling all IME events by checking if
the RenderWidget is for an OOPIF. This will revert the behvaior to that
of prior to the refactoring.

This CL also adds an interactive browser test which catches regressions
of this sort on Mac and Aura platforms.

BUG=688842, 602723
==========

to

==========
Fix a recent regression in IME inside OOPIFs

IME events are only processed at the renderer if there is widget/page
focus. This used to be done by checking the variabel m_imeAcceptEvents
inside WebFrameWidgetImpl which technically tracks the page focus.

After a recent refactoring, the logic for verifying page focus was moved
to RenderWidget which saves the unnecessary call to
WebInputMethodController IME methods when page is not focused. But
unfortunately this broke IME because page focus
(RenderWidget::has_focus_) does not get updated for RenderWidgets
corresponding to OOPIFs. The only reason it was working before was that
m_imeAcceptEvents is initialized to |true| and stay because we never
update page focus for WebFrameWidgetImpl.

This CL will temporarily allow handling all IME events by checking if
the RenderWidget is for an OOPIF. This will revert the behvaior to that
of prior to the refactoring.

This CL also adds an interactive browser test which catches regressions
of this sort on Mac and Aura platforms.

BUG=688842, 602723
==========

[commit-bot: I haz the power, 2017-02-06 16:59:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[EhsanK, 2017-02-06 17:46:40]

ekaramad@chromium.org changed reviewers:
+ alexmos@chromium.org, creis@chromium.org

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

Hello Alex,

I think Charlie is OOO. I was wondering if you could review this instead.
Thanks!

https://codereview.chromium.org/2681473002/diff/1/content/renderer/render_wid...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2681473002/diff/1/content/renderer/render_wid...
content/renderer/render_widget.cc:1601: }
Just a formatting fix (irrelevant to the actual issue).

[commit-bot: I haz the power, 2017-02-06 17:54:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-06 17:54:45]

Dry run: This issue passed the CQ dry run.

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

Thanks, one question below before I review the rest.

https://codereview.chromium.org/2681473002/diff/1/content/renderer/render_wid...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2681473002/diff/1/content/renderer/render_wid...
content/renderer/render_widget.cc:664: return GetWebWidget()->isWebFrameWidget()
&& (has_focus_ || for_oopif_);
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]

Thank you Alex. I added some comments to the question you asked. Please let me
know which approach you would like me to take and if we should fix focus in this
CL.

PTAL.

https://codereview.chromium.org/2681473002/diff/1/content/renderer/render_wid...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2681473002/diff/1/content/renderer/render_wid...
content/renderer/render_widget.cc:664: return GetWebWidget()->isWebFrameWidget()
&& (has_focus_ || for_oopif_);
On 2017/02/06 23:47:46, alexmos wrote:
> 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. 
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.

> So is it possible to fix this by looking up the
> state kept in the RV instead?
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). 

> 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 :) ).
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]

https://codereview.chromium.org/2681473002/diff/1/content/renderer/render_wid...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2681473002/diff/1/content/renderer/render_wid...
content/renderer/render_widget.cc:664: return GetWebWidget()->isWebFrameWidget()
&& (has_focus_ || for_oopif_);
On 2017/02/07 07:02:13, EhsanK wrote:
> On 2017/02/06 23:47:46, alexmos wrote:
> > 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. 
> 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.
> 
> > So is it possible to fix this by looking up the
> > state kept in the RV instead?
> 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). 
> 
> > 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 :) ).
> 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?

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.

[alexmos, 2017-02-08 02:16:23]

https://codereview.chromium.org/2681473002/diff/1/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2681473002/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:795: //
This test verifies that commit text by sending the IPC works for all frames
nit: commit text -> committing text?

https://codereview.chromium.org/2681473002/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:797: //
current has focus and a focused <input> and then verifies that the <input>
nit: s/current/currently/

https://codereview.chromium.org/2681473002/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:798: //
value equals to the text to commit.
Might be useful to reference https://crbug.com/688842.

https://codereview.chromium.org/2681473002/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:825: for
(size_t index = 0; index < 1U; ++index) {
Should 1U be frames.size() instead?

https://codereview.chromium.org/2681473002/diff/1/content/public/test/text_in...
File content/public/test/text_input_test_utils.cc (right):

https://codereview.chromium.org/2681473002/diff/1/content/public/test/text_in...
content/public/test/text_input_test_utils.cc:273:
web_composition_underlines.push_back(blink::WebCompositionUnderline(
optional: emplace_back would let you avoid explicitly writing
blink::WebCompositionUnderline().

[EhsanK, 2017-02-08 02:34:53]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 02:35:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

Thanks Alex! I filed a new bug for page focus and tried to fix the comments.
PTAL.

https://codereview.chromium.org/2681473002/diff/1/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2681473002/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:795: //
This test verifies that commit text by sending the IPC works for all frames
On 2017/02/08 02:16:23, alexmos wrote:
> nit: commit text -> committing text?

Acknowledged.

https://codereview.chromium.org/2681473002/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:797: //
current has focus and a focused <input> and then verifies that the <input>
On 2017/02/08 02:16:23, alexmos wrote:
> nit: s/current/currently/

Acknowledged. I redid this paragraph. Sorry for the many typos.

https://codereview.chromium.org/2681473002/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:798: //
value equals to the text to commit.
On 2017/02/08 02:16:23, alexmos wrote:
> Might be useful to reference https://crbug.com/688842.

Acknowledged.

https://codereview.chromium.org/2681473002/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:825: for
(size_t index = 0; index < 1U; ++index) {
On 2017/02/08 02:16:23, alexmos wrote:
> Should 1U be frames.size() instead?

Yes. good catch! left over from testing my test :).

https://codereview.chromium.org/2681473002/diff/1/content/renderer/render_wid...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2681473002/diff/1/content/renderer/render_wid...
content/renderer/render_widget.cc:664: return GetWebWidget()->isWebFrameWidget()
&& (has_focus_ || for_oopif_);
On 2017/02/08 01:42:20, alexmos wrote:
> On 2017/02/07 07:02:13, EhsanK wrote:
> > On 2017/02/06 23:47:46, alexmos wrote:
> > > 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. 
> > 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.
> > 
> > > So is it possible to fix this by looking up the
> > > state kept in the RV instead?
> > 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). 
> > 
> > > 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 :) ).
> > 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?
> 
> 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.

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).

[commit-bot: I haz the power, 2017-02-08 03:32:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 03:32:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alexmos, 2017-02-08 06:02:49]

Thanks, LGTM

[EhsanK, 2017-02-08 06:06:42]

The CQ bit was checked by ekaramad@chromium.org

[commit-bot: I haz the power, 2017-02-08 06:06:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 06:14:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 06:14:26]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[EhsanK, 2017-02-08 06:18:25]

ekaramad@chromium.org changed reviewers:
+ sky@chromium.org

[EhsanK, 2017-02-08 06:18:26]

Hello Scott,

Charlie is OOO so could you please take a look?

Thanks!

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

https://codereview.chromium.org/2681473002/diff/80001/chrome/browser/renderer...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2681473002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:806: for
(size_t i = 0; i < frames.size(); ++i)
Do you need an assertion that the RenderFrameHosts are non-null?

https://codereview.chromium.org/2681473002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:809:
auto focus_input_in_frame = [](content::RenderFrameHost* frame) {
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.

https://codereview.chromium.org/2681473002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:825: 
Assert the size of sample_text is the same as frames?

https://codereview.chromium.org/2681473002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:828:
focus_input_in_frame(frames[index]);
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.

https://codereview.chromium.org/2681473002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:836: //
Running a NOP js code to make sure the <input> text is updated.
How do you know the SendImeCommitTextToWidget is completed by the time the no-op
runs?

[EhsanK, 2017-02-09 00:49:55]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 00:50:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

Thanks Scott!

Please take another look.

https://codereview.chromium.org/2681473002/diff/1/content/public/test/text_in...
File content/public/test/text_input_test_utils.cc (right):

https://codereview.chromium.org/2681473002/diff/1/content/public/test/text_in...
content/public/test/text_input_test_utils.cc:273:
web_composition_underlines.push_back(blink::WebCompositionUnderline(
On 2017/02/08 02:16:23, alexmos wrote:
> optional: emplace_back would let you avoid explicitly writing
> blink::WebCompositionUnderline().

Sorry I just saw this suggestion. Thanks (And Done).

https://codereview.chromium.org/2681473002/diff/80001/chrome/browser/renderer...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2681473002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:806: for
(size_t i = 0; i < frames.size(); ++i)
On 2017/02/08 16:36:53, sky wrote:
> Do you need an assertion that the RenderFrameHosts are non-null?

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.

https://codereview.chromium.org/2681473002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:809:
auto focus_input_in_frame = [](content::RenderFrameHost* frame) {
On 2017/02/08 16:36:53, sky wrote:
> 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.
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.

https://codereview.chromium.org/2681473002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:825: 
On 2017/02/08 16:36:53, sky wrote:
> Assert the size of sample_text is the same as frames?

Done.

https://codereview.chromium.org/2681473002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:828:
focus_input_in_frame(frames[index]);
On 2017/02/08 16:36:53, sky wrote:
> 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.

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

https://codereview.chromium.org/2681473002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:836: //
Running a NOP js code to make sure the <input> text is updated.
On 2017/02/08 16:36:53, sky wrote:
> How do you know the SendImeCommitTextToWidget is completed by the time the
no-op
> runs?
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.

[commit-bot: I haz the power, 2017-02-09 01:50:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 01:50:02]

Dry run: This issue passed the CQ dry run.

[sky, 2017-02-09 17:18:38]

LGTM

https://codereview.chromium.org/2681473002/diff/140001/chrome/browser/rendere...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2681473002/diff/140001/chrome/browser/rendere...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:291:
selected_text_)
{} (as conditional spans multiple-lines)

[EhsanK, 2017-02-09 22:03:15]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 22:03:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[EhsanK, 2017-02-09 22:03:48]

Thanks for the reviews! I will CQ soon.

https://codereview.chromium.org/2681473002/diff/140001/chrome/browser/rendere...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2681473002/diff/140001/chrome/browser/rendere...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:291:
selected_text_)
On 2017/02/09 17:18:37, sky wrote:
> {} (as conditional spans multiple-lines)

Done.

[commit-bot: I haz the power, 2017-02-09 22:06:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 22:06:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[EhsanK, 2017-02-10 01:22:35]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 01:23:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[EhsanK, 2017-02-10 02:13:19]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2017-02-10 02:13:19]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2017-02-10 02:13:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexmos@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2681473002/#ps180001
(title: "Resolved Merge Conflict")

[commit-bot: I haz the power, 2017-02-10 02:13:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 02:23:30]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1486692799873650,
"parent_rev": "84069f265a506df8b66ab1109aa0a576f086b971", "commit_rev":
"b6483a05119a0ebb57d3968783371c119a83d49c"}

[commit-bot: I haz the power, 2017-02-10 02:24:03]

Description was changed from

==========
Fix a recent regression in IME inside OOPIFs

IME events are only processed at the renderer if there is widget/page
focus. This used to be done by checking the variabel m_imeAcceptEvents
inside WebFrameWidgetImpl which technically tracks the page focus.

After a recent refactoring, the logic for verifying page focus was moved
to RenderWidget which saves the unnecessary call to
WebInputMethodController IME methods when page is not focused. But
unfortunately this broke IME because page focus
(RenderWidget::has_focus_) does not get updated for RenderWidgets
corresponding to OOPIFs. The only reason it was working before was that
m_imeAcceptEvents is initialized to |true| and stay because we never
update page focus for WebFrameWidgetImpl.

This CL will temporarily allow handling all IME events by checking if
the RenderWidget is for an OOPIF. This will revert the behvaior to that
of prior to the refactoring.

This CL also adds an interactive browser test which catches regressions
of this sort on Mac and Aura platforms.

BUG=688842, 602723
==========

to

==========
Fix a recent regression in IME inside OOPIFs

IME events are only processed at the renderer if there is widget/page
focus. This used to be done by checking the variabel m_imeAcceptEvents
inside WebFrameWidgetImpl which technically tracks the page focus.

After a recent refactoring, the logic for verifying page focus was moved
to RenderWidget which saves the unnecessary call to
WebInputMethodController IME methods when page is not focused. But
unfortunately this broke IME because page focus
(RenderWidget::has_focus_) does not get updated for RenderWidgets
corresponding to OOPIFs. The only reason it was working before was that
m_imeAcceptEvents is initialized to |true| and stay because we never
update page focus for WebFrameWidgetImpl.

This CL will temporarily allow handling all IME events by checking if
the RenderWidget is for an OOPIF. This will revert the behvaior to that
of prior to the refactoring.

This CL also adds an interactive browser test which catches regressions
of this sort on Mac and Aura platforms.

BUG=688842, 602723

Review-Url: https://codereview.chromium.org/2681473002
Cr-Commit-Position: refs/heads/master@{#449521}
Committed:
https://chromium.googlesource.com/chromium/src/+/b6483a05119a0ebb57d396878337...
==========

[commit-bot: I haz the power, 2017-02-10 02:24:04]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/b6483a05119a0ebb57d396878337...

[Charlie Reis, 2017-02-14 21:11:04]

RS LGTM given Alex's review of https://codereview.chromium.org/2681473002/.

[Charlie Reis, 2017-02-14 21:11:53]

On 2017/02/14 21:11:04, Charlie Reis wrote:
> RS LGTM given Alex's review of https://codereview.chromium.org/2681473002/.

(Sorry, wrong CL: I was trying to reply to
https://codereview.chromium.org/2696883002)