Route IME-related IPCs to the active/focused RenderWidget (Mac)

Route IME-related IPCs to the active/focused RenderWidget (Mac)

This CL will route IPCs resulted from IME-related methods in RenderWidgetHostCocoaView to the correct (focused) RenderWidget. The CL also adds unit tests for each method modified.

Modified methods:
*  confirmComposition (InputMsg_ImeConfirmComposition)
*  insertText (InputMsg_ImeConfirmComposition)
*  setMarkedText (InputMsg_ImeSetComposition)
*  unmarkText (InputMsg_ImeConfirmComposition)

BUG=578168, 602723
Committed: https://crrev.com/fd5f5a89a6a3cc76fade71f9c32e4be1801d95ea
Cr-Commit-Position: refs/heads/master@{#411552}

[EhsanK, 2016-08-10 01:08:46]

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

[commit-bot: I haz the power, 2016-08-10 01:09:31]

Dry run: 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-08-10 02:32:56]

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

[commit-bot: I haz the power, 2016-08-10 02:32:56]

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

[EhsanK, 2016-08-10 16:43:55]

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

[commit-bot: I haz the power, 2016-08-10 16:44:46]

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

[EhsanK, 2016-08-10 16:50:10]

Patchset #3 (id:40001) has been deleted

[EhsanK, 2016-08-10 20:06:12]

ekaramad@chromium.org changed reviewers:
+ avi@chromium.org, creis@chromium.org, kenrb@chromium.org, nasko@chromium.org

[EhsanK, 2016-08-10 20:06:13]

PTAL.

[Avi (use Gerrit), 2016-08-10 21:01:41]

LGTM but it's been a while since I've dealt with the Mac UI. Can you run this by
one of the Mac team?

https://codereview.chromium.org/2229223002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2229223002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:19: #include
"base/strings/string16.h"
Why do you need this include?

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

https://codereview.chromium.org/2229223002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:583: return
!!GetTextInputManager() ? GetTextInputManager()->GetActiveWidget()
You shouldn't need the !!.

[EhsanK, 2016-08-10 21:27:27]

ekaramad@chromium.org changed reviewers:
+ asvitkine@google.com

[EhsanK, 2016-08-10 21:27:29]

Thank you avi@ for the reviews. PTAL.

Adding asvitkine@ for Mac UI.

asvitkine@: Could you please review this change, or suggest someone else who
could? Thanks!

https://codereview.chromium.org/2229223002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2229223002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:19: #include
"base/strings/string16.h"
On 2016/08/10 21:01:40, Avi wrote:
> Why do you need this include?

We are using it in multiple places in this file. I assumed IWYU holds here, but
was not quite sure. This is btw unrelated to my change I just happened to notice
it. Let me know if I should revert this.

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

https://codereview.chromium.org/2229223002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:583: return
!!GetTextInputManager() ? GetTextInputManager()->GetActiveWidget()
On 2016/08/10 21:01:40, Avi wrote:
> You shouldn't need the !!.

Acknowledged.

[Alexei Svitkine (slow), 2016-08-11 06:14:01]

asvitkine@chromium.org changed reviewers:
+ erikchen@chromium.org
- asvitkine@google.com

[Alexei Svitkine (slow), 2016-08-11 06:14:02]

Routing to erikchen@ for Mac UI as I'm just as out of touch (or possibly more)
as Avi.

[erikchen, 2016-08-11 16:25:30]

lgtm

This seems ok to me, though I'm not an expert for IME on Mac (I don't know if
anyone is).

https://codereview.chromium.org/2229223002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
(right):

https://codereview.chromium.org/2229223002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1268:
child_process_host_ = new MockRenderProcessHost(&browser_context_);
Do we need unique_ptrs to prevent leaks?

https://codereview.chromium.org/2229223002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1339:
NSRange selectedRange = NSMakeRange(0, 4);
wrap in scoped_nsobject. Ditto for other ObjC allocations.

[Avi (use Gerrit), 2016-08-11 17:25:31]

m

https://codereview.chromium.org/2229223002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
(right):

https://codereview.chromium.org/2229223002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1339:
NSRange selectedRange = NSMakeRange(0, 4);
On 2016/08/11 16:25:30, erikchen wrote:
> wrap in scoped_nsobject. Ditto for other ObjC allocations.

This is re line 1338, the NSString, not the NSRange.

[EhsanK, 2016-08-11 20:42:58]

Thank you for the reviews. PTAL.

https://codereview.chromium.org/2229223002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
(right):

https://codereview.chromium.org/2229223002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1268:
child_process_host_ = new MockRenderProcessHost(&browser_context_);
On 2016/08/11 16:25:30, erikchen wrote:
> Do we need unique_ptrs to prevent leaks?

The |child_widget_| is being destroyed in TearDown(). That will automatically
destroy the |child_view_|. Also, on its destruction path, it will call
RemoveRoute() on the |child_process_host_| which will eventually clean up and
delete the MockRenderProcessHost as I see here:
https://cs.chromium.org/chromium/src/content/public/test/mock_render_process_...

https://cs.chromium.org/chromium/src/content/public/test/mock_render_process_...

So I think we should not leak memory like this. Is there any other concerns I am
missing here?

https://codereview.chromium.org/2229223002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1339:
NSRange selectedRange = NSMakeRange(0, 4);
On 2016/08/11 16:25:30, erikchen wrote:
> wrap in scoped_nsobject. Ditto for other ObjC allocations.

