Closed Bug 1077345 Opened 10 years ago Closed 9 years ago

Make compositionend event dispatchers of widget simpler

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: dev-doc-complete, inputmethod)

Attachments

(13 files, 8 obsolete files)

1.27 KB, patch
xyuan
: review+
Details | Diff | Splinter Review
4.71 KB, patch
jchen
: review+
Details | Diff | Splinter Review
2.20 KB, patch
emk
: review+
Details | Diff | Splinter Review
11.06 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
11.79 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
7.12 KB, patch
Details | Diff | Splinter Review
3.61 KB, patch
Details | Diff | Splinter Review
7.14 KB, patch
Details | Diff | Splinter Review
23.51 KB, patch
smaug
: review+
Details | Diff | Splinter Review
48.85 KB, patch
Details | Diff | Splinter Review
14.59 KB, patch
Details | Diff | Splinter Review
25.99 KB, patch
Details | Diff | Splinter Review
16.33 KB, patch
emk
: review+
Details | Diff | Splinter Review
Currently, when IME composition is committed, widget dispatches a text event (compositionchange event if bug 960871 is fixed) without clause ranges, first. Then, dispatches compositionend with same commit string (Although, Android's nsWindows::RemoveIMEComposition() doesn't so.).

First issue of this is, dispatching similar two events from widget is really redundant. With using only one event for this, we can reduce IPC cost.

Second issue of this is, like Android's nsWindow, this may cause a bug of data value of compositionend event.

Third issue of this is, each IME handler for every platform stores last dispatched composition string for setting data value of both text event and compositionend event. However, in XP level, TextComposition stores same string. Therefore, it wastes memory for storing same string for same purpose.

Fourth issue of this is, dispatching two events from widget makes the code ugly. I'd like to reduce such code from widget.


Therefore, my ideas to solve above problems are:

1. Create NS_COMPOSITION_COMMIT event.

This event should be handled by TextComposition. widget doesn't need to set data value at dispatching this event. Instead, TextComposition dispatches a text event and a compositionend event with stored data.

2. Create NS_COMPOSITION_COMMIT_WITH_DATA event.

This event should be handled by TextComposition. widget should set commit string which is specified by native IME event. Then, TextComposition dispatches a compositionupdate event, a text event and a compositionend event automatically.

I'll try to create patches for this bug next week.

If you have some idea, let me know.
I was thinking about to use only a compositionend event or a text event. And TextComposition will fire the other event. However, I feel that this approach may make other developers confused. Therefore, I think that creating new commit events only for widget makes the strategy simpler and clearer.
Let TextComposition check if a dispatched event causes destroying PresShell or something with a method.
Attachment #8521900 - Flags: review?(bugs)
Let's create a method which dispatches an event which is generated from a specified message and another composition event.
Attachment #8521901 - Flags: review?(bugs)
WidgetCompositionEvent should have utility methods which check which DOM event will be dispatched. This makes the callers clearer and easier to maintain.
Attachment #8521903 - Flags: review?(bugs)
Let's define NS_COMPOSITION_COMMIT_AS_IS message. The message means that composition will be committed with the last data. I don't have better idea of its name. NS_COMPOSITION_COMMIT_WITH_LAST_DATA is too long for our coding rules...
Attachment #8521904 - Flags: review?(bugs)
EventUtils.sendCompositionEvent() should support "compositioncommitasis". This makes a lot of tests simpler.

And let's test NS_COMPOSITION_COMMIT_AS_IS in window_composition_text_querycontent.xul.
Attachment #8521909 - Flags: review?(bugs)
Add NS_COMPOSITION_COMMIT message which will commit composition with WidgetCompositionEvent::mData.

