768742 - OS/2 IME level3 support

Status: RESOLVED FIXED

(Core Graveyard :: Widget: OS/2, defect)

Description

[KO Myung-Hun]

User Agent: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.1.19) Gecko/20110421 SeaMonkey/2.0.14
Build ID: 20110421220512

Steps to reproduce:

Input DBCS chars


Actual results:

IME level3 does not work


Expected results:

IME level3 should work

Comment 1

[KO Myung-Hun]

This is a tracking bug to implement OS/2 IME level3 support.

Comment 2

[KO Myung-Hun]

Attachment: First patch

Comment 3

[KO Myung-Hun]

Know problem :

If a focus is changed due to mouse button click, a conversion string still exists on an old widget. And a conversions string suddenly appears on a new window when starting to input a DBCS char.

Comment 4

[KO Myung-Hun]

Attachment: Second patch

Fix a focus problem

Comment 5

[Dave Yeo]

The patch seems to have been on top of other patches and does not apply. Please qpop -a, put the patch from bug#684487 at the top and this next in .hg\patches\series, qpush it and qrefresh it after fixing the rejected hunks. Plus when posting the updated patch, the comment should mention it is meant to be applied on top of the other patch.

Comment 6

[KO Myung-Hun]

Attachment: Patch against trunk

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78353003288e

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/78353003288e

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Looks like that the patch's event order is wrong. compositionupdate event MUST be fired only when the composition string is changed, and before dispatching text event. So, you need to store dispatched composition string and need to compare it with newer composition string before dispatching compositionupdate event.

Comment 10

[KO Myung-Hun]

If so, how about compositionend event ? It must be processed before text event, too ?

And where can I get the documents for these ?

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to KO Myung-Hun from comment #10)
> If so, how about compositionend event ? It must be processed before text
> event, too ?

compositionend event must be dispatched immediately after a text event whose string doesn't contain "composing" state.

> And where can I get the documents for these ?

Ah, I've not written it yet :-(

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI:

dispatching compositionupdate event and text event on Windows:
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsIMM32Handler.cpp#1648

dispatching compositionupdate event and text event and compositionend event on Windows at commit:
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsIMM32Handler.cpp#1090

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Note that if the order of the events are different from other platforms, some web applications may not work correctly.

In current our design, web applications can NOT see the latest composition string in editor from compositionupdate event because text event hasn't been dispatched yet. After that, text event would cause HTML5 input event. At input event handler, web applications can see the result of the latest composition events, i.e., editor contains the latest composition string.

This fact has been documented in MDN for web application developers:
https://developer.mozilla.org/en/DOM/DOM_event_reference/compositionupdate

Comment 14

[KO Myung-Hun]

Attachment: Patch to fix a compositionupdate event problem

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 641437 [details] [diff] [review]
Patch to fix a compositionupdate event problem

Could you attach the patch created with hg diff -U 8 -p?

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Er, hg qdiff -U 8 -p if you're using mq.

Comment 17

[KO Myung-Hun]

Attachment: Patch reformatted with -U 8 -p

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 641688 [details] [diff] [review]
Patch reformatted with -U 8 -p

># HG changeset patch
># Parent 32c911bc25790a2b70a64b05972d3edfe3cc12a8
>Fix a buggy process of compositionupdate event
>
>1. compositionupdate must be fired before dispatching text event
>
>2. compositionupdate must be fired only if a conversion string was changed
>
>diff --git a/widget/os2/nsWindow.cpp b/widget/os2/nsWindow.cpp
>--- a/widget/os2/nsWindow.cpp
>+++ b/widget/os2/nsWindow.cpp
>@@ -2484,16 +2484,18 @@
>   if (spfnImGetResultString(himi, IMR_RESULT_RESULTSTRING, pBuf,
>                             &ulBufLen)) {
>     delete pBuf;
> 
>     return false;
>   }
> 
>   if (!mIsComposing) {
>+    mLastDispatchedCompositionString.Truncate();
>+
>     nsCompositionEvent start(true, NS_COMPOSITION_START, this);
>     InitEvent(start);
>     DispatchWindowEvent(&start);
> 
>     mIsComposing = true;
>   }
> 
>   nsAutoChar16Buffer outBuf;
>@@ -2507,16 +2509,17 @@
>   text.theText = outBuf.Elements();
>   DispatchWindowEvent(&text);
> 
>   nsCompositionEvent end(true, NS_COMPOSITION_END, this);
>   InitEvent(end);
>   end.data = text.theText;
>   DispatchWindowEvent(&end);
>   mIsComposing = false;
>+  mLastDispatchedCompositionString.Truncate();
> 
>   return true;
> }

These changes look like in nsWindow::ImeResultString(). Then, please fire NS_COMPOSITION_UPDATE before dispatching NS_TEXT_TEXT. Of course, only when the committing text is different from mLastDispatchedCompositionString.

