Send IME events to windowless plug-ins (Chromium side)

Send IME events to windowless plug-ins (Chromium side)

This change adds a new class WebPluginIMEWin that converts the platform-independent IME data sent from a renderer process (or WebKit) to the Win32 IME messages and send them to a plug-in. To allow the plug-in to retrieve the IME data with IMM32 function calls, this change also adds a patch to GetProcessAddress(). (Flash seems to retrieve the pointers to IMM32 function with this function.) This change also sends IME status retrieved from the plug-in to a browser process (via a renderer process).

BUG=82507
TEST=manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=103869

[cpu_(ooo_6.6-7.5), 2011-06-10 18:35:32]

You probably want to remove viettrungluu and brettw from the reviewers and add
to this review jschuh and ananta

[cpu_(ooo_6.6-7.5), 2011-06-10 18:38:06]

http://codereview.chromium.org/7082034/diff/14001/webkit/plugins/npapi/webplu...
File webkit/plugins/npapi/webplugin_ime_win.cc (right):

http://codereview.chromium.org/7082034/diff/14001/webkit/plugins/npapi/webplu...
webkit/plugins/npapi/webplugin_ime_win.cc:64: 
I worry that Justin's new UIPI sandbox hardening has broken this.

[jschuh, 2011-06-11 17:31:50]

http://codereview.chromium.org/7082034/diff/14001/webkit/plugins/npapi/webplu...
File webkit/plugins/npapi/webplugin_ime_win.cc (right):

http://codereview.chromium.org/7082034/diff/14001/webkit/plugins/npapi/webplu...
webkit/plugins/npapi/webplugin_ime_win.cc:64: 
On 2011/06/10 18:38:06, cpu wrote:
> I worry that Justin's new UIPI sandbox hardening has broken this.

Yeah, this definitely needs to be tested against the current trunk. With UIPI
now enabled, I'm not sure these IME messages can get through.

[Hironori Bono, 2011-06-13 09:51:09]

Greetings Carlos and Justin,

Thank you for your review and comments. Even though I'm not totally sure about
the UIPC code of Chrome, does it prevent us from sending IME messages to
windowless Flash with NPP_HandleEvent()? (This change sends IME events always
with NPP_HandleEvent().)

Regards,

Hironori Bono

http://codereview.chromium.org/7082034/diff/14001/webkit/plugins/npapi/webplu...
File webkit/plugins/npapi/webplugin_ime_win.cc (right):

http://codereview.chromium.org/7082034/diff/14001/webkit/plugins/npapi/webplu...
webkit/plugins/npapi/webplugin_ime_win.cc:64: 
Thank you for your comments. This change sends these IME messages to windowless
Flash with NPP_HandleEvent(), i.e. this change does not send these messages with
SendMesssage() or PostMessage(). Even though I have not built Chrome to test
this change, I wonder if UIPI prevents sending these messages via
NPP_HandleEvent().

[brettw, 2011-06-13 14:27:10]

http://codereview.chromium.org/7082034/diff/14001/content/renderer/webplugin_...
File content/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/7082034/diff/14001/content/renderer/webplugin_...
content/renderer/webplugin_delegate_proxy.cc:1063:
render_view_->UpdatePluginIMEStatus(input_type, caret_rect);
Unless you have future plans for the code in RenderView, I would just send your
IME message directly from here rather than adding more stuff to RenderView.

[Hironori Bono, 2011-06-16 07:34:11]

Greeting Brett,

Thank you for your review and comment. I have uploaded the updated change that
applied your comment. (Unfortunately, this change depends on
<http://webkit.org/b/61272> and cannot land it without it.)

Regards,

Hironori Bono

http://codereview.chromium.org/7082034/diff/14001/content/renderer/webplugin_...
File content/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/7082034/diff/14001/content/renderer/webplugin_...
content/renderer/webplugin_delegate_proxy.cc:1063:
render_view_->UpdatePluginIMEStatus(input_type, caret_rect);
On 2011/06/13 14:27:10, brettw wrote:
> Unless you have future plans for the code in RenderView, I would just send
your
> IME message directly from here rather than adding more stuff to RenderView.

Done. Thank you for noticing it.

[brettw, 2011-06-16 17:37:06]

Content rubber stamp LGTM (didn't take time to understand the details).

[cpu_(ooo_6.6-7.5), 2011-06-18 00:31:37]

Justin's change makes it so there is always a window (dummy window for
activation) even when flash is windowless. Could that be used to simplify your
code?

I believe it is popup style before it is re-parented by the browser.

Otherwise I see nothing wrong with your code. I hope Ananta has had a chance to
look at it.

[Hironori Bono, 2011-06-22 04:13:18]

Greetings Carlos,

In brief, it does not help simplify my change so much without changing Flash
code. (Flash calls ImmGetContext() with an invalid window handle, i.e.
IsWindow() returns FALSE, for this case.)
By the way, if I understand his change correctly, "using an IME attached to the
dummy window" is equal to "using an IME attached to a plug-in process", right?
To see IME-related regressions caused by sandboxed Flash, I'm a little wondering
if it is a good idea to keep attaching an IME to a plug-in process. (In my
personal opinion, I would like to reduce dependency of a plugin process onto
IMEs.) It may be an overkill, though.

Regards,

Hironori Bono

[cpu_(ooo_6.6-7.5), 2011-06-23 19:54:33]

On 2011/06/22 04:13:18, hbono wrote:
> Greetings Carlos,
> 
> In brief, it does not help simplify my change so much without changing Flash
> code. (Flash calls ImmGetContext() with an invalid window handle, i.e.
> IsWindow() returns FALSE, for this case.)

Ok. 

> By the way, if I understand his change correctly, "using an IME attached to
the
> dummy window" is equal to "using an IME attached to a plug-in process", right?

Yes.

> To see IME-related regressions caused by sandboxed Flash, I'm a little
wondering
> if it is a good idea to keep attaching an IME to a plug-in process. (In my
> personal opinion, I would like to reduce dependency of a plugin process onto
> IMEs.) It may be an overkill, though.

I would also like to reduce the dependency on IME as well. For the time being
lets land your change when you are ready.  So LGTM.



> 
> Regards,
> 
> Hironori Bono