By this change and NS_COMPOSITION_COMMIT_AS_IS, nsIDOMWindowUtils::SendCompositionEvent() doesn't need to support "compositionend". But I think that it shouldn't cause an exception (or should we keep supporting it for compatibility?).
Attachment #8521912 - Flags: review?(bugs)
EventUtils.synthesizeComposition() should support "compositioncommit" and tests should use it. It makes a lot of tests simpler. And also we should test the behavior of nsIDOMWindowUtils::SendCompositionEvent("compositioncommit") in window_composition_text_querycontent.xul.
Attachment #8521915 - Flags: review?(bugs)
FYI: These patches will reduce count of IPC at committing composition. This means that last compositionchange event for removing IME selection or modifying composition string and compositionend are dispatched continuously in a process. I.e., nobody can do anything between them.
Comment on attachment 8521900 [details] [diff] [review]
part.1 Add TextComposition::IsSafeToDispatchEvent() for checking if it's safe to dispatch an event to the DOM tree

Hmm, since this is about rather different thing than script blockers, perhaps 
the method should be called.
IsValidStateForComposition()

With that, r+
Attachment #8521900 - Flags: review?(bugs) → review+
Comment on attachment 8521901 [details] [diff] [review]
part.2 Add TextComposition::DispatchCompositionEventAs() for dispatching WidgetCompositionEvent only whose message is different from original composition event

>+  if (aMessage == NS_COMPOSITION_UPDATE) {
>+    mLastData = compositionEvent.mData;
>+  }
Could you explain this?
It is not obvious when reading just this patch.

>+   */
>+  void DispatchCompositionEventAs(
>+         const WidgetCompositionEvent* aCompositionEvent,
>+         uint32_t aMessage);
Could this be called
  void CloneAndDispatchAs(
         const WidgetCompositionEvent* aCompositionEvent,
         uint32_t aMessage);


With those, r+
Attachment #8521901 - Flags: review?(bugs) → review+
Comment on attachment 8521903 [details] [diff] [review]
part.3 Add WidgetCompositionEvent::CausesDOMTextEvent() and WidgetCompositionEvent::CausesDOMCompositionEndEvent()

Not sure this makes the code any easier to read, though perhaps not harder to read either.
Attachment #8521903 - Flags: review?(bugs) → review+
Comment on attachment 8521904 [details] [diff] [review]
part.4 Add NS_COMPOSITION_COMMIT_AS_IS event which automatically commits composition with the last data

I think I don't quite understand the setup here.


