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

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

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.

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

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

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.

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

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.

(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.

(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

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?

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

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.

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

Have you contacted plugin-futures@mozilla.org?  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.

Greetings,

Thank you for your comment.

(In reply to comment #15)
> Have you contacted plugin-futures@mozilla.org?  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

(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?

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

(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?

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