Request composition info from the focused RenderWidget

Request composition info from the focused RenderWidget

Currently on Mac, we do not setup monitoring composition range which leads to
misplaced composition window for Japanese IME (When moving back and forth between
composition clauses).

This CL makes the changes to route the monitoring IPC to the currently active RWH.

BUG=651604
Committed: https://crrev.com/71d0ae311054949697cf26b354b6b9a4efbbbaa4
Cr-Commit-Position: refs/heads/master@{#424443}

[EhsanK, 2016-09-29 22:22:26]

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

[commit-bot: I haz the power, 2016-09-29 22:23:03]

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

[EhsanK, 2016-09-29 22:23:26]

ekaramad@chromium.org changed reviewers:
+ creis@chromium.org, erikchen@chromium.org, nona@chromium.org

[EhsanK, 2016-09-29 22:23:27]

PTAL

[Charlie Reis, 2016-09-30 00:03:30]

I'll try to take a look at this and your other Mac fix tomorrow, but is there a
way to add a test for either one?   (In general, please aim to include tests
when possible, or maybe say a bit about why it isn't practical when that
happens.)

[commit-bot: I haz the power, 2016-09-30 00:14:12]

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

[commit-bot: I haz the power, 2016-09-30 00:14:13]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2016-09-30 03:13:24]

On 2016/09/30 00:03:30, Charlie Reis wrote:
> I'll try to take a look at this and your other Mac fix tomorrow, but is there
a
> way to add a test for either one?   (In general, please aim to include tests
> when possible, or maybe say a bit about why it isn't practical when that
> happens.)

Sorry for missing the test in the first patch. I was not quite sure how to do it
then so I decided to send it empty and think meanwhile. All I could do was a
unit test to verify that we do send out the right IPCs.

All that being said, I think we have proper testing for the composition range
tracking but it only makes sure that we track info from child frames (not this
specific case which was introduced in nona@'s change after the IME landed).

I don't think I can add a test for the other CL though, but lets discuss it over
there.

[Charlie Reis, 2016-09-30 22:39:16]

Thanks for adding the test here.  This looks fine (with nits) from an owner's
perspective, but I'll defer to erikchen@ or nona@ about the mechanics of the
change.

https://codereview.chromium.org/2373243005/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2373243005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:896: // state and
|updated_view|.
How is this comment related to the null check below?  Is it just a separate
observation?

It also seems like an indirect way to explain why we need to use updated_view's
RWH instead of render_widget_host_.  Is that what the first sentence is saying? 
I'm wondering if there's a way to make this clearer.

https://codereview.chromium.org/2373243005/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
(right):

https://codereview.chromium.org/2373243005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1492: //
This test verifies than when a RenderWidgetHostView changes its
nit: s/than/that/

[EhsanK, 2016-10-05 16:39:02]

Thank you Charlie.

I rebased + changed in Patch 3 which was bad. So now Patch 4 is rebase and Patch
5 is the change. Consider patch 3 as a deleted patch (I wouldn't dare to
actually delete it again :)
https://codereview.chromium.org/2278283002/#msg42).

https://codereview.chromium.org/2373243005/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2373243005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:896: // state and
|updated_view|.
On 2016/09/30 22:39:16, Charlie Reis wrote:
> How is this comment related to the null check below?  Is it just a separate
> observation?
A separate observation. I left a new comment for the null check though. Please
see if it is useful.

> It also seems like an indirect way to explain why we need to use
updated_view's
> RWH instead of render_widget_host_.  Is that what the first sentence is
saying? 
> I'm wondering if there's a way to make this clearer.
Sort of...yes. I modified the explanation a bit. But FWIW, I will explain a few
issues here:

1- We would like to make sure only the RWH with active TextInputState monitor
composition info. For that, when RWH is active we send active IPC and when the
state is NONE we send stop IPC.

2- If TextInputManager calls this method when there is active TextInputState,
the very next call is always for a TextInputState of NONE, and vice versa.

3- When state is Text, we have a TextInputManager::GetActiveWidget() return the
active widget which is the one corresponding to |updated_view|. But when state
becomes NONE, GetActiveWidget() returns nullptr. But the widget which lost its
state (if it still exists) is the one corresponding to |updated_view|. So we
could as well only use |updated_view| to start/stop monitoring.

4- This behavior, to activate and deactivate monitoring was also implemented for
Aura. But we do not use |updated_view| there and rather, try to remember the
last widget which had active TextInputState (due to the fact that when we lose
state, GetActiveWidget() is nullptr so we have no clue which widget should stop
monitoring). So we stored the routing and process IDs to be safe. The reason we
did not use |updated_view| was that
RenderWidgetHostViewBase::GetRenderWidgetHost() is pure virtual. In the
destruction path of RWHVs, and inside RWHVBase dtor we make a call to
TextInputManager:Unregister(this) to free up memory which comes back here is the
|updated_view| had active TextInputState before. That of course was a crash back
then.

So in the comments here I wanted to emphasize that since TextInputManager
behaves as in item 2, item 1 is do able by just sending IPCs to the
|updated_view|.

Sorry for the long response. I hope it helps.

https://codereview.chromium.org/2373243005/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
(right):

https://codereview.chromium.org/2373243005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1492: //
This test verifies than when a RenderWidgetHostView changes its
On 2016/09/30 22:39:16, Charlie Reis wrote:
> nit: s/than/that/

Done.

[Seigo Nonaka, 2016-10-06 23:49:02]

lgtm, thank you for fixing the issue.

This is a regression in M54 right?
Maybe too late for M54 but we may want to merge this to M55 branch if this CL
misses the branch point.

[Charlie Reis, 2016-10-07 05:23:57]

Thanks, LGTM.

[EhsanK, 2016-10-11 15:18:06]

Thanks for the reviews! FYI, I will CQ soon.

creis@: As nona@ is suggesting this might need merging. Do we need to given that
this happens with --isolate-extensions? And is a regression of IME in <iframe>s?
Does it need to be merged into M-54 then?

[EhsanK, 2016-10-11 15:18:12]

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

[commit-bot: I haz the power, 2016-10-11 15:18:32]

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

[EhsanK, 2016-10-11 15:26:28]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2016-10-11 15:26:28]

