OS/2 IME level3 support

RESOLVED FIXED in mozilla24

Status

Core Graveyard
Widget: OS/2
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: KO Myung-Hun, Assigned: KO Myung-Hun)

Tracking

({inputmethod, intl})

10 Branch
mozilla24
x86
OS/2
inputmethod, intl
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 6 obsolete attachments)

(Assignee)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
This is a tracking bug to implement OS/2 IME level3 support.
(Assignee)

Updated

5 years ago
Hardware: Other → x86
(Assignee)

Comment 2

5 years ago
Created attachment 636968 [details] [diff] [review]
First patch
(Assignee)

Comment 3

5 years ago
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.

Updated

5 years ago
Assignee: nobody → mozilla
Status: UNCONFIRMED → NEW
Component: Untriaged → Widget: OS/2
Ever confirmed: true
Keywords: inputmethod
Product: Firefox → Core
QA Contact: untriaged → os2
(Assignee)

Comment 4

5 years ago
Created attachment 637458 [details] [diff] [review]
Second patch

Fix a focus problem
Attachment #636968 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #637458 - Flags: review?(daveryeo)

Updated

5 years ago
Depends on: 684487
Keywords: intl

Updated

5 years ago
Assignee: mozilla → komh

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
Created attachment 638096 [details] [diff] [review]
Patch against trunk
Attachment #638096 - Flags: review?(daveryeo)

Updated

5 years ago
Attachment #638096 - Flags: review?(daveryeo) → review+

Updated

5 years ago
Keywords: checkin-needed
Attachment #637458 - Attachment is obsolete: true
Attachment #637458 - Flags: review?(daveryeo)
https://hg.mozilla.org/integration/mozilla-inbound/rev/78353003288e
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/78353003288e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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.
(Assignee)

Comment 10

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

And where can I get the documents for these ?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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 :-(
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
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
(Assignee)

Comment 14

5 years ago
Created attachment 641437 [details] [diff] [review]
Patch to fix a compositionupdate event problem
Attachment #641437 - Flags: review?(masayuki)
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?
Er, hg qdiff -U 8 -p if you're using mq.
(Assignee)

Comment 17

5 years ago
Created attachment 641688 [details] [diff] [review]
Patch reformatted with -U 8 -p
Attachment #641437 - Attachment is obsolete: true
Attachment #641437 - Flags: review?(masayuki)
Attachment #641688 - Flags: review?(masayuki)
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.
(Assignee)

Comment 19

5 years ago
(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.
(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.
(Assignee)

Comment 21

5 years ago
(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. ^^
(Assignee)

Comment 22

5 years ago
Created attachment 641770 [details] [diff] [review]
v3 for compositionupdate
Attachment #641688 - Attachment is obsolete: true
Attachment #641688 - Flags: review?(masayuki)
Attachment #641770 - Flags: review?(masayuki)
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!
Attachment #641770 - Flags: review?(masayuki) → review+
(Assignee)

Comment 24

5 years ago
(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. ^_____^
(Assignee)

Comment 25

5 years ago
Created attachment 641831 [details] [diff] [review]
v4 for compositionupdate
(Assignee)

Updated

5 years ago
Attachment #641831 - Flags: review?(masayuki)
Comment on attachment 641831 [details] [diff] [review]
v4 for compositionupdate

Sorry for the delay, I'll land this soon.
Attachment #641831 - Flags: review?(masayuki) → review+
https://hg.mozilla.org/mozilla-central/rev/2b1f30074100

Thank you, again!
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 28

4 years ago
Created attachment 757391 [details] [diff] [review]
Suppoert multiple clauses

This patch fixes the the multiple clauses problem on OS/2.
Attachment #757391 - Flags: review?(masayuki)
(Assignee)

Updated

4 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
(Assignee)

Comment 30

4 years ago
(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. ^^
(Assignee)

Comment 31

4 years ago
Created attachment 758538 [details] [diff] [review]
Support multiple clauses V2
Attachment #757391 - Attachment is obsolete: true
Attachment #757391 - Flags: review?(masayuki)
Attachment #758538 - Flags: review?(masayuki)
Comment on attachment 758538 [details] [diff] [review]
Support multiple clauses V2

># HG changeset patch
># Parent 318c97f3ed633baa18e2f04c45496c101d35a500
># User KO Myung-Hun <komh@chollian.net>
>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.
(Assignee)

Comment 33

4 years ago
(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 <komh@chollian.net>
> >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.
(Assignee)

Comment 34

4 years ago
Created attachment 759737 [details] [diff] [review]
Support multiple clauses V3
Attachment #758538 - Attachment is obsolete: true
Attachment #758538 - Flags: review?(masayuki)
Attachment #759737 - Flags: review?(masayuki)
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.
Attachment #759737 - Flags: review?(masayuki) → review+
(Assignee)

Comment 36

4 years ago
(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. ^^
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8945a734ab7
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e8945a734ab7
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: mozilla16 → mozilla24
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.