> 
> bool nsWindow::ImeConversionString(HIMI himi)
> {
>   PCHAR pBuf;
>   ULONG ulBufLen;
>@@ -2534,64 +2537,72 @@
>   if (spfnImGetConversionString(himi, IMR_CONV_CONVERSIONSTRING, pBuf,
>                                 &ulBufLen)) {
>     delete pBuf;
> 
>     return false;
>   }
> 
>   if (!mIsComposing) {
>+    mLastDispatchedCompositionString.Truncate();
>+
>     nsCompositionEvent start(true, NS_COMPOSITION_START, this);
>     InitEvent(start);
>     DispatchWindowEvent(&start);
> 
>     mIsComposing = true;
>   }
> 
>   nsAutoChar16Buffer outBuf;
>   PRInt32 outBufLen;
>   MultiByteToWideChar(0, pBuf, ulBufLen, outBuf, outBufLen);
> 
>   delete pBuf;
> 
>+  nsString compositionString;
>+
>+  compositionString = outBuf.Elements();
>+

I think that nsString shouldn't use here. It could hurt the heap.
Cannot you use nsDependentString, instead? It just wraps the buffer with nsString class. If you cannot use it, please use nsAutoString. nsAutoString doesn't hurt the heap if the string is short enough (only available on the stack, so, cannot be used for class members).

>   nsAutoTArray<nsTextRange, 4> textRanges;
> 
>   // Is there a conversion string ?
>-  if (outBufLen) {
>+  if (mLastDispatchedCompositionString != compositionString) {
>+    nsCompositionEvent update(true, NS_COMPOSITION_UPDATE, this);
>+    InitEvent(update);
>+    update.data = compositionString;
>+    mLastDispatchedCompositionString = compositionString;
>+    DispatchWindowEvent(&update);

Here is okay, but...

>+
>     nsTextRange newRange;
>     newRange.mStartOffset = 0;
>-    newRange.mEndOffset = outBufLen;
>+    newRange.mEndOffset = compositionString.Length();
>     newRange.mRangeType = NS_TEXTRANGE_SELECTEDRAWTEXT;
>     textRanges.AppendElement(newRange);
> 
>-    newRange.mStartOffset = outBufLen;
>+    newRange.mStartOffset = compositionString.Length();
>     newRange.mEndOffset = newRange.mStartOffset;
>     newRange.mRangeType = NS_TEXTRANGE_CARETPOSITION;
>     textRanges.AppendElement(newRange);
>   }

The textRanges change is necessary even if composition string isn't changing actually. So, you should close the |if (mLastDispatchedCompositionString != compositionString) {| block immediately after |DispatchWindowEvent(&update);|. And you should add |if (!compositionString.IsEmpty()) {| this code.

FYI: I think that this code doesn't work with Japanese IME becasue Japanese IME can separate a composition string to two or more clauses. But this code sets only one clause for all of the composition string. But you don't need to do anything for this for now because each patch should fix only one issue.

Otherwise, looks o.k. for me.

Comment 19

[KO Myung-Hun]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #18)
> Comment on attachment 641688 [details] [diff] [review]
> Patch reformatted with -U 8 -p
> 
> ># HG changeset patch
> ># Parent 32c911bc25790a2b70a64b05972d3edfe3cc12a8
> >Fix a buggy process of compositionupdate event
> >
> >1. compositionupdate must be fired before dispatching text event
> >
> >2. compositionupdate must be fired only if a conversion string was changed
> >
> >diff --git a/widget/os2/nsWindow.cpp b/widget/os2/nsWindow.cpp
> >--- a/widget/os2/nsWindow.cpp
> >+++ b/widget/os2/nsWindow.cpp
> >@@ -2507,16 +2509,17 @@
> >   text.theText = outBuf.Elements();
> >   DispatchWindowEvent(&text);
> > 
> >   nsCompositionEvent end(true, NS_COMPOSITION_END, this);
> >   InitEvent(end);
> >   end.data = text.theText;
> >   DispatchWindowEvent(&end);
> >   mIsComposing = false;
> >+  mLastDispatchedCompositionString.Truncate();
> > 
> >   return true;
> > }
> 
> These changes look like in nsWindow::ImeResultString(). Then, please fire
> NS_COMPOSITION_UPDATE before dispatching NS_TEXT_TEXT. Of course, only when
> the committing text is different from mLastDispatchedCompositionString.
> 

NS_COMPOSITION_UPDATE should be fired whenever dispatching NS_TEXT_TEXT ?

> > 
> > bool nsWindow::ImeConversionString(HIMI himi)
> > {
> >   PCHAR pBuf;
> >   ULONG ulBufLen;
> >@@ -2534,64 +2537,72 @@
> >   if (spfnImGetConversionString(himi, IMR_CONV_CONVERSIONSTRING, pBuf,
> >                                 &ulBufLen)) {
> >     delete pBuf;
> > 
> >     return false;
> >   }
> > 
> >   if (!mIsComposing) {
> >+    mLastDispatchedCompositionString.Truncate();
> >+
> >     nsCompositionEvent start(true, NS_COMPOSITION_START, this);
> >     InitEvent(start);
> >     DispatchWindowEvent(&start);
> > 
> >     mIsComposing = true;
> >   }
> > 
> >   nsAutoChar16Buffer outBuf;
> >   PRInt32 outBufLen;
> >   MultiByteToWideChar(0, pBuf, ulBufLen, outBuf, outBufLen);
> > 
> >   delete pBuf;
> > 
> >+  nsString compositionString;
> >+
> >+  compositionString = outBuf.Elements();
> >+
> 
> I think that nsString shouldn't use here. It could hurt the heap.
> Cannot you use nsDependentString, instead? It just wraps the buffer with
> nsString class. If you cannot use it, please use nsAutoString. nsAutoString
> doesn't hurt the heap if the string is short enough (only available on the
> stack, so, cannot be used for class members).
> 

How about declaring it as a class member like mLastDispatchedCompositionString ?

> >   nsAutoTArray<nsTextRange, 4> textRanges;
> > 
> >   // Is there a conversion string ?
> >-  if (outBufLen) {
> >+  if (mLastDispatchedCompositionString != compositionString) {
> >+    nsCompositionEvent update(true, NS_COMPOSITION_UPDATE, this);
> >+    InitEvent(update);
> >+    update.data = compositionString;
> >+    mLastDispatchedCompositionString = compositionString;
> >+    DispatchWindowEvent(&update);
> 
> Here is okay, but...
> 
> >+
> >     nsTextRange newRange;
> >     newRange.mStartOffset = 0;
> >-    newRange.mEndOffset = outBufLen;
> >+    newRange.mEndOffset = compositionString.Length();
> >     newRange.mRangeType = NS_TEXTRANGE_SELECTEDRAWTEXT;
> >     textRanges.AppendElement(newRange);
> > 
> >-    newRange.mStartOffset = outBufLen;
> >+    newRange.mStartOffset = compositionString.Length();
> >     newRange.mEndOffset = newRange.mStartOffset;
> >     newRange.mRangeType = NS_TEXTRANGE_CARETPOSITION;
> >     textRanges.AppendElement(newRange);
> >   }
> 
> The textRanges change is necessary even if composition string isn't changing
> actually. So, you should close the |if (mLastDispatchedCompositionString !=
> compositionString) {| block immediately after
> |DispatchWindowEvent(&update);|. And you should add |if
> (!compositionString.IsEmpty()) {| this code.
> 

Ok.

> FYI: I think that this code doesn't work with Japanese IME becasue Japanese
> IME can separate a composition string to two or more clauses. But this code
> sets only one clause for all of the composition string. But you don't need
> to do anything for this for now because each patch should fix only one issue.
> 

Unforunately, I don't know about Japanese IME, but when I asked to some Japanese users, they said that they didn't have problem at all. 

Of course, if anyone reports this problem, I'll try to fix it, then.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to KO Myung-Hun from comment #19)
> > These changes look like in nsWindow::ImeResultString(). Then, please fire
> > NS_COMPOSITION_UPDATE before dispatching NS_TEXT_TEXT. Of course, only when
> > the committing text is different from mLastDispatchedCompositionString.
> > 
> 
> NS_COMPOSITION_UPDATE should be fired whenever dispatching NS_TEXT_TEXT ?

Please think like this. We say "composing" during NS_COMPOSITION_START and NS_COMPOSITION_END. NS_COMPOSITION_UPDATE must be dispatched during "composing" and only when the composition string is actually changed. If you dispatched NS_TEXT_TEXT event at not "composing", you don't need to dispatch NS_COMPOSITION_UPDATE (I don't know if such case actually occurs and works). Otherwise, you need to dispatch NS_COMPOSITION_UPDATE before NS_TEXT_TEXT if *composition string is changed*.

Here is the spec:
http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#event-type-compositionupdate

> > > bool nsWindow::ImeConversionString(HIMI himi)
> > > {
> > >   PCHAR pBuf;
> > >   ULONG ulBufLen;
> > >@@ -2534,64 +2537,72 @@
> > >   if (spfnImGetConversionString(himi, IMR_CONV_CONVERSIONSTRING, pBuf,
> > >                                 &ulBufLen)) {
> > >     delete pBuf;
> > > 
> > >     return false;
> > >   }
> > > 
> > >   if (!mIsComposing) {
> > >+    mLastDispatchedCompositionString.Truncate();
> > >+
> > >     nsCompositionEvent start(true, NS_COMPOSITION_START, this);
> > >     InitEvent(start);
> > >     DispatchWindowEvent(&start);
> > > 
> > >     mIsComposing = true;
> > >   }
> > > 
> > >   nsAutoChar16Buffer outBuf;
> > >   PRInt32 outBufLen;
> > >   MultiByteToWideChar(0, pBuf, ulBufLen, outBuf, outBufLen);
> > > 
> > >   delete pBuf;
> > > 
> > >+  nsString compositionString;
> > >+
> > >+  compositionString = outBuf.Elements();
> > >+
> > 
> > I think that nsString shouldn't use here. It could hurt the heap.
> > Cannot you use nsDependentString, instead? It just wraps the buffer with
> > nsString class. If you cannot use it, please use nsAutoString. nsAutoString
> > doesn't hurt the heap if the string is short enough (only available on the
> > stack, so, cannot be used for class members).
> > 
> 
> How about declaring it as a class member like
> mLastDispatchedCompositionString ?

I don't think that it's good idea if it's not referenced many times. It's better to store the them only in stack.

> > FYI: I think that this code doesn't work with Japanese IME becasue Japanese
> > IME can separate a composition string to two or more clauses. But this code
> > sets only one clause for all of the composition string. But you don't need
> > to do anything for this for now because each patch should fix only one issue.
> > 
> 
> Unforunately, I don't know about Japanese IME, but when I asked to some
> Japanese users, they said that they didn't have problem at all. 
> 
> Of course, if anyone reports this problem, I'll try to fix it, then.

Thanks.

I don't know actual behavior of IME on OS/2. But traditionally, Japanese IME can have two or more clauses, so, one of the clauses must be "focused" by NS_TEXTRANGE_SELECTED*TEXT and the others must be "unfocused" by NS_TEXTRANGE_RAWINPUT or NS_TEXTRANGE_CONVERTEDTEXT.

See this section: http://www.w3.org/TR/2012/WD-ime-api-20120524/#selected-clause

There are 4 clauses in the composition string on the Notepad. The thicker underlined clause is the "focused" clause. Only the "focused" clause is being converted in the screenshot.

Comment 21

[KO Myung-Hun]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #20)
> (In reply to KO Myung-Hun from comment #19)
> 
> I don't know actual behavior of IME on OS/2. But traditionally, Japanese IME
> can have two or more clauses, so, one of the clauses must be "focused" by
> NS_TEXTRANGE_SELECTED*TEXT and the others must be "unfocused" by
> NS_TEXTRANGE_RAWINPUT or NS_TEXTRANGE_CONVERTEDTEXT.
> 
> See this section:
> http://www.w3.org/TR/2012/WD-ime-api-20120524/#selected-clause
> 
> There are 4 clauses in the composition string on the Notepad. The thicker
> underlined clause is the "focused" clause. Only the "focused" clause is
> being converted in the screenshot.

Thanks for your information. ^^

Comment 22

[KO Myung-Hun]

Attachment: v3 for compositionupdate

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 641770 [details] [diff] [review]
v3 for compositionupdate

># HG changeset patch
># Parent 32c911bc25790a2b70a64b05972d3edfe3cc12a8
>Fix a buggy process of compositionupdate event
>
>1. compositionupdate must be fired before dispatching text event
>
>2. compositionupdate must be fired only if a conversion string was changed
>
>diff --git a/widget/os2/nsWindow.cpp b/widget/os2/nsWindow.cpp
>--- a/widget/os2/nsWindow.cpp
>+++ b/widget/os2/nsWindow.cpp
>@@ -2484,39 +2484,54 @@
>   if (spfnImGetResultString(himi, IMR_RESULT_RESULTSTRING, pBuf,
>                             &ulBufLen)) {
>     delete pBuf;
>
>     return false;
>   }
>
>   if (!mIsComposing) {
>+    mLastDispatchedCompositionString.Truncate();
>+
>     nsCompositionEvent start(true, NS_COMPOSITION_START, this);
>     InitEvent(start);
>     DispatchWindowEvent(&start);
>
>     mIsComposing = true;
>   }
>
>   nsAutoChar16Buffer outBuf;
>   PRInt32 outBufLen;
>   MultiByteToWideChar(0, pBuf, ulBufLen, outBuf, outBufLen);
>
>   delete pBuf;
>
>+  nsAutoString compositionString;
>+
>+  compositionString = outBuf.Elements();

Please use this style:
nsAutoString compositionString(outBuf.Elements());

>   nsAutoChar16Buffer outBuf;
>   PRInt32 outBufLen;
>   MultiByteToWideChar(0, pBuf, ulBufLen, outBuf, outBufLen);
>
>   delete pBuf;
>
>+  nsAutoString compositionString;
>+
>+  compositionString = outBuf.Elements();

Same as above.

>   nsAutoTArray<nsTextRange, 4> textRanges;
>
>-  // Is there a conversion string ?
>-  if (outBufLen) {
>+  if (!compositionString.IsEmpty())
>+  {

uplist the "{" to above line.

I.e., |if (!compositionString.IsEmpty()) {|


Otherwise, looks okay for me. Would you attach new patch with the fix? Then, I'll check in it later.

Thank you for your great work!

Comment 24

[KO Myung-Hun]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #23)
> Comment on attachment 641770 [details] [diff] [review]
> v3 for compositionupdate
> 
> ># HG changeset patch
> ># Parent 32c911bc25790a2b70a64b05972d3edfe3cc12a8
> >Fix a buggy process of compositionupdate event
> >
> >1. compositionupdate must be fired before dispatching text event
> >
> >2. compositionupdate must be fired only if a conversion string was changed
> >
> >diff --git a/widget/os2/nsWindow.cpp b/widget/os2/nsWindow.cpp
> >--- a/widget/os2/nsWindow.cpp
> >+++ b/widget/os2/nsWindow.cpp
> >@@ -2484,39 +2484,54 @@
> >
> >+  nsAutoString compositionString;
> >+
> >+  compositionString = outBuf.Elements();
> 
> Please use this style:
> nsAutoString compositionString(outBuf.Elements());
> 

Ooops... It's my unfamiliarity with C++. T.T

> >   nsAutoChar16Buffer outBuf;
> >   PRInt32 outBufLen;
> >   MultiByteToWideChar(0, pBuf, ulBufLen, outBuf, outBufLen);
> >
> >   delete pBuf;
> >
> >+  nsAutoString compositionString;
> >+
> >+  compositionString = outBuf.Elements();
> 
> Same as above.
> 
> >   nsAutoTArray<nsTextRange, 4> textRanges;
> >
> >-  // Is there a conversion string ?
> >-  if (outBufLen) {
> >+  if (!compositionString.IsEmpty())
> >+  {
> 
> uplist the "{" to above line.
> 
> I.e., |if (!compositionString.IsEmpty()) {|
> 
> 

Ok.

> Otherwise, looks okay for me. Would you attach new patch with the fix? Then,
> I'll check in it later.
> 

Of course, thanks.

> Thank you for your great work!

It's my pleasure. ^_____^

Comment 25

[KO Myung-Hun]

Attachment: v4 for compositionupdate

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 641831 [details] [diff] [review]
v4 for compositionupdate

Sorry for the delay, I'll land this soon.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/mozilla-central/rev/2b1f30074100

Thank you, again!

Comment 28

[KO Myung-Hun]

Attachment: Suppoert multiple clauses

This patch fixes the the multiple clauses problem on OS/2.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 757391 [details] [diff] [review]
Suppoert multiple clauses

> bool nsWindow::ImeResultString(HIMI himi)
> {
>-  PCHAR pBuf;
>   ULONG ulBufLen;
>
>   // Get a buffer size
>   ulBufLen = 0;
>   if (spfnImGetResultString(himi, IMR_RESULT_RESULTSTRING, NULL, &ulBufLen))
>     return false;
>
>-  pBuf = new CHAR[ulBufLen];
>-  if (!pBuf)
>+  nsTArray<CHAR> compositionStringA;

|nsAutoTArray<CHAR, 64>| is better. If the length is less than 64 (it should be so!), SetCapacity() doesn't mess the heap.

>+  if (!compositionStringA.SetCapacity(ulBufLen / sizeof(CHAR)))
>     return false;

Would you use |{}|?

>@@ -2522,76 +2517,163 @@
>   end.data = compositionString;
>   DispatchWindowEvent(&end);
>   mIsComposing = false;
>   mLastDispatchedCompositionString.Truncate();
>
>   return true;
> }
>
>+static PRUint32
>+PlatformToNSAttr(PRUint8 aAttr)
>+{
>+  switch (aAttr)
>+  {
>+    case CP_ATTR_INPUT_ERROR:
>+    case CP_ATTR_INPUT:
>+      return NS_TEXTRANGE_RAWINPUT;
>+
>+    case CP_ATTR_CONVERTED:
>+      return NS_TEXTRANGE_CONVERTEDTEXT;
>+
>+    case CP_ATTR_TARGET_NOTCONVERTED:
>+      return NS_TEXTRANGE_SELECTEDRAWTEXT;
>+
>+    case CP_ATTR_TARGET_CONVERTED:
>+      return NS_TEXTRANGE_SELECTEDCONVERTEDTEXT;
>+
>+    default:
>+      NS_ASSERTION(false, "unknown attribute");

Looks like MOZ_NOT_REACHED() is better for here. It aborts the process even if it's a release build.

>+      return NS_TEXTRANGE_CARETPOSITION;

Looks like it's strange. NS_TEXTRANGE_RAWINPUT looks better. Although, the return state never runs actually.

>+  }
>+}
>+
> bool nsWindow::ImeConversionString(HIMI himi)
> {
>-  PCHAR pBuf;
>   ULONG ulBufLen;
>
>   // Get a buffer size
>   ulBufLen = 0;
>   if (spfnImGetConversionString(himi, IMR_CONV_CONVERSIONSTRING, NULL,
>                                 &ulBufLen))
>     return false;
>
>-  pBuf = new CHAR[ulBufLen];
>-  if (!pBuf)
>+  nsTArray<CHAR> compositionStringA;

Same, please use |nsAutoTArray<CHAR, 64>|.

>+  if (!compositionStringA.SetCapacity(ulBufLen / sizeof(CHAR)))
>     return false;

IIRC, when OOM occurs, our library makes the process crashed. So, you don't need to check it, you need just to call the method.

>
>-  if (spfnImGetConversionString(himi, IMR_CONV_CONVERSIONSTRING, pBuf,
>-                                &ulBufLen)) {
>-    delete pBuf;
>-
>+  if (spfnImGetConversionString(himi, IMR_CONV_CONVERSIONSTRING,
>+                                compositionStringA.Elements(), &ulBufLen))
>     return false;
>-  }
>
>   if (!mIsComposing) {
>     mLastDispatchedCompositionString.Truncate();
>
>     nsCompositionEvent start(true, NS_COMPOSITION_START, this);
>     InitEvent(start);
>     DispatchWindowEvent(&start);
>
>     mIsComposing = true;
>   }
>
>   nsAutoChar16Buffer outBuf;
>   int32_t outBufLen;
>-  MultiByteToWideChar(0, pBuf, ulBufLen, outBuf, outBufLen);
>-
>-  delete pBuf;
>+  MultiByteToWideChar(0, compositionStringA.Elements(), ulBufLen,
>+                      outBuf, outBufLen);
>
>   nsAutoString compositionString(outBuf.Elements());
>
>   // Is a conversion string changed ?
>   if (mLastDispatchedCompositionString != compositionString) {
>     nsCompositionEvent update(true, NS_COMPOSITION_UPDATE, this);
>     InitEvent(update);
>     update.data = compositionString;
>     mLastDispatchedCompositionString = compositionString;
>     DispatchWindowEvent(&update);
>   }
>
>   nsAutoTArray<nsTextRange, 4> textRanges;
>
>   if (!compositionString.IsEmpty()) {
>+    ulBufLen = 0;
>+    if (spfnImGetConversionString(himi, IMR_CONV_CONVERSIONCLAUSE, 0,
>+                                  &ulBufLen))
>+      return false;

Please use |{}| here.

However, I have some worry here. If the method fails only when OOM occurs, this doesn't have problem. However, if not so, we should assume that there is only one clause which includes all of the composition string like current code.

So, I believe that you should create a new method for setting textRanges, e.g., |bool SetTextRanges(HIMI aHIMI, const nsAString& aCompositionString, nsTArray<nsTextRanges>& aTextRanges)|. Then, if the new method fails, the caller should set only one clause like current code.

>+
>+    ULONG ulClauseCount = NS_MAX<ULONG>(2, ulBufLen / sizeof(ULONG));

Use std::max().

>+    nsTArray<ULONG> clauseOffsets;
>+    nsTArray<UCHAR> clauseAttr;

Similar to above, please use nsAutoTArray<ULONG, 4> and nsAutoTArray<UCHAR, 4>.

>+    ULONG ulCursorPos;
>+
>+    if (!clauseOffsets.SetCapacity(ulClauseCount) ||
>+        !clauseAttr.SetCapacity(ulClauseCount))
>+      return false;

Same as above, just call them. You don't need to check the result.

>+
>+    if (spfnImGetConversionString(himi, IMR_CONV_CONVERSIONCLAUSE,
>+                                  clauseOffsets.Elements(), &ulBufLen))
>+      return false;

Pleas use |{}|.

>+
>+    // Korean IME does not provide clause and cursor infomation
>+    if (ulBufLen == 0) {
>+      clauseOffsets[0] = 0;
>+      clauseOffsets[1] = compositionString.Length();
>+      clauseAttr[0] = NS_TEXTRANGE_SELECTEDRAWTEXT;
>+      ulCursorPos = compositionString.Length();
>+    }
>+    else
>+    {

|} else {| is our style, although, I don't know the local rules under widget/os2.

>+      ulBufLen = 0;
>+      if (spfnImGetConversionString(himi, IMR_CONV_CONVERSIONATTR, 0,
>+                                    &ulBufLen))
>+        return false;

Pleas use |{}|.

>+
>+      nsTArray<UCHAR> attr;

Please use |nsAutoTArray<UCHAR, 64>|.

>+      if (!attr.SetCapacity(ulBufLen / sizeof(UCHAR)))
>+        return false;

You don't need to check the result.

>+
>+      if (spfnImGetConversionString(himi, IMR_CONV_CONVERSIONATTR,
>+                                    attr.Elements(), &ulBufLen))
>+        return false;

Please use |{}|.

>+
>+      // Assume that all the conversion attribute in a clause are same
>+      for (ULONG i = 0; i < ulClauseCount - 1; ++i)
>+        clauseAttr[i] = attr[clauseOffsets[i]];

Please use |{}|.

>+
>+      // Convert ANSI string offsets to Unicode string offsets
>+      ULONG lastAnsiOffset = clauseOffsets[0] = 0;
>+      for (ULONG i = 1; i < ulClauseCount - 1; ++i) {
>+        MultiByteToWideChar(0,
>+                            compositionStringA.Elements() + lastAnsiOffset,
>+                            clauseOffsets[i] - lastAnsiOffset,
>+                            outBuf, outBufLen);
>+        lastAnsiOffset = clauseOffsets[i];
>+        clauseOffsets[i] = clauseOffsets[i - 1] + outBufLen;
>+      }

Hmm, looks like you don't need to use lastAnsiOffset if you always convert the ANSI string from its start. I think that it makes the for loop simpler.

>+
>+      ulBufLen = sizeof(ULONG);
>+      if (spfnImGetConversionString(himi, IMR_CONV_CURSORPOS, &ulCursorPos,
>+                                    &ulBufLen))
>+        return false;

Use |{}|.

>+
>+      // Convert ANSI string position to Unicode string position
>+      MultiByteToWideChar(0, compositionStringA.Elements(), ulCursorPos,
>+                          outBuf, outBufLen);
>+      ulCursorPos = outBufLen;

FYI: on Windows, IME may not have cursor position. So, I think that you should append caret range only when the spfnImGetConversionString() with IMR_CONV_CURSORPOS succeeded. It must be safer.


Thank you for your work. I'd like to review your patch again after you update this.

Comment 30

[KO Myung-Hun]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (Offline 6/11 - 6/17) from comment #29)
> Comment on attachment 757391 [details] [diff] [review]
> Suppoert multiple clauses
> 
> > bool nsWindow::ImeResultString(HIMI himi)
> > {
> >-  PCHAR pBuf;
> >   ULONG ulBufLen;
> >
> >   // Get a buffer size
> >   ulBufLen = 0;
> >   if (spfnImGetResultString(himi, IMR_RESULT_RESULTSTRING, NULL, &ulBufLen))
> >     return false;
> >
> >-  pBuf = new CHAR[ulBufLen];
> >-  if (!pBuf)
> >+  nsTArray<CHAR> compositionStringA;
> 
> |nsAutoTArray<CHAR, 64>| is better. If the length is less than 64 (it should
> be so!), SetCapacity() doesn't mess the heap.
> 
> >+  if (!compositionStringA.SetCapacity(ulBufLen / sizeof(CHAR)))
> >     return false;
> 
> Would you use |{}|?
> 

No problem.

> >@@ -2522,76 +2517,163 @@
> >   end.data = compositionString;
> >   DispatchWindowEvent(&end);
> >   mIsComposing = false;
> >   mLastDispatchedCompositionString.Truncate();
> >
> >   return true;
> > }
> >
> >+static PRUint32
> >+PlatformToNSAttr(PRUint8 aAttr)
> >+{
> >+  switch (aAttr)
> >+  {
> >+    case CP_ATTR_INPUT_ERROR:
> >+    case CP_ATTR_INPUT:
> >+      return NS_TEXTRANGE_RAWINPUT;
> >+
> >+    case CP_ATTR_CONVERTED:
> >+      return NS_TEXTRANGE_CONVERTEDTEXT;
> >+
> >+    case CP_ATTR_TARGET_NOTCONVERTED:
> >+      return NS_TEXTRANGE_SELECTEDRAWTEXT;
> >+
> >+    case CP_ATTR_TARGET_CONVERTED:
> >+      return NS_TEXTRANGE_SELECTEDCONVERTEDTEXT;
> >+
> >+    default:
> >+      NS_ASSERTION(false, "unknown attribute");
> 
> Looks like MOZ_NOT_REACHED() is better for here. It aborts the process even
> if it's a release build.
> 
> >+      return NS_TEXTRANGE_CARETPOSITION;
> 
> Looks like it's strange. NS_TEXTRANGE_RAWINPUT looks better. Although, the
> return state never runs actually.
> 

I borrowed this function from windows implementation. Anyway no problem. ^^

> >+  }
> >+}
> >+
> > bool nsWindow::ImeConversionString(HIMI himi)
> > {
> >-  PCHAR pBuf;
> >   ULONG ulBufLen;
> >
> >   // Get a buffer size
> >   ulBufLen = 0;
> >   if (spfnImGetConversionString(himi, IMR_CONV_CONVERSIONSTRING, NULL,
> >                                 &ulBufLen))
> >     return false;
> >
> >-  pBuf = new CHAR[ulBufLen];
> >-  if (!pBuf)
> >+  nsTArray<CHAR> compositionStringA;
> 
> Same, please use |nsAutoTArray<CHAR, 64>|.
> 

Ok.

> >+  if (!compositionStringA.SetCapacity(ulBufLen / sizeof(CHAR)))
> >     return false;
> 
> IIRC, when OOM occurs, our library makes the process crashed. So, you don't
> need to check it, you need just to call the method.
> 

Good information. ^^

> >
> >-  if (spfnImGetConversionString(himi, IMR_CONV_CONVERSIONSTRING, pBuf,
> >-                                &ulBufLen)) {
> >-    delete pBuf;
> >-
> >+  if (spfnImGetConversionString(himi, IMR_CONV_CONVERSIONSTRING,
> >+                                compositionStringA.Elements(), &ulBufLen))
> >     return false;
> >-  }
> >
> >   if (!mIsComposing) {
> >     mLastDispatchedCompositionString.Truncate();
> >
> >     nsCompositionEvent start(true, NS_COMPOSITION_START, this);
> >     InitEvent(start);
> >     DispatchWindowEvent(&start);
> >
> >     mIsComposing = true;
> >   }
> >
> >   nsAutoChar16Buffer outBuf;
> >   int32_t outBufLen;
> >-  MultiByteToWideChar(0, pBuf, ulBufLen, outBuf, outBufLen);
> >-
> >-  delete pBuf;
> >+  MultiByteToWideChar(0, compositionStringA.Elements(), ulBufLen,
> >+                      outBuf, outBufLen);
> >
> >   nsAutoString compositionString(outBuf.Elements());
> >
> >   // Is a conversion string changed ?
> >   if (mLastDispatchedCompositionString != compositionString) {
> >     nsCompositionEvent update(true, NS_COMPOSITION_UPDATE, this);
> >     InitEvent(update);
> >     update.data = compositionString;
> >     mLastDispatchedCompositionString = compositionString;
> >     DispatchWindowEvent(&update);
> >   }
> >
> >   nsAutoTArray<nsTextRange, 4> textRanges;
> >
> >   if (!compositionString.IsEmpty()) {
> >+    ulBufLen = 0;
> >+    if (spfnImGetConversionString(himi, IMR_CONV_CONVERSIONCLAUSE, 0,
> >+                                  &ulBufLen))
> >+      return false;
> 
> Please use |{}| here.
> 

Ok.

> However, I have some worry here. If the method fails only when OOM occurs,
> this doesn't have problem. However, if not so, we should assume that there
> is only one clause which includes all of the composition string like current
> code.
> 
> So, I believe that you should create a new method for setting textRanges,
> e.g., |bool SetTextRanges(HIMI aHIMI, const nsAString& aCompositionString,
> nsTArray<nsTextRanges>& aTextRanges)|. Then, if the new method fails, the
> caller should set only one clause like current code.
> 

I've implemented with a simple block instead.

> >+
> >+    ULONG ulClauseCount = NS_MAX<ULONG>(2, ulBufLen / sizeof(ULONG));
> 
> Use std::max().
> 

Ok.

> >+    nsTArray<ULONG> clauseOffsets;
> >+    nsTArray<UCHAR> clauseAttr;
> 
> Similar to above, please use nsAutoTArray<ULONG, 4> and nsAutoTArray<UCHAR,
> 4>.
> 

Yes.

> >+    ULONG ulCursorPos;
> >+
> >+    if (!clauseOffsets.SetCapacity(ulClauseCount) ||
> >+        !clauseAttr.SetCapacity(ulClauseCount))
> >+      return false;
> 
> Same as above, just call them. You don't need to check the result.
> 

Ok.

> >+
> >+    if (spfnImGetConversionString(himi, IMR_CONV_CONVERSIONCLAUSE,
> >+                                  clauseOffsets.Elements(), &ulBufLen))
> >+      return false;
> 
> Pleas use |{}|.
> 

Ok.

> >+
> >+    // Korean IME does not provide clause and cursor infomation
> >+    if (ulBufLen == 0) {
> >+      clauseOffsets[0] = 0;
> >+      clauseOffsets[1] = compositionString.Length();
> >+      clauseAttr[0] = NS_TEXTRANGE_SELECTEDRAWTEXT;
> >+      ulCursorPos = compositionString.Length();
> >+    }
> >+    else
> >+    {
> 
> |} else {| is our style, although, I don't know the local rules under
> widget/os2.
> 

Sorry. It's my habit. ^^

> >+      ulBufLen = 0;
> >+      if (spfnImGetConversionString(himi, IMR_CONV_CONVERSIONATTR, 0,
> >+                                    &ulBufLen))
> >+        return false;
> 
> Pleas use |{}|.
> 

Ok.

> >+
> >+      nsTArray<UCHAR> attr;
> 
> Please use |nsAutoTArray<UCHAR, 64>|.
> 

Ok.

> >+      if (!attr.SetCapacity(ulBufLen / sizeof(UCHAR)))
> >+        return false;
> 
> You don't need to check the result.
> 

Yes.

> >+
> >+      if (spfnImGetConversionString(himi, IMR_CONV_CONVERSIONATTR,
> >+                                    attr.Elements(), &ulBufLen))
> >+        return false;
> 
> Please use |{}|.
> 

Ok.

> >+
> >+      // Assume that all the conversion attribute in a clause are same
> >+      for (ULONG i = 0; i < ulClauseCount - 1; ++i)
> >+        clauseAttr[i] = attr[clauseOffsets[i]];
> 
> Please use |{}|.
> 

Ok.

> >+
> >+      // Convert ANSI string offsets to Unicode string offsets
> >+      ULONG lastAnsiOffset = clauseOffsets[0] = 0;
> >+      for (ULONG i = 1; i < ulClauseCount - 1; ++i) {
> >+        MultiByteToWideChar(0,
> >+                            compositionStringA.Elements() + lastAnsiOffset,
> >+                            clauseOffsets[i] - lastAnsiOffset,
> >+                            outBuf, outBufLen);
> >+        lastAnsiOffset = clauseOffsets[i];
> >+        clauseOffsets[i] = clauseOffsets[i - 1] + outBufLen;
> >+      }
> 
> Hmm, looks like you don't need to use lastAnsiOffset if you always convert
> the ANSI string from its start. I think that it makes the for loop simpler.
> 

Good suggesion. Much simpler. ^^

> >+
> >+      ulBufLen = sizeof(ULONG);
> >+      if (spfnImGetConversionString(himi, IMR_CONV_CURSORPOS, &ulCursorPos,
> >+                                    &ulBufLen))
> >+        return false;
> 
> Use |{}|.
> 

Ok.

> >+
> >+      // Convert ANSI string position to Unicode string position
> >+      MultiByteToWideChar(0, compositionStringA.Elements(), ulCursorPos,
> >+                          outBuf, outBufLen);
> >+      ulCursorPos = outBufLen;
> 
> FYI: on Windows, IME may not have cursor position. So, I think that you
> should append caret range only when the spfnImGetConversionString() with
> IMR_CONV_CURSORPOS succeeded. It must be safer.
> 

Absolutely.

> 
> Thank you for your work. I'd like to review your patch again after you
> update this.

I've attached the updated patch. ^^

Comment 31

[KO Myung-Hun]

Attachment: Support multiple clauses V2

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 758538 [details] [diff] [review]
Support multiple clauses V2

># HG changeset patch
># Parent 318c97f3ed633baa18e2f04c45496c101d35a500
># User KO Myung-Hun <[e-mail]>
>Support multiple clauses of OS/2 IME

Insert the bug #.

>   if (!compositionString.IsEmpty()) {
>+    bool oneClause = false;

Hmm, I strongly recommend that you should separate this blog to a method, though. But it doesn't matter.

>+
>+    ulBufLen = 0;
>+    if (spfnImGetConversionString(himi, IMR_CONV_CONVERSIONCLAUSE, 0,
>+                                  &ulBufLen)) {
>+      oneClause = true;  // Assume that there is only one clause
>+    }
>+
>+    ULONG ulClauseCount = std::max(2UL, ulBufLen / sizeof(ULONG));
>+    nsAutoTArray<ULONG, 4> clauseOffsets;
>+    nsAutoTArray<UCHAR, 4> clauseAttr;
>+    ULONG ulCursorPos;
>+
>+    clauseOffsets.SetCapacity(ulClauseCount);

Shouldn't this be ulClauseCount + 1 according to your logic? But see below.

>+    clauseAttr.SetCapacity(ulClauseCount);
>+
>+    if (spfnImGetConversionString(himi, IMR_CONV_CONVERSIONCLAUSE,
>+                                  clauseOffsets.Elements(), &ulBufLen)) {
>+      oneClause = true;  // Assume that there is only one clause
>+    }

I have a question, does this API set clauseOffsets[ulClauseCount] to compositionString.Length()?

>+    for (ULONG i = 0; i < ulClauseCount - 1; ++i) {
>+      newRange.mStartOffset = clauseOffsets[i];
>+      newRange.mEndOffset = clauseOffsets[i + 1];
>+      newRange.mRangeType = PlatformToNSAttr(clauseAttr[i]);
>+      textRanges.AppendElement(newRange);
>+    }

If not so, the clauseOffsets[ulClauseCount] doesn't have expected value.

I guess that |clauseOffsets.SetCapacity(ulClauseCount)| is correct, and the value of clauseOffsets[ulClauseCount] isn't what we expect. If so, we should do:

newRange.mEndOffset =
  (i + 1 == ulClauseCount) ? compositionString.Length() :
                             clauseOffsets[i + 1];

Isn't it?

>+
>+    if (ulCursorPos != static_cast<ULONG>(-1)) {

I think that you should do |#define NO_IME_CARET (static_cast<ULONG>(-1))| for clearer code.

Comment 33

[KO Myung-Hun]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (Offline 6/11 - 6/17) from comment #32)
> Comment on attachment 758538 [details] [diff] [review]
> Support multiple clauses V2
> 
> ># HG changeset patch
> ># Parent 318c97f3ed633baa18e2f04c45496c101d35a500
> ># User KO Myung-Hun <[e-mail]>
> >Support multiple clauses of OS/2 IME
> 
> Insert the bug #.
> 

Ok.

> >   if (!compositionString.IsEmpty()) {
> >+    bool oneClause = false;
> 
> Hmm, I strongly recommend that you should separate this blog to a method,
> though. But it doesn't matter.
> 

Any special reasons ?

> >+
> >+    ulBufLen = 0;
> >+    if (spfnImGetConversionString(himi, IMR_CONV_CONVERSIONCLAUSE, 0,
> >+                                  &ulBufLen)) {
> >+      oneClause = true;  // Assume that there is only one clause
> >+    }
> >+
> >+    ULONG ulClauseCount = std::max(2UL, ulBufLen / sizeof(ULONG));
> >+    nsAutoTArray<ULONG, 4> clauseOffsets;
> >+    nsAutoTArray<UCHAR, 4> clauseAttr;
> >+    ULONG ulCursorPos;
> >+
> >+    clauseOffsets.SetCapacity(ulClauseCount);
> 
> Shouldn't this be ulClauseCount + 1 according to your logic? But see below.
> 

Yes, it's correct. See below.

> >+    clauseAttr.SetCapacity(ulClauseCount);
> >+
> >+    if (spfnImGetConversionString(himi, IMR_CONV_CONVERSIONCLAUSE,
> >+                                  clauseOffsets.Elements(), &ulBufLen)) {
> >+      oneClause = true;  // Assume that there is only one clause
> >+    }
> 
> I have a question, does this API set clauseOffsets[ulClauseCount] to
> compositionString.Length()?
> 

In my understanding, this API set clauseOffsets[ulClauseCount - 1] to compositionString.Length().

> >+    for (ULONG i = 0; i < ulClauseCount - 1; ++i) {
> >+      newRange.mStartOffset = clauseOffsets[i];
> >+      newRange.mEndOffset = clauseOffsets[i + 1];
> >+      newRange.mRangeType = PlatformToNSAttr(clauseAttr[i]);
> >+      textRanges.AppendElement(newRange);
> >+    }
> 
> If not so, the clauseOffsets[ulClauseCount] doesn't have expected value.
> 
> I guess that |clauseOffsets.SetCapacity(ulClauseCount)| is correct, and the
> value of clauseOffsets[ulClauseCount] isn't what we expect. If so, we should
> do:
> 
> newRange.mEndOffset =
>   (i + 1 == ulClauseCount) ? compositionString.Length() :
>                              clauseOffsets[i + 1];
> 
> Isn't it?
> 

See above.

> >+
> >+    if (ulCursorPos != static_cast<ULONG>(-1)) {
> 
> I think that you should do |#define NO_IME_CARET (static_cast<ULONG>(-1))|
> for clearer code.

Ok.

Comment 34

[KO Myung-Hun]

Attachment: Support multiple clauses V3

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 759737 [details] [diff] [review]
Support multiple clauses V3

Great! Thank you for your work!!

> > >   if (!compositionString.IsEmpty()) {
> > >+    bool oneClause = false;
> > 
> > Hmm, I strongly recommend that you should separate this blog to a method,
> > though. But it doesn't matter.
> > 
> 
> Any special reasons ?

Because if you separate the block into another method, then, just return false if one of API fails its work.  Then, the caller just initialize the text range itself with only one clause.

I believe that this is clearer code.

However, I don't mind the current patch's code. It's just my suggestion.

Comment 36

[KO Myung-Hun]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (Offline 6/11 - 6/17) from comment #35)
> Comment on attachment 759737 [details] [diff] [review]
> Support multiple clauses V3
> 
> Great! Thank you for your work!!
> 
> > > >   if (!compositionString.IsEmpty()) {
> > > >+    bool oneClause = false;
> > > 
> > > Hmm, I strongly recommend that you should separate this blog to a method,
> > > though. But it doesn't matter.
> > > 
> > 
> > Any special reasons ?
> 
> Because if you separate the block into another method, then, just return
> false if one of API fails its work.  Then, the caller just initialize the
> text range itself with only one clause.
> 
> I believe that this is clearer code.
> 
> However, I don't mind the current patch's code. It's just my suggestion.

Thanks for your very kind explanation. ^^

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8945a734ab7

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e8945a734ab7