Bug 61272 – [Chromium] On-the-spot IME support for windowless plug-ins

• Component: Plug-ins
• Status: RESOLVED FIXED

[Hironori Bono]

(Copied from <http://crbug.com/82507>.)

What steps will reproduce the problem?

1. Open <http://baseonmars.co.uk/bugs/wmode>.
2. Click the text area under wmode="opaque".
3. Enable an IME. (This issue happens on any IMEs.)
4. Type text.

What is the expected result?

We can see IME text in the textarea.

What happens instead?

We cannot see any text.

Please provide any additional information below. Attach a screenshot if
possible.

Technically, this issue consists of two issues:
1. Chrome should send WM_IME_* events as written in <http://crbug.com/13233#c72>.
2. Chrome cancels composition when the focused node is a windowless plug-in.

Regards,

[Hironori Bono]

Created attachment 94403 [details]
Patch v0

Greetings,

I would like to upload the initial change that dispatches IME data to a plug-in process. (This change also prevents cancelling an ongoing composition when we use an IME on a windowless plug-in.
This is just for your information since the attached layout test needs our Chromium-side change. This test fails without it because a plug-in process cannot send the IME data to plug-ins.

Regards,

Hironori Bono

[Hironori Bono]

Created attachment 95420 [details]
Patch v1

Greetings,

I have uploaded the WebKit-side change that fixed this issue on Chromium. (Its Chromium-side change is <http://codereview.chromium.org/7082034/>.) Unfortunately, I do not have any good ideas about how Win Safari controls NPAPI plug-ins and I cannot implement it for Win Safari. Sorry. 

Regards,

Hironori Bono

[Alexey Proskuryakov]

Adding composition events to WebEvent seems unfortunate, as there are two completely different code paths for handling these now. Anders, Jeff or Sam are probably the best people to discuss this with.

[Hironori Bono]

Greetings Alexey,

Thank you for your comment.

(In reply to comment #3)
> Adding composition events to WebEvent seems unfortunate, as there are two completely different code paths for handling these now. Anders, Jeff or Sam are probably the best people to discuss this with.

You are totally right. This is a Chromium-specific solution and we need another code path for Win Safari. I have just started reading code under "Source/WebCore/plugins" to figure out how I can merge this change to Win Safari. By the way, I made this change Chromium-specific one also because this feature is not so tested well and it surely takes some more time to stabilize it. I'm planning to merge this change to Win Safari while we stabilize this feature with Chromium.

Regards,

Hironori Bono

[Kent Tamura]

The patch has changes for WebKit/chromium/public/.  So fishd wants to review it.

[Kent Tamura]

Comment on attachment 95420 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=95420&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:1394
> +        String pluginText = text;
> +        pluginText.append('\n');
> +        pluginText += String::number(selectionStart);
> +        pluginText.append('\n');
> +        pluginText += String::number(selectionEnd);
> +        pluginText.append('\n');
> +        for (size_t i = 0; i < underlines.size(); ++i) {
> +            pluginText += String::number(underlines[i].startOffset);
> +            pluginText.append('\n');
> +            pluginText += String::number(underlines[i].endOffset);
> +            pluginText.append('\n');
> +            pluginText += String::number(underlines[i].color);
> +            pluginText.append('\n');
> +            pluginText.append(underlines[i].thick ? '1' : '0');
> +            pluginText.append('\n');
> +        }

We had better use wtf/text/StringBuilder.

> LayoutTests/platform/chromium/test_expectations.txt:4036
> +
> +// We need a Chromium-side change to fix this test.
> +BUGCR82507 : platform/chromium-win/plugins/windowless-plugin-ime-event-test.html = TEXT

I recommend adding lines not at the bottom of test_expectations.txt in order to avoid patch conflicts.

[Hironori Bono]

Created attachment 95899 [details]
Patch v2

Greetings Kent-san,

Thank you for your comments.

(In reply to comment #6)
> (From update of attachment 95420 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95420&action=review
  ...
> We had better use wtf/text/StringBuilder.

If I recall correctly, it is not so simple to append text to a StringBuffer object as this code does. (I could be wrong, though.) Since this code is used only when we type text with an IME, I'm a little wondering if we use StringBuffer here and make this code more complicated. Would it be possible to give me a good idea that can use StringBuffer without losing simplicity?

> > LayoutTests/platform/chromium/test_expectations.txt:4036
> > +
> > +// We need a Chromium-side change to fix this test.
> > +BUGCR82507 : platform/chromium-win/plugins/windowless-plugin-ime-event-test.html = TEXT
> 
> I recommend adding lines not at the bottom of test_expectations.txt in order to avoid patch conflicts.

Thank you for noticing it. I have moved it to the 'Plugin tests' section.

Regards,

Hironori Bono

[WebKit Review Bot]

Attachment 95899 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:55:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kent Tamura]

(In reply to comment #7)
> > We had better use wtf/text/StringBuilder.
> 
> If I recall correctly, it is not so simple to append text to a StringBuffer object as this code does. (I could be wrong, though.) Since this code is used only when we type text with an IME, I'm a little wondering if we use StringBuffer here and make this code more complicated. Would it be possible to give me a good idea that can use StringBuffer without losing simplicity?

I'm not sure what you are worrying about.  I think this code can be written with StringBuilder in similar amount of code.
String::append() and operator+= replace an internal StringImpl.  We had better avoid use them.

Also, we have makeString() in wtf/text/StringConcatenate.h.  It might make the code simpler.

[Hironori Bono]

(In reply to comment #9)
> I'm not sure what you are worrying about.  I think this code can be written with StringBuilder in similar amount of code.
> String::append() and operator+= replace an internal StringImpl.  We had better avoid use them.
> 
> Also, we have makeString() in wtf/text/StringConcatenate.h.  It might make the code simpler.

Sorry, I misread your StringBuilder as StringBuffer. Using StringBuilder makes sense.

Regards,

Hironori Bono

[Darin Fisher (:fishd, Google)]

Comment on attachment 95899 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=95899&action=review

> Source/WebKit/chromium/public/WebInputEvent.h:111
> +        RawCompositionSet,

do these become WebInputEvents that would never be passed to WebWidget::handleInputEvent?
i'm concerned about defining plugin-specific input events.  can you tell me more about
why these are input events for plugins and not input events for WebWidgets?  why wouldn't
we just add methods for setting and confirming composition on WebPlugin?

[Hironori Bono]

Created attachment 96943 [details]
Patch v3

Greetings Tony and Tamura-san,

Thank you for your reviews and comments.

> Sorry, I misread your StringBuilder as StringBuffer. Using StringBuilder makes sense.

I have updated my change to use StringBuilder.

(In reply to comment #11)
> do these become WebInputEvents that would never be passed to WebWidget::handleInputEvent?

Right. This is only used for plug-ins.

> i'm concerned about defining plugin-specific input events.  can you tell me more about
> why these are input events for plugins and not input events for WebWidgets?  why wouldn't
> we just add methods for setting and confirming composition on WebPlugin?

Thank you for your comment. I have also considered this option before I started implementing this change. In brief, I have added an input event just because an IME event was mostly coupled with a keyboard events and it looked safer to use the same framework that sends keyboard events to send IME events. I'm not sure which is the better option in terms of sharing code between Chromium and Safari, adding an input event or adding a method to WebPlugin. Unfortunately, my Safari change for this issue is still in progress. (Even though this change is currently a Chromium-specific change, I would like to share code with Safari Win as much as possible.)

Regards,

Hironori Bono

[WebKit Review Bot]

Attachment 96943 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:55:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hironori Bono]

Greetings,

This change has not been reviewed for more than a week. I'm wondering how I can move this change forward.

Regards,

Hironori Bono

[Tony Chang]

Have you contacted [e-mail]?  I just want to make sure we have buy-in from other browser developers that this is the right way to handle IME input on windowless plugins.

[Hironori Bono]

Greetings,

Thank you for your comment.

(In reply to comment #15)
> Have you contacted [e-mail]?  I just want to make sure we have buy-in from other browser developers that this is the right way to handle IME input on windowless plugins.

Thank you for noticing the ML. After several bouncing, I have finally sent an e-mail to the ML today:
https://mail.mozilla.org/pipermail/plugin-futures/2011-June/000431.html

Nevertheless, we see an issue that a windowless Flash prevents us from typing IME text on a windowed Flash in a page that has both a windowless Flash and a windowed Flash <http://crbug.com/75556>. This affects some of top Japanese sites and we do need a short-team solution.

Regards,

Hironori Bono

[Darin Fisher (:fishd, Google)]

(In reply to comment #12)
> > i'm concerned about defining plugin-specific input events.  can you tell me more about
> > why these are input events for plugins and not input events for WebWidgets?  why wouldn't
> > we just add methods for setting and confirming composition on WebPlugin?
> 
> Thank you for your comment. I have also considered this option before I started implementing this change. In brief, I have added an input event just because an IME event was mostly coupled with a keyboard events and it looked safer to use the same framework that sends keyboard events to send IME events. I'm not sure which is the better option in terms of sharing code between Chromium and Safari, adding an input event or adding a method to WebPlugin. Unfortunately, my Safari change for this issue is still in progress. (Even though this change is currently a Chromium-specific change, I would like to share code with Safari Win as much as possible.)


Note, also that WebInputEvent is intended to match up with WebCore::Platform*Event.  You are adding a case where that is not true.

Also, I see that you are not eliminating WebWidget::*composition methods in favor of using input events for IME.  It just seems to me that we would be better served with an API on WebPlugin for IME composition that mirrors the API we have on WebWidget.

Perhaps we could even distill that API into a separate interface, which both WebWidget and WebPlugin extend?

[Hironori Bono]

Greetings Darin,

(In reply to comment #17)
> Note, also that WebInputEvent is intended to match up with WebCore::Platform*Event.  You are adding a case where that is not true.
> Also, I see that you are not eliminating WebWidget::*composition methods in favor of using input events for IME.  It just seems to me that we would be better served with an API on WebPlugin for IME composition that mirrors the API we have on WebWidget.
> Perhaps we could even distill that API into a separate interface, which both WebWidget and WebPlugin extend?

Thank you for your suggestion. I would like to update my change to satisfy your constraints. (I do not have any intention to stick to my opinion and to continue providing the worst user-experience to Chromium users.) But, I cannot figure out which option is better: copying the IME interfaces of WebWidget to WebPlugin, or; adding a new interface both to WebWidget and WebPlugin. In my personal opinion, I prefer to just copying the IME interfaces of WebWidget to WebPlugin because it is easier for me to implement it. Nevertheless, I'm afraid you do not like my choice and my work becomes in vain again. (I have already spent lots of time investigating Flash before implementing the current change, without noticing this work becomes in vain.) To prevent working for nothing again, I would like to clarify my choice is reasonable for you and remove any ambiguities. (Unfortunately, I cannot talk with you so often because of time difference.)

Regards,

Hironori Bono

[Darin Fisher (:fishd, Google)]

(In reply to comment #18)
> Thank you for your suggestion. I would like to update my change to satisfy your constraints. (I do not have any intention to stick to my opinion and to continue providing the worst user-experience to Chromium users.) But, I cannot figure out which option is better: copying the IME interfaces of WebWidget to WebPlugin, or; adding a new interface both to WebWidget and WebPlugin. In my personal opinion, I prefer to just copying the IME interfaces of WebWidget to WebPlugin because it is easier for me to implement it. Nevertheless, I'm afraid you do not like my choice and my work becomes in vain again. (I have already spent lots of time investigating Flash before implementing the current change, without noticing this work becomes in vain.) To prevent working for nothing again, I would like to clarify my choice is reasonable for you and remove any ambiguities. (Unfortunately, I cannot talk with you so often because of time difference.)

Would you consider posting a quick patch to show what the additional methods on WebPlugin might look like?  Glancing at WebWidget, it isn't immediately obvious to me.  I'm guessing this includes all of the methods with "composition" in their name.  But, there's also textInputType() and caretOrSelectionBounds(), etc..  I can't tell if you need to put those on WebPlugin too.

Please don't spend a lot of time rewriting your patch.  Just post what the interface changes would be.  We can quickly review those, and then once we agree on the interface, the rest should go quickly without nearly as much back-n-forth.  Sound good?

[Hironori Bono]

Greetings Alexey,

(In reply to comment #3)
> Adding composition events to WebEvent seems unfortunate, as there are two completely different code paths for handling these now. Anders, Jeff or Sam are probably the best people to discuss this with.

Thank you for your suggestion and sorry for my slow response. It took long time to find a good solution for Safari.
I have filed Bug 64345 <http://webkit.org/b/64345> for Safari and uploaded the change for Safari because it also needs changes to the PluginView class. It would be great to read the change and to give me the comments of Safari experts there.

Regards,

Hironori Bono