Acknowledged.

https://codereview.chromium.org/2229223002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1339:
NSRange selectedRange = NSMakeRange(0, 4);
On 2016/08/11 17:25:31, Avi wrote:
> On 2016/08/11 16:25:30, erikchen wrote:
> > wrap in scoped_nsobject. Ditto for other ObjC allocations.
> 
> This is re line 1338, the NSString, not the NSRange.

Acknowledged.

[erikchen, 2016-08-11 20:52:39]

https://codereview.chromium.org/2229223002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
(right):

https://codereview.chromium.org/2229223002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1268:
child_process_host_ = new MockRenderProcessHost(&browser_context_);
On 2016/08/11 20:42:58, EhsanK wrote:
> On 2016/08/11 16:25:30, erikchen wrote:
> > Do we need unique_ptrs to prevent leaks?
> 
> The |child_widget_| is being destroyed in TearDown(). That will automatically
> destroy the |child_view_|. Also, on its destruction path, it will call
> RemoveRoute() on the |child_process_host_| which will eventually clean up and
> delete the MockRenderProcessHost as I see here:
>
https://cs.chromium.org/chromium/src/content/public/test/mock_render_process_...
> 
>
https://cs.chromium.org/chromium/src/content/public/test/mock_render_process_...
> 
> So I think we should not leak memory like this. Is there any other concerns I
am
> missing here?

Point taken.

That's a really opaque cleanup path [and fraught with potential problems], but
that's not related to your CL. Still lgtm.

[EhsanK, 2016-08-11 21:49:24]

On 2016/08/11 20:52:39, erikchen wrote:
>
https://codereview.chromium.org/2229223002/diff/100001/content/browser/render...
> File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
> (right):
> 
>
https://codereview.chromium.org/2229223002/diff/100001/content/browser/render...
> content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1268:
> child_process_host_ = new MockRenderProcessHost(&browser_context_);
> On 2016/08/11 20:42:58, EhsanK wrote:
> > On 2016/08/11 16:25:30, erikchen wrote:
> > > Do we need unique_ptrs to prevent leaks?
> > 
> > The |child_widget_| is being destroyed in TearDown(). That will
automatically
> > destroy the |child_view_|. Also, on its destruction path, it will call
> > RemoveRoute() on the |child_process_host_| which will eventually clean up
and
> > delete the MockRenderProcessHost as I see here:
> >
>
https://cs.chromium.org/chromium/src/content/public/test/mock_render_process_...
> > 
> >
>
https://cs.chromium.org/chromium/src/content/public/test/mock_render_process_...
> > 
> > So I think we should not leak memory like this. Is there any other concerns
I
> am
> > missing here?
> 
> Point taken.
> 
> That's a really opaque cleanup path [and fraught with potential problems], but
> that's not related to your CL. Still lgtm.

I think if I delete either of those things deliberately it will end up in a
crash at exit time. I was first trying to write tests with stack variables which
did not work of course.

[EhsanK, 2016-08-11 21:50:08]

Thank you all for the reviews! FYI, I will CQ after successful dry-run.

[EhsanK, 2016-08-11 21:51:17]

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

[commit-bot: I haz the power, 2016-08-11 21:51:44]

Dry run: 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-08-11 23:17:13]

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

[commit-bot: I haz the power, 2016-08-11 23:17:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[EhsanK, 2016-08-12 03:15:39]

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

[commit-bot: I haz the power, 2016-08-12 03:16:11]

Dry run: 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-08-12 04:15:18]

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

[commit-bot: I haz the power, 2016-08-12 04:15:19]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2016-08-12 04:29:38]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2016-08-12 04:29:39]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2229223002/#ps140001
(title: "Addressed erickchen@ comments + Making RWHVMacTest ->
InputMethodMacTest.")

[commit-bot: I haz the power, 2016-08-12 04:29:51]

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-08-12 04:33:27]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-08-12 04:35:39]

Description was changed from

==========
Route IME-related IPCs to the active/focused RenderWidget (Mac)

This CL will route IPCs resulted from IME-related methods in
RenderWidgetHostCocoaView to the correct (focused) RenderWidget. The CL also
adds unit tests for each method modified.

Modified methods:
*  confirmComposition (InputMsg_ImeConfirmComposition)
*  insertText (InputMsg_ImeConfirmComposition)
*  setMarkedText (InputMsg_ImeSetComposition)
*  unmarkText (InputMsg_ImeConfirmComposition)

BUG=578168, 602723
==========

to

==========
Route IME-related IPCs to the active/focused RenderWidget (Mac)

This CL will route IPCs resulted from IME-related methods in
RenderWidgetHostCocoaView to the correct (focused) RenderWidget. The CL also
adds unit tests for each method modified.

Modified methods:
*  confirmComposition (InputMsg_ImeConfirmComposition)
*  insertText (InputMsg_ImeConfirmComposition)
*  setMarkedText (InputMsg_ImeSetComposition)
*  unmarkText (InputMsg_ImeConfirmComposition)

BUG=578168, 602723

Committed: https://crrev.com/fd5f5a89a6a3cc76fade71f9c32e4be1801d95ea
Cr-Commit-Position: refs/heads/master@{#411552}
==========

[commit-bot: I haz the power, 2016-08-12 04:35:41]

Patchset 7 (id:??) landed as
https://crrev.com/fd5f5a89a6a3cc76fade71f9c32e4be1801d95ea
Cr-Commit-Position: refs/heads/master@{#411552}