The CQ bit was checked by ekaramad@chromium.org

[commit-bot: I haz the power, 2016-10-11 15:26:42]

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, 2016-10-11 16:04:20]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-11 16:05:51]

Description was changed from

==========
Request composition info from the focused RenderWidget

Currently on Mac, we do not setup monitoring composition range which leads to
misplaced composition window for Japanese IME (When moving back and forth
between
composition clauses).

This CL makes the changes to route the monitoring IPC to the currently active
RWH.

BUG=651604
==========

to

==========
Request composition info from the focused RenderWidget

Currently on Mac, we do not setup monitoring composition range which leads to
misplaced composition window for Japanese IME (When moving back and forth
between
composition clauses).

This CL makes the changes to route the monitoring IPC to the currently active
RWH.

BUG=651604

Committed: https://crrev.com/71d0ae311054949697cf26b354b6b9a4efbbbaa4
Cr-Commit-Position: refs/heads/master@{#424443}
==========

[commit-bot: I haz the power, 2016-10-11 16:05:52]

Patchset 5 (id:??) landed as
https://crrev.com/71d0ae311054949697cf26b354b6b9a4efbbbaa4
Cr-Commit-Position: refs/heads/master@{#424443}

[Charlie Reis, 2016-10-11 16:06:16]

On 2016/10/11 15:18:06, EhsanK wrote:
> Thanks for the reviews! FYI, I will CQ soon.
> 
> creis@: As nona@ is suggesting this might need merging. Do we need to given
that
> this happens with --isolate-extensions? And is a regression of IME in
<iframe>s?
> Does it need to be merged into M-54 then?

It's probably too late to merge to M54, for anything but a security fix or crash
regression.  Is the IME dialog still usable and just in the wrong place?  Or is
it unusable?

If it's a serious enough regression, we can ask the TPM if they want to accept
the merge, but the fix will need to bake on Canary/Dev for a while before
requesting it.  (Note: this is better discussed on the bug than on the CL.)