>       nsAutoString commitData(aDiscard ? EmptyString() : lastData);
>-      bool changingData = lastData != commitData;
>+      if (commitData == mLastData) {
>+        WidgetCompositionEvent commitEvent(true, NS_COMPOSITION_COMMIT_AS_IS,
>+                                           widget);
...

>   // Otherwise, synthesize the commit in content.
>   nsAutoString data(aDiscard ? EmptyString() : lastData);
>+  if (data == mLastData) {
>+    DispatchCompositionEventRunnable(NS_COMPOSITION_COMMIT_AS_IS, EmptyString(),
>+                                     true);
>+    return NS_OK;
>+  }

I'm now lost with the setup. I thought this would be coming from widget level.




> // composition events
> #define NS_COMPOSITION_EVENT_START    2200
> #define NS_COMPOSITION_START          (NS_COMPOSITION_EVENT_START)
>+// NS_COMPOSITION_END is the message for DOM compositionend event.
>+// This event should NOT be dispatched from widget if NS_COMPOSITION_COMMIT
>+// is available.
What does 'if NS_COMPOSITION_COMMIT is available' mean?
Comment on attachment 8521909 [details] [diff] [review]
part.5 Use synthesizeComposition({"compositioncommitasis") in the tests

I'm really missing something about the setup.
How can we drop all the synthesizeCompositionChange calls?
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #15)
> FYI: These patches will reduce count of IPC at committing composition. This
> means that last compositionchange event for removing IME selection or
> modifying composition string and compositionend are dispatched continuously
> in a process. I.e., nobody can do anything between them.
This sounds great!
(In reply to Olli Pettay [:smaug] from comment #19)
> Comment on attachment 8521904 [details] [diff] [review]
> part.4 Add NS_COMPOSITION_COMMIT_AS_IS event which automatically commits
> composition with the last data
> 
> >       nsAutoString commitData(aDiscard ? EmptyString() : lastData);
> >-      bool changingData = lastData != commitData;
> >+      if (commitData == mLastData) {
> >+        WidgetCompositionEvent commitEvent(true, NS_COMPOSITION_COMMIT_AS_IS,
> >+                                           widget);

This large |else| block handles commit request for synthesized composition (i.e., nsIDOMWindowUtils::SendCompositionEvent() caused the composition).

> >   // Otherwise, synthesize the commit in content.
> >   nsAutoString data(aDiscard ? EmptyString() : lastData);
> >+  if (data == mLastData) {
> >+    DispatchCompositionEventRunnable(NS_COMPOSITION_COMMIT_AS_IS, EmptyString(),
> >+                                     true);
> >+    return NS_OK;
> >+  }
> 
> I'm now lost with the setup. I thought this would be coming from widget
> level.

On the other hand, this may be occur for the |if| block, i.e., after requesting to commit the composition to native IME. If native IME commits the composition asynchronously (e.g., iBus on Linux), TextComposition needs to emulate synchronous commit for callers. Therefore, this emulates the widget's behavior with runnable event.

> > // composition events
> > #define NS_COMPOSITION_EVENT_START    2200
> > #define NS_COMPOSITION_START          (NS_COMPOSITION_EVENT_START)
> >+// NS_COMPOSITION_END is the message for DOM compositionend event.
> >+// This event should NOT be dispatched from widget if NS_COMPOSITION_COMMIT
> >+// is available.
> What does 'if NS_COMPOSITION_COMMIT is available' mean?

I'll add NS_COMPOSITION_COMMIT event in part.6. It commits composition with its mData value. So, if widget needs to commit composition with specific commit string, widget shouldn't dispatch NS_COMPOSITION_CHANGE before NS_COMPOSITION_COMMIT_AS_IS. I meant so, but indeed, it's unclear...
(In reply to Olli Pettay [:smaug] from comment #20)
> Comment on attachment 8521909 [details] [diff] [review]
> part.5 Use synthesizeComposition({"compositioncommitasis") in the tests
> 
> I'm really missing something about the setup.
> How can we drop all the synthesizeCompositionChange calls?

Removed synthesizeCompositionChange() calls are dispatching composition change event with same string as the previous synthesizeCompositionChange() call. But they have needed to call it only for removing IME selections from the composition string.

By the new feature, syntheizeComposition("compositioncommitasis"), removing IME selections is automatically performed before dispatching compositionend event. (line 233 in TextComposition.cpp of attachment 8521904 [details] [diff] [review]).

Therefore, some tests can stop calling redundant synthesizeCompositionChange() immediately before synthesizing compositionend event.
(In reply to Olli Pettay [:smaug] from comment #17)
> Comment on attachment 8521901 [details] [diff] [review]
> part.2 Add TextComposition::DispatchCompositionEventAs() for dispatching
> WidgetCompositionEvent only whose message is different from original
> composition event
> 
> >+  if (aMessage == NS_COMPOSITION_UPDATE) {
> >+    mLastData = compositionEvent.mData;
> >+  }
> Could you explain this?
> It is not obvious when reading just this patch.

compositionupdate is dispatched only when composition string is *actually* modified. (e.g., it shouldn't be fired when only IME selection is modified.) Therefore, TextComposition needs to store the last data when composition update is dispatched.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #24)
> (In reply to Olli Pettay [:smaug] from comment #17)
> > Comment on attachment 8521901 [details] [diff] [review]
> > part.2 Add TextComposition::DispatchCompositionEventAs() for dispatching
> > WidgetCompositionEvent only whose message is different from original
> > composition event
> > 
> > >+  if (aMessage == NS_COMPOSITION_UPDATE) {
> > >+    mLastData = compositionEvent.mData;
> > >+  }
> > Could you explain this?
> > It is not obvious when reading just this patch.
> 
> compositionupdate is dispatched only when composition string is *actually*
> modified. (e.g., it shouldn't be fired when only IME selection is modified.)
> Therefore, TextComposition needs to store the last data when composition
> update is dispatched.
But why we special case NS_COMPOSITION_UPDATE there.


I still don't quite understand the setup.
Widget level code is now supposed to dispatch which composition events?
And which events should TextComposition dispatch?
In other words, I think we need some documentation for this all in TextEvents.h
(In reply to Olli Pettay [:smaug] from comment #25)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #24)
> > (In reply to Olli Pettay [:smaug] from comment #17)
> > > Comment on attachment 8521901 [details] [diff] [review]
> > > part.2 Add TextComposition::DispatchCompositionEventAs() for dispatching
> > > WidgetCompositionEvent only whose message is different from original
> > > composition event
> > > 
> > > >+  if (aMessage == NS_COMPOSITION_UPDATE) {
> > > >+    mLastData = compositionEvent.mData;
> > > >+  }
> > > Could you explain this?
> > > It is not obvious when reading just this patch.
> > 
> > compositionupdate is dispatched only when composition string is *actually*
> > modified. (e.g., it shouldn't be fired when only IME selection is modified.)
> > Therefore, TextComposition needs to store the last data when composition
> > update is dispatched.
> But why we special case NS_COMPOSITION_UPDATE there.

Because it guarantees to update mLastData when NS_COMPOSITION_UPDATE is dispatched. Even if there would be other callers to dispatch NS_COMPOSITION_UPDATE, this would keep guaranteeing it.

> I still don't quite understand the setup.
> Widget level code is now supposed to dispatch which composition events?
> And which events should TextComposition dispatch?

NS_COMPOSITION_START, NS_COMPOSITION_CHANGE, NS_COMPOSITION_COMMIT_AS_IS, NS_COMPOSITION_COMMIT should be dispatched by widget.

NS_COMPOSITION_UPDATE and NS_COMPOSITION_END should be dispatched only by TextComposition.

In other words, only NS_COMPOSITION_START should be dispatched from widget and causes a standard DOM event directly.

(In reply to Olli Pettay [:smaug] from comment #26)
> In other words, I think we need some documentation for this all in
> TextEvents.h

I guess you meant BasicEvents.h (at defining NS_COMPOSITION_*).

I explained which event shouldn't be dispatched by widget. See:

https://bugzilla.mozilla.org/attachment.cgi?id=8521904&action=diff#a/widget/BasicEvents.h_sec2
https://bugzilla.mozilla.org/attachment.cgi?id=8521912&action=diff#a/widget/BasicEvents.h_sec2
But in the patches AS_IS is dispatched in TextComposition, yet you say only by the Widget.
(In reply to Olli Pettay [:smaug] from comment #29)
> But in the patches AS_IS is dispatched in TextComposition, yet you say only
> by the Widget.

TextComposition needs to emulate widget behavior in two cases:

1. RequestToCommit() is called when synthesized composition is there. In this case, instead of widget, TextComposition needs to dispatch NS_COMPOSITION_COMMIT_AS_IS or NS_COMPOSITION_COMMIT. Of course, it can dispatch NS_COMPOSITION_CHANGE and NS_COMPOSITION_END for it directly. But using same path which is used by widget is better for automated testing.

2. RequestToCommit() is called when native composition is there but native IME doesn't commit/cancel the composition synchronously. In this case, TextComposition needs to dispatch NS_COMPOSITION_COMMIT_AS_IS or NS_COMPOSITION_COMMIT for performing synchronous commit composition. This is necessary for some callers.

So, in above cases, TextComposition needs to behave a "fake" widget. Therefore, I've never said that they should be dispatched *only* by widget. I just said that they should be dispatched by widget.
Comment on attachment 8521904 [details] [diff] [review]
part.4 Add NS_COMPOSITION_COMMIT_AS_IS event which automatically commits composition with the last data


>+  if (dispatchEvent) {
>+    // If the composition event should cause a DOM text event, we should
>+    // overwrite the event message as NS_COMPOSITION_CHANGE because due to
>+    // the limitation of mapping between event messages and DOM event types,
>+    // we cannot map multiple event messages to a DOM event type.
>+    AutoRestore<uint32_t> saveEventMessage(aCompositionEvent->message);
>+    if (dispatchDOMTextEvent) {
>+      aCompositionEvent->message = NS_COMPOSITION_CHANGE;
>+    }
I don't like this. Something is passing aCompositionEvent to this method, and during the method call
the value of ->message changes. That is rather unexpected, and error prone.
Why not create a new event?

>+    case NS_COMPOSITION_COMMIT_AS_IS: {
>       WidgetCompositionEvent compEvent(true, mEventMessage, widget);
>-      compEvent.mData = mData;
>+      if (mEventMessage == NS_COMPOSITION_COMMIT_AS_IS) {
>+        compEvent.mData = mData;
>+      }

Hmm, so TextComposition::DispatchCompositionEvent expects that
+  if (aCompositionEvent->message == NS_COMPOSITION_COMMIT_AS_IS) {
...
+    NS_ASSERTION(aCompositionEvent->mData.IsEmpty(),

...
>       IMEStateManager::DispatchCompositionEvent(mEventTarget, presContext,
>                                                 &compEvent, &status, nullptr,
>                                                 mIsSynthesizedEvent);

....and IMEStateManager::DispatchCompositionEvent ends up calling TextComposition::DispatchCompositionEvent.
So I don't quite understand what is supposed to happen here. We assign possible value to compositionEvent.mData but that data should be always empty.
And TextComposition::DispatchCompositionEvent always sets compositionEvent.mData in case message == NS_COMPOSITION_COMMIT_AS_IS
Attachment #8521904 - Flags: review?(bugs) → review-
Comment on attachment 8521909 [details] [diff] [review]
part.5 Use synthesizeComposition({"compositioncommitasis") in the tests

"compoing string" ?
Attachment #8521909 - Flags: review?(bugs) → review+
Attachment #8521915 - Flags: review?(bugs) → review+
Comment on attachment 8521912 [details] [diff] [review]
part.6 Add NS_COMPOSITION event which automatically commits composition with its mData

>   /**
>    * Synthesize a composition event to the window. 
>    *
>    * Cannot be accessed from unprivileged context (not content-accessible)
>    * Will throw a DOM security error if called without chrome privileges.
>    *
>-   * @param aType     The event type: "compositionstart", "compositionend" or
>-   *                  "compositioncommitasis".
>+   * @param aType     The event type: "compositionstart",
>+   *                  "compositioncommitasis", or "compositioncommit".
We don't support compositionchange or some such here anymore?
Shouldn't we. How else do you test test all the steps during composition?
But not really a bug in this patch.
Attachment #8521912 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #33)
> We don't support compositionchange or some such here anymore?
Ah, forgot nsICompositionStringSynthesizer
(In reply to Olli Pettay [:smaug] from comment #31)
> Comment on attachment 8521904 [details] [diff] [review]
> part.4 Add NS_COMPOSITION_COMMIT_AS_IS event which automatically commits
> composition with the last data
> >       IMEStateManager::DispatchCompositionEvent(mEventTarget, presContext,
> >                                                 &compEvent, &status, nullptr,
> >                                                 mIsSynthesizedEvent);
> 
> ....and IMEStateManager::DispatchCompositionEvent ends up calling
> TextComposition::DispatchCompositionEvent.
> So I don't quite understand what is supposed to happen here. We assign
> possible value to compositionEvent.mData but that data should be always
> empty.
> And TextComposition::DispatchCompositionEvent always sets
> compositionEvent.mData in case message == NS_COMPOSITION_COMMIT_AS_IS

Oh, that's a bug. It should be !=. I have no idea to test this case without expanding our composition synthesizer supports async commit.
Comment on attachment 8524350 [details] [diff] [review]
part.3 Add WidgetCompositionEvent::CausesDOMTextEvent() and WidgetCompositionEvent::CausesDOMCompositionEndEvent() (r=smaug)

Oops, this already has r+.
Attachment #8524350 - Attachment description: part.3 Add WidgetCompositionEvent::CausesDOMTextEvent() and WidgetCompositionEvent::CausesDOMCompositionEndEvent() → part.3 Add WidgetCompositionEvent::CausesDOMTextEvent() and WidgetCompositionEvent::CausesDOMCompositionEndEvent() (r=smaug)
Attachment #8524350 - Flags: review?(bugs)
Okay, then, we need to add arguments and result of CloneAndDispatchAs().
Attachment #8521904 - Attachment is obsolete: true
Attachment #8524352 - Flags: review?(bugs)
Comment on attachment 8521917 [details] [diff] [review]
part.8 CompositionManager.endComposition() in forms.js should use nsIDOMWindowUtils.sendCompositionEvent('compositioncommit')

B2G can use "compositioncommit" at committing composition with specified string. (Can skip to use CompositionStringSynthesizer() for removing IME selections from composition string)
Attachment #8521917 - Flags: review?(xyuan)
Comment on attachment 8521918 [details] [diff] [review]
part.9 nsWindow for Android should use NS_COMPOSITION_COMMIT* event

Now, widget shouldn't dispatch NS_COMPOSITION_END and NS_COMPOSITION_CHANGE without WidgetCompositionEvent::mRanges.

Instead, NS_COMPOSITION_COMMIT_AS_IS and NS_COMPOSITION_COMMIT are available. The former causes committing composition with the last dispatched composition string. The latter is committed with specified string (WidgetCompositionEvent::mData).
Attachment #8521918 - Flags: review?(nchen)
Comment on attachment 8521919 [details] [diff] [review]
part.10 nsTextStore should use NS_COMPOSITION_COMMIT event

Now, widget shouldn't dispatch NS_COMPOSITION_END and NS_COMPOSITION_CHANGE without WidgetCompositionEvent::mRanges.

Instead, NS_COMPOSITION_COMMIT_AS_IS and NS_COMPOSITION_COMMIT are available. The former causes committing composition with the last dispatched composition string. The latter is committed with specified string (WidgetCompositionEvent::mData).
Attachment #8521919 - Flags: review?(VYV03354)
Comment on attachment 8521920 [details] [diff] [review]
part.11 nsIMM32Handler should use NS_COMPOSITION_COMMIT event and NS_COMPOSITION_COMMIT_AS_IS event

ditto.

Additionally, we can get rid of mLastDispatchedCompositionString by this change.
Attachment #8521920 - Flags: review?(VYV03354)
Comment on attachment 8521923 [details] [diff] [review]
part.12 IMEInputHandler of Cocoa should use NS_COMPOSITION_COMMIT* event

Now, widget shouldn't dispatch NS_COMPOSITION_END and NS_COMPOSITION_CHANGE without WidgetCompositionEvent::mRanges.

Instead, NS_COMPOSITION_COMMIT_AS_IS and NS_COMPOSITION_COMMIT are available. The former causes committing composition with the last dispatched composition string. The latter is committed with specified string (WidgetCompositionEvent::mData).

Unfortunately, even with this patch, we still need mLastDispatchedCompositionString.
Attachment #8521923 - Flags: review?(smichaud)
Comment on attachment 8521925 [details] [diff] [review]
part.13 nsGtkIMModule should use NS_COMPOSITION_COMMIT* event

Now, widget shouldn't dispatch NS_COMPOSITION_END and NS_COMPOSITION_CHANGE without WidgetCompositionEvent::mRanges.

Instead, NS_COMPOSITION_COMMIT_AS_IS and NS_COMPOSITION_COMMIT are available. The former causes committing composition with the last dispatched composition string. The latter is committed with specified string (WidgetCompositionEvent::mData).
Attachment #8521925 - Flags: review?(m_kato)
Attachment #8521917 - Flags: review?(xyuan) → review+
Attachment #8524352 - Flags: review?(bugs) → review+
Attachment #8521919 - Flags: review?(VYV03354) → review+
Comment on attachment 8521920 [details] [diff] [review]
part.11 nsIMM32Handler should use NS_COMPOSITION_COMMIT event and NS_COMPOSITION_COMMIT_AS_IS event

Review of attachment 8521920 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsIMM32Handler.cpp
@@ +1011,5 @@
>      PR_LOG(gIMM32Log, PR_LOG_ALWAYS,
>        ("IMM32: HandleComposition, GCS_RESULTSTR\n"));
>  
>      DispatchCompositionChangeEvent(aWindow, aIMEContext, false);
>      HandleEndComposition(aWindow);

Why nsIMM32Handler dispatches NS_COMPOSITION_CHANGE without mRange here?  If aCheckAttr is false, DispatchCompositionChangeEvent will not set mRange.  Why does DispatchCompositionChangeEvent still have the aCheckAttr argument in the first place?

Moreover, widget shouldn't dispatch NS_COMPOSITION_CHANGE before NS_COMPOSITION_COMMIT_AS_IS according to your comment #22.  But this code sequence is exactly doing that.
Comment on attachment 8521923 [details] [diff] [review]
part.12 IMEInputHandler of Cocoa should use NS_COMPOSITION_COMMIT* event

I won't be able to get to this until later this week, or possibly next week.

I've got a very large and very risky patch at bug 1092630 that I urgently need to test and review.
Comment on attachment 8521918 [details] [diff] [review]
part.9 nsWindow for Android should use NS_COMPOSITION_COMMIT* event

Review of attachment 8521918 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!
Attachment #8521918 - Flags: review?(nchen) → review+
(In reply to Masatoshi Kimura [:emk] from comment #50)
> Why nsIMM32Handler dispatches NS_COMPOSITION_CHANGE without mRange here?  If
> aCheckAttr is false, DispatchCompositionChangeEvent will not set mRange. 
> Why does DispatchCompositionChangeEvent still have the aCheckAttr argument
> in the first place?

Indeed, I'll check it tomorrow. (I'm on vacation today) Thank you for your review.

(In reply to Steven Michaud from comment #51)
> Comment on attachment 8521923 [details] [diff] [review]
> part.12 IMEInputHandler of Cocoa should use NS_COMPOSITION_COMMIT* event
> 
> I won't be able to get to this until later this week, or possibly next week.
> 
> I've got a very large and very risky patch at bug 1092630 that I urgently
> need to test and review.

No problem. I'm writing big patches in another bug, so, the delay isn't problem.
Attachment #8521925 - Flags: review?(m_kato) → review+
Comment on attachment 8525922 [details] [diff] [review]
part.11 nsIMM32Handler should use NS_COMPOSITION_COMMIT event and NS_COMPOSITION_COMMIT_AS_IS event

LGTM
Attachment #8525922 - Flags: review?(VYV03354) → review+
Comment on attachment 8521923 [details] [diff] [review]
part.12 IMEInputHandler of Cocoa should use NS_COMPOSITION_COMMIT* event

This looks fine to me.

I do have a couple of nits about one of your comments, though:

+  /**
+   * DispatchCompositionCommitEvent() dispatches a compositiocommit event or
+   * compositioncommitasis event.  If aCommitString is null, dispatches
+   * compositioncommitasis event.  I.e., if aCompositionString is null, this
+   * commits the composition with the last data.  Otherwise, commits the
+   * composition with aCommitString value.
+   */

compositiocommit -> compositioncommit

if aCompositionString is null -> if aCommitString is null
Attachment #8521923 - Flags: review?(smichaud) → review+
Depends on: 1243657
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.