Fix an IME regression for <webview> due to missing IPC message param

Fix an IME regression for <webview> due to missing IPC message param

The IPC message BrowserPluginHostMsg_ImeFinishComposingText does not have a
parameter for the instance ID of BrowserPlugin and therefore, is dropped on
the browser side inside BrowserPluginMessageFilter. This causes a regression
in (Japanese) IME where clicking during an ongoing composition does not commit.

BUG=720023
Review-Url: https://codereview.chromium.org/2887973002
Cr-Commit-Position: refs/heads/master@{#473976}
Committed: https://chromium.googlesource.com/chromium/src/+/f035886ce3dc15b321f1cd7d0c64948e552e53cc

[EhsanK, 2017-05-16 21:08:32]

ekaramad@chromium.org changed reviewers:
+ nasko@chromium.org, wjmaclean@chromium.org

[EhsanK, 2017-05-16 21:08:33]

Please take a look. Thanks!

[EhsanK, 2017-05-18 15:04:23]

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

[EhsanK, 2017-05-18 15:04:23]

Adding kenrb@ for the IPC review (nasko seems to be OOO?)

Ken, Could you PTAL?

Thanks!

[wjmaclean, 2017-05-18 15:13:05]

https://codereview.chromium.org/2887973002/diff/1/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/2887973002/diff/1/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest.cc:916: bool keep_selection)
{
Where is the new parameter getting used?

[EhsanK, 2017-05-18 15:21:38]

Thanks. PTAL.

https://codereview.chromium.org/2887973002/diff/1/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/2887973002/diff/1/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest.cc:916: bool keep_selection)
{
On 2017/05/18 15:13:05, wjmaclean wrote:
> Where is the new parameter getting used?

Before getting here I suppose:
https://cs.chromium.org/chromium/src/content/browser/browser_plugin/browser_p...

This IPC comes from the embedder and we need the instance ID to properly route
it.

[wjmaclean, 2017-05-18 15:37:17]

https://codereview.chromium.org/2887973002/diff/1/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/2887973002/diff/1/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest.cc:916: bool keep_selection)
{
On 2017/05/18 15:21:38, EhsanK wrote:
> On 2017/05/18 15:13:05, wjmaclean wrote:
> > Where is the new parameter getting used?
> 
> Before getting here I suppose:
>
https://cs.chromium.org/chromium/src/content/browser/browser_plugin/browser_p...
> 
> This IPC comes from the embedder and we need the instance ID to properly route
> it.

I don't understand. You added a parameter to a function, and no code seems to
use the parameter.

[EhsanK, 2017-05-18 17:44:30]

Thanks. PTAL.

https://codereview.chromium.org/2887973002/diff/1/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/2887973002/diff/1/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest.cc:916: bool keep_selection)
{
On 2017/05/18 15:37:17, wjmaclean wrote:
> On 2017/05/18 15:21:38, EhsanK wrote:
> > On 2017/05/18 15:13:05, wjmaclean wrote:
> > > Where is the new parameter getting used?
> > 
> > Before getting here I suppose:
> >
>
https://cs.chromium.org/chromium/src/content/browser/browser_plugin/browser_p...
> > 
> > This IPC comes from the embedder and we need the instance ID to properly
route
> > it.
> 
> I don't understand. You added a parameter to a function, and no code seems to
> use the parameter.
As we spoke offline, I added a DCHECK as well as some comments in the header
file for the IPC. Just to answer the question in the CL: this is a by-product of
using IPC macros for defining handlers and |browser_plugin_instance_id| being
the first param in the IPC. So we have to do it this way unfortunately. Also,
the actual value is used in the BrowserPluginMessageFilter to find this
BrowserPluginGuest.

[wjmaclean, 2017-05-18 17:46:03]

On 2017/05/18 17:44:30, EhsanK wrote:
> Thanks. PTAL.
> 
>
https://codereview.chromium.org/2887973002/diff/1/content/browser/browser_plu...
> File content/browser/browser_plugin/browser_plugin_guest.cc (right):
> 
>
https://codereview.chromium.org/2887973002/diff/1/content/browser/browser_plu...
> content/browser/browser_plugin/browser_plugin_guest.cc:916: bool
keep_selection)
> {
> On 2017/05/18 15:37:17, wjmaclean wrote:
> > On 2017/05/18 15:21:38, EhsanK wrote:
> > > On 2017/05/18 15:13:05, wjmaclean wrote:
> > > > Where is the new parameter getting used?
> > > 
> > > Before getting here I suppose:
> > >
> >
>
https://cs.chromium.org/chromium/src/content/browser/browser_plugin/browser_p...
> > > 
> > > This IPC comes from the embedder and we need the instance ID to properly
> route
> > > it.
> > 
> > I don't understand. You added a parameter to a function, and no code seems
to
> > use the parameter.
> As we spoke offline, I added a DCHECK as well as some comments in the header
> file for the IPC. Just to answer the question in the CL: this is a by-product
of
> using IPC macros for defining handlers and |browser_plugin_instance_id| being
> the first param in the IPC. So we have to do it this way unfortunately. Also,
> the actual value is used in the BrowserPluginMessageFilter to find this
> BrowserPluginGuest.

lgtm, thanks!

[nasko, 2017-05-19 19:44:51]

LGTM with a nit.

https://codereview.chromium.org/2887973002/diff/20001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/2887973002/diff/20001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:915: void
BrowserPluginGuest::OnImeFinishComposingText(int instance_id,
nit: I would've used browser_plugin_instance_id, to be consistent with the rest
of the code.

[EhsanK, 2017-05-22 14:28:01]

Thanks! CQ-ing soon.

https://codereview.chromium.org/2887973002/diff/20001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/2887973002/diff/20001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:915: void
BrowserPluginGuest::OnImeFinishComposingText(int instance_id,
On 2017/05/19 19:44:51, nasko wrote:
> nit: I would've used browser_plugin_instance_id, to be consistent with the
rest
> of the code.

Thanks! Done.

[EhsanK, 2017-05-22 14:28:08]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2017-05-22 14:28:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
wjmaclean@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2887973002/#ps40001
(title: "Addressed nasko@'s comment")

[commit-bot: I haz the power, 2017-05-22 14:28:25]

CQ is trying da patch.

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

[EhsanK, 2017-05-22 14:28:26]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2017-05-22 14:32:28]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2017-05-22 14:32:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
wjmaclean@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2887973002/#ps60001
(title: "Rebased")

[commit-bot: I haz the power, 2017-05-22 14:32:45]

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-05-22 15:58:15]

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

[commit-bot: I haz the power, 2017-05-22 15:58:17]

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

[EhsanK, 2017-05-23 17:09:01]

The CQ bit was checked by ekaramad@chromium.org

[commit-bot: I haz the power, 2017-05-23 17:09:33]

CQ is trying da patch.

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

[EhsanK, 2017-05-23 17:33:28]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2017-05-23 17:33:28]

The CQ bit was checked by ekaramad@chromium.org

[commit-bot: I haz the power, 2017-05-23 17:33: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, 2017-05-23 18:10:16]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495560808678200,
"parent_rev": "29ef12086e6ac7eb83677e7730df00fda5c6af9b", "commit_rev":
"f035886ce3dc15b321f1cd7d0c64948e552e53cc"}

[commit-bot: I haz the power, 2017-05-23 18:10:29]

Description was changed from

==========
Fix an IME regression for <webview> due to missing IPC message param

The IPC message BrowserPluginHostMsg_ImeFinishComposingText does not have a
parameter for the instance ID of BrowserPlugin and therefore, is dropped on
the browser side inside BrowserPluginMessageFilter. This causes a regression
in (Japanese) IME where clicking during an ongoing composition does not commit.

BUG=720023
==========

to

==========
Fix an IME regression for <webview> due to missing IPC message param

The IPC message BrowserPluginHostMsg_ImeFinishComposingText does not have a
parameter for the instance ID of BrowserPlugin and therefore, is dropped on
the browser side inside BrowserPluginMessageFilter. This causes a regression
in (Japanese) IME where clicking during an ongoing composition does not commit.

BUG=720023

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

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

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/f035886ce3dc15b321f1cd7d0c64...