Make nsEditor store composition with TextComposition

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Blocks 1 bug, {inputmethod})

Trunk
mozilla30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 12 obsolete attachments)

1.59 KB, patch
smaug
: review+
Details | Diff | Splinter Review
12.98 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.70 KB, patch
Details | Diff | Splinter Review
8.09 KB, patch
smaug
: review+
Details | Diff | Splinter Review
17.30 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
14.90 KB, patch
Details | Diff | Splinter Review
4.84 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Currently, nsEditor manages composition itself. However, now, there is TextComposition class. Let's make nsEditor use it.
While there is a composition, nsEditor should hold mozilla::TextComposition which is alive during compositionstart - compositionend.
Attachment #8367756 - Flags: review?(ehsan)
mInIMEMode is true during compositionstart - compositionend.

I.e., this equals to !!mComposition. Therefore, we can remove it.
Attachment #8367758 - Flags: review?(ehsan)
nsEditor::mIsIMEComposing is true while the editor has uncommitted composition string.

We can move it to mozilla::TextComposition with a new method EditorWillHandleTextEvent().
Attachment #8367760 - Flags: review?(ehsan)
Attachment #8367760 - Flags: review?(bugs)
Let's move composition string which is in the focused editor to TextComposition. This allows others (e.g., nsContentEventHandler) can refer composition string easier.

The new MOZ_ASSERT() finds a bug in the test :-(
Attachment #8367761 - Flags: review?(ehsan)
Attachment #8367761 - Flags: review?(bugs)
I realized that creating composition events from JS chrome causes making editor confused because the events become trusted events (http://mxr.mozilla.org/mozilla-central/source/dom/events/nsEventDispatcher.cpp#657).

nsEditor assumes that unexpected composition events never come. I.e., current code can be confused by the created composition events from Chrome JS.

Additionally, such composition events don't hold WidgetGUIEvent::widget properly. Therefore, the event cannot be related to any mozilla::TextComposition instance. I.e., this causes crash if compositionstart event is dispatched from Chrome JS. Actually, widget/tests/test_composition_text_querycontent.xul meets crash.

Therefore, nsEditor shouldn't accept/handle composition events whose widget is nullptr.

FYI: nsEditor doesn't handle "compositionupdate" now. Therefore, the if condition doesn't check "compositionupdate".
Attachment #8367764 - Flags: review?(bugs)
When nsEditor instance is destroyed, the composition string isn't committed in the editor. After that, compositionupdate, text, compositionend events are dispatched by nsIMEStateManager. This causes a crash by a new MOZ_ASSERT() in TextComposition::DispatchEvent() (https://bugzilla.mozilla.org/attachment.cgi?id=8367760&action=diff#a/dom/events/TextComposition.cpp_sec3).

For keeping composition string status in TextComposition, we need to emulate the editor behavior for TextComposition itself.
Attachment #8367765 - Flags: review?(ehsan)
Attachment #8367765 - Flags: review?(bugs)
Comment on attachment 8367756 [details] [diff] [review]
part.1 nsEditor should store a pointer of TextComposition while composition

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

::: editor/libeditor/base/nsEditor.cpp
@@ +2014,5 @@
>  
>  nsresult
> +nsEditor::BeginIMEComposition(WidgetCompositionEvent* aCompositionEvent)
> +{
> +  mComposition = nsIMEStateManager::GetTextCompositionFor(aCompositionEvent);

Please MOZ_ASSERT that mComposition is null before this line.

@@ +2016,5 @@
> +nsEditor::BeginIMEComposition(WidgetCompositionEvent* aCompositionEvent)
> +{
> +  mComposition = nsIMEStateManager::GetTextCompositionFor(aCompositionEvent);
> +  MOZ_ASSERT(mComposition,
> +             "nsIMEStateManager doesn't return proper composition");

Looking at the implementation of this function, it can indeed return null, so I don't think this assertion is correct.  We need to handle the failure some other way.
Attachment #8367756 - Flags: review?(ehsan) → review-
Comment on attachment 8367758 [details] [diff] [review]
part.2 Remove nsEditor::mInIMEMode

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

r=me assuming that you somehow ensure that mComposition is not null in the first part.
Attachment #8367758 - Flags: review?(ehsan) → review+
Comment on attachment 8367760 [details] [diff] [review]
part.3 Remove nsEditor::mIsIMEComposing

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

::: editor/libeditor/text/nsPlaintextEditor.cpp
@@ -833,2 @@
>  {
> -  NS_ABORT_IF_FALSE(aTextRangeList, "aTextRangeList must not be nullptr");

Can't we keep this for aTextEvent, given the fact that we don't null check it before dereferencing?
Attachment #8367760 - Flags: review?(ehsan) → review+
Comment on attachment 8367761 [details] [diff] [review]
part.4 Remove nsEditor::mIMEBufferLength

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

::: editor/libeditor/base/nsEditor.cpp
@@ +245,5 @@
>  
>    /* initialize IME stuff */
>    mIMETextNode = nullptr;
>    mIMETextOffset = 0;
> +

Nit: please drop this line.
Attachment #8367761 - Flags: review?(ehsan) → review+
Comment on attachment 8367765 [details] [diff] [review]
part.6 Emulate editor behavior in mozilla::TextComposition after focused editor is destroyed during composition

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

This feels really dirty to me at the least.  But I also think that it's wrong.  What if for example a text control gets reframed when it receives a compositionstart event?  Would we just not drop the rest of the events here?

At any rate, this needs a bunch of tests examining the cases where the editor gets destroyed and the cases where the editor gets recreated as a result of a reframe.
Attachment #8367765 - Flags: review?(ehsan) → review-
(In reply to :Ehsan Akhgari (needinfo? me!) (@ work week, reduced responsiveness) from comment #8)
> @@ +2016,5 @@
> > +nsEditor::BeginIMEComposition(WidgetCompositionEvent* aCompositionEvent)
> > +{
> > +  mComposition = nsIMEStateManager::GetTextCompositionFor(aCompositionEvent);
> > +  MOZ_ASSERT(mComposition,
> > +             "nsIMEStateManager doesn't return proper composition");
> 
> Looking at the implementation of this function, it can indeed return null,
> so I don't think this assertion is correct.  We need to handle the failure
> some other way.

Hmm, it must be safe except the case of part.5 patch.

Because when PresShell tries to dispatch a composition or text event, it uses nsIMEStateManager::DispatchCompositionEvent(). It ensures to exist an instance for the widget of the event.

However, indeed, this is a little bit risky for other developers who are not familiar with around there. This patch should be safer approach. This don't handle composition if there is no proper TextComposition instance.

I'll request review after testing all patches but welcome the feedback before that.
(In reply to :Ehsan Akhgari (needinfo? me!) (@ work week, reduced responsiveness) from comment #12)
> At any rate, this needs a bunch of tests examining the cases where the
> editor gets destroyed and the cases where the editor gets recreated as a
> result of a reframe.

Indeed, however, it sounds like that it's really another bug because this bug's goal doesn't change any behavior actually. This is preparation of other bugs.
This makes nsEditor register itself to TextComposition. TextComposition holds a handling editor with weak reference. By this, TextComposition can emulate editor's behavior only while any editor doesn't handle text events.
Attachment #8367765 - Attachment is obsolete: true
Attachment #8367769 - Attachment is obsolete: true
Attachment #8367765 - Flags: review?(bugs)
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d1543c1f6f7d

If we can access nsEditor directly from TextComposition, we might make composition event handling simpler. But I don't have concrete idea.
Ehsan:

Do you have idea to test reframing cases? If you have, I'll file a bug for researching it. Anyway, we should have such automated tests.
Comment on attachment 8368389 [details] [diff] [review]
part.1 nsEditor should store a pointer of TextComposition while composition

See comment 13 for the detail.
Attachment #8368389 - Flags: review?(ehsan)
Comment on attachment 8368405 [details] [diff] [review]
part.2 Remove nsEditor::mInIMEMode

This has minor change for EnsureComposition().
Attachment #8368405 - Flags: review?(ehsan)
Comment on attachment 8368408 [details] [diff] [review]
part.6 mozilla::TextComposition should holds a weak reference to composition handling editor during composition

See comment 18 for the detail of new approach.
Attachment #8368408 - Flags: review?(ehsan)
Attachment #8368408 - Flags: review?(bugs)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #20)
> Ehsan:
> 
> Do you have idea to test reframing cases? If you have, I'll file a bug for
> researching it. Anyway, we should have such automated tests.

I'm not super confident on my knowledge of IME events, but here is the basic idea:

<input type=text oncompositionstart="this.style.display='none';this.style.display='';">

Or:

<input type=text oncompositionstart="this.style.display='none';this.style.display='';document.body.clientWidth;">

The first case makes sure that the input element is reframed when a compositionstart event is dispatched.  The reframe will happen the next time that someone flushes layout though.  If that is not soon enough, you can use the idea in the second test case by accessing something such as clientWidth which would flush layout forcefully.  This ensures that the layout is properly flushed before the event handler returns.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #14)
> (In reply to :Ehsan Akhgari (needinfo? me!) (@ work week, reduced
> responsiveness) from comment #12)
> > At any rate, this needs a bunch of tests examining the cases where the
> > editor gets destroyed and the cases where the editor gets recreated as a
> > result of a reframe.
> 
> Indeed, however, it sounds like that it's really another bug because this
> bug's goal doesn't change any behavior actually. This is preparation of
> other bugs.

Sure, we can add the tests in any bug, but I would feel safer if you at least tested that stuff explicitly locally before landing this.
Comment on attachment 8368389 [details] [diff] [review]
part.1 nsEditor should store a pointer of TextComposition while composition

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

r=me with the below addressed.

::: dom/events/nsIMEStateManager.cpp
@@ +1140,5 @@
> +{
> +  MOZ_ASSERT(aEvent->AsCompositionEvent() || aEvent->AsTextEvent(),
> +             "aEvent has to be WidgetCompositionEvent or WidgetTextEvent");
> +  return sTextCompositions ?
> +    sTextCompositions->GetCompositionFor(aEvent->widget) : nullptr;

OK, so what you said in comment 13 is convincing, however that doesn't ensure that this code path will not return null in the future.  So, how about calling EnsureTextCompositionArray() both here and in the other overload of this function above, and just remove the null checking stuff?  That would make it clear at compile time that these two methods will never return null.
Attachment #8368389 - Flags: review?(ehsan) → review+
Attachment #8368405 - Flags: review?(ehsan) → review+
Comment on attachment 8368408 [details] [diff] [review]
part.6 mozilla::TextComposition should holds a weak reference to composition handling editor during composition

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

This seems better to me, but Olli should have the final say over the approach here.

::: dom/events/TextComposition.cpp
@@ +164,5 @@
> +
> +void
> +TextComposition::EndHandlingComposition(nsIEditor* aEditor)
> +{
> +  if (GetEditor() != aEditor) {

How can this ever happen?  I think you should assert that this doesn't happen instead.

@@ +174,5 @@
> +nsIEditor*
> +TextComposition::GetEditor() const
> +{
> +  nsCOMPtr<nsIEditor> editor = do_QueryReferent(mEditorWeak);
> +  return editor;

This code is wrong: you grab a strong ref to the editor, then the nsCOMPtr's destructor releases that reference and you return a raw pointer to it.  Thus if the caller forgets to addref it, you may end up accessing a freed pointer.  Please return an already_AddRefed<nsIEditor> instead.

But on the other hand, you never actally use the returned pointer.  If you don't plan to use it in the future either, then you can convert this method to bool HasEditor() and just |return !!editor;|.
Attachment #8368408 - Flags: review?(ehsan) → review+
Thank you, Ehsan!

(In reply to :Ehsan Akhgari (needinfo? me!) (@ work week, reduced responsiveness) from comment #26)
> Comment on attachment 8368389 [details] [diff] [review]
> part.1 nsEditor should store a pointer of TextComposition while composition
> > +  MOZ_ASSERT(aEvent->AsCompositionEvent() || aEvent->AsTextEvent(),
> > +             "aEvent has to be WidgetCompositionEvent or WidgetTextEvent");
> > +  return sTextCompositions ?
> > +    sTextCompositions->GetCompositionFor(aEvent->widget) : nullptr;
> 
> OK, so what you said in comment 13 is convincing, however that doesn't
> ensure that this code path will not return null in the future.  So, how
> about calling EnsureTextCompositionArray() both here and in the other
> overload of this function above, and just remove the null checking stuff? 
> That would make it clear at compile time that these two methods will never
> return null.

Hmm, okay. But it may cause crash if aEvent->widget is nullptr because widget is important key of the class. So, if passing nullprt to the widget, we should call MOZ_CRASH().

However, as the part.5, nsEditor shouldn't handle composition/text events whose widget is nullptr. So, if the crash occurred, it should be a bug of part.5 code or somebody would break it. Anyway, it's very rare case.

(In reply to :Ehsan Akhgari (needinfo? me!) (@ work week, reduced responsiveness) from comment #27)
> Comment on attachment 8368408 [details] [diff] [review]
> part.6 mozilla::TextComposition should holds a weak reference to composition
> handling editor during composition
> @@ +164,5 @@
> > +
> > +void
> > +TextComposition::EndHandlingComposition(nsIEditor* aEditor)
> > +{
> > +  if (GetEditor() != aEditor) {
> 
> How can this ever happen?  I think you should assert that this doesn't
> happen instead.

There are two scenarios:

* When nsEditor::IsAcceptableInputEvent() has a bug and two or more editor tries to handle same composition.

* When destroying editor occurs after new editor receives a composition/text event.

The former is really a bug. However, I don't familiar with the latter case. Does it never occurs? Then "assert" you mentioned is MOZ_ASSERT()? or NS_ASSERTION()?

> @@ +174,5 @@
> > +nsIEditor*
> > +TextComposition::GetEditor() const
> > +{
> > +  nsCOMPtr<nsIEditor> editor = do_QueryReferent(mEditorWeak);
> > +  return editor;
> 
> This code is wrong: you grab a strong ref to the editor, then the nsCOMPtr's
> destructor releases that reference and you return a raw pointer to it.  Thus
> if the caller forgets to addref it, you may end up accessing a freed
> pointer.  Please return an already_AddRefed<nsIEditor> instead.

Is it possible? In my understanding, when nsWeakPtr has a valid pointer, there is at least one owner of the instance. If the caller may use the editor's method or other complex class such as PresShell's method, it must receive the result with nsCOMPtr. However, I understand your concern. I'll change it.
Flags: needinfo?(ehsan)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #28)
> Thank you, Ehsan!
> 
> (In reply to :Ehsan Akhgari (needinfo? me!) (@ work week, reduced
> responsiveness) from comment #26)
> > Comment on attachment 8368389 [details] [diff] [review]
> > part.1 nsEditor should store a pointer of TextComposition while composition
> > > +  MOZ_ASSERT(aEvent->AsCompositionEvent() || aEvent->AsTextEvent(),
> > > +             "aEvent has to be WidgetCompositionEvent or WidgetTextEvent");
> > > +  return sTextCompositions ?
> > > +    sTextCompositions->GetCompositionFor(aEvent->widget) : nullptr;
> > 
> > OK, so what you said in comment 13 is convincing, however that doesn't
> > ensure that this code path will not return null in the future.  So, how
> > about calling EnsureTextCompositionArray() both here and in the other
> > overload of this function above, and just remove the null checking stuff? 
> > That would make it clear at compile time that these two methods will never
> > return null.
> 
> Hmm, okay. But it may cause crash if aEvent->widget is nullptr because
> widget is important key of the class. So, if passing nullprt to the widget,
> we should call MOZ_CRASH().
> 
> However, as the part.5, nsEditor shouldn't handle composition/text events
> whose widget is nullptr. So, if the crash occurred, it should be a bug of
> part.5 code or somebody would break it. Anyway, it's very rare case.

In that case we should probably abort when that happens.

> (In reply to :Ehsan Akhgari (needinfo? me!) (@ work week, reduced
> responsiveness) from comment #27)
> > Comment on attachment 8368408 [details] [diff] [review]
> > part.6 mozilla::TextComposition should holds a weak reference to composition
> > handling editor during composition
> > @@ +164,5 @@
> > > +
> > > +void
> > > +TextComposition::EndHandlingComposition(nsIEditor* aEditor)
> > > +{
> > > +  if (GetEditor() != aEditor) {
> > 
> > How can this ever happen?  I think you should assert that this doesn't
> > happen instead.
> 
> There are two scenarios:
> 
> * When nsEditor::IsAcceptableInputEvent() has a bug and two or more editor
> tries to handle same composition.
> 
> * When destroying editor occurs after new editor receives a composition/text
> event.

This should not happen, we should eagerly recreate the editor whereever it's needed.  If it's happening in your tests, you've hit a bug in the lazy editor initialization code.

> The former is really a bug. However, I don't familiar with the latter case.
> Does it never occurs? Then "assert" you mentioned is MOZ_ASSERT()? or
> NS_ASSERTION()?

Based on the above, I think we should MOZ_ASSERT this.

> > @@ +174,5 @@
> > > +nsIEditor*
> > > +TextComposition::GetEditor() const
> > > +{
> > > +  nsCOMPtr<nsIEditor> editor = do_QueryReferent(mEditorWeak);
> > > +  return editor;
> > 
> > This code is wrong: you grab a strong ref to the editor, then the nsCOMPtr's
> > destructor releases that reference and you return a raw pointer to it.  Thus
> > if the caller forgets to addref it, you may end up accessing a freed
> > pointer.  Please return an already_AddRefed<nsIEditor> instead.
> 
> Is it possible? In my understanding, when nsWeakPtr has a valid pointer,
> there is at least one owner of the instance. If the caller may use the
> editor's method or other complex class such as PresShell's method, it must
> receive the result with nsCOMPtr. However, I understand your concern. I'll
> change it.

This issue has nothing to do with weak pointers.  See below for a demo of the problem I'm talking about:

nsIEditor* GetEditor() { // return a non-addref'ed pointer }

void foo() {
  nsIEditor* editor = GetEditor();
  CallSomethingWhichCanRunJS(); // or arbitrary gecko code which might cause the editor object to be destroyed
  // at this point, the |editor| pointer is pointing to an object which was freed
  editor->DoSomething(); // sec-critical bug because of use-after-free
}
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #29)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #28)
> > Thank you, Ehsan!
> > 
> > (In reply to :Ehsan Akhgari (needinfo? me!) (@ work week, reduced
> > responsiveness) from comment #26)
> > > Comment on attachment 8368389 [details] [diff] [review]
> > > part.1 nsEditor should store a pointer of TextComposition while composition
> > > > +  MOZ_ASSERT(aEvent->AsCompositionEvent() || aEvent->AsTextEvent(),
> > > > +             "aEvent has to be WidgetCompositionEvent or WidgetTextEvent");
> > > > +  return sTextCompositions ?
> > > > +    sTextCompositions->GetCompositionFor(aEvent->widget) : nullptr;
> > > 
> > > OK, so what you said in comment 13 is convincing, however that doesn't
> > > ensure that this code path will not return null in the future.  So, how
> > > about calling EnsureTextCompositionArray() both here and in the other
> > > overload of this function above, and just remove the null checking stuff? 
> > > That would make it clear at compile time that these two methods will never
> > > return null.
> > 
> > Hmm, okay. But it may cause crash if aEvent->widget is nullptr because
> > widget is important key of the class. So, if passing nullprt to the widget,
> > we should call MOZ_CRASH().
> > 
> > However, as the part.5, nsEditor shouldn't handle composition/text events
> > whose widget is nullptr. So, if the crash occurred, it should be a bug of
> > part.5 code or somebody would break it. Anyway, it's very rare case.
> 
> In that case we should probably abort when that happens.

Hmm, I'm trying to create nsIMEStateManager::TextCompositionInstanceFor() which never returns nullptr. However, it needs to take nsPresContext and the event target node which are used for creating TextComposition instance. This fact makes new patch complicated. Since we need to pass non-null PresContext of the editor, get current event target from PresShell (for same logic at creating the instance in normal pass) and check if the given event is actually being dispatched. And each unexpected case needs to crash.

So, comparing null check in editor, your approach needs very messy code in nsIMEStateManager. Do you still think that we should do so?

# I misunderstand your comment. EnsureTextCompositionArray() doesn't create TextComposition instance. It just ensures the array of managing all TextComposition instances.
http://mxr.mozilla.org/mozilla-central/source/dom/events/nsIMEStateManager.cpp#534
Actually, it needs a lot of arguments to create TextComposition instance:
http://mxr.mozilla.org/mozilla-central/source/dom/events/nsIMEStateManager.cpp#563
Flags: needinfo?(ehsan)
This patch really ensures to store mComposition (if it fails, it crashes).
Attachment #8368389 - Attachment is obsolete: true
See comment 30. I think that this is the simplest approach. Creating TextComposition instance from nsEditor is difficult especially computing event target which is same result of PresShell. So, at least for now, we should allow to crash in EnsureComposition().
Attachment #8369941 - Attachment is obsolete: true
Attachment #8370000 - Flags: review?(ehsan)
Flags: needinfo?(ehsan)
Comment on attachment 8370000 [details] [diff] [review]
part.1 nsEditor should store a pointer of TextComposition while composition

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

OK, I guess we'll know soon if this happens in the wild!
Attachment #8370000 - Flags: review?(ehsan) → review+
Smaug:

Could you review this if you're available? This bug blocks some bugs which I'm working on.
Sorry about delay. Will review tomorrow.
Would it be possible to have a patch which contains all the changes. Easier to see the big picture that way.
Or I just review these patches now.
Attachment #8368406 - Flags: review?(bugs) → review+
Comment on attachment 8368407 [details] [diff] [review]
part.4 Remove nsEditor::mIMEBufferLength (r=ehsan)


>   nsPresContext* GetPresContext() const { return mPresContext; }
>   nsINode* GetEventTargetNode() const { return mNode; }
>   // The latest CompositionEvent.data value except compositionstart event.
>-  const nsString& GetLastData() const { return mLastData; }
>+  const nsString& LastData() const { return mLastData; }
>+  // The composition string which is already handled by the focused editor.
>+  // This value is modified by EditorDidHandleTextEvent().
>+  const nsString& String() const { return mString; }
I don't understand this comment. EditorDidHandleTextEvent() doesn't modify mString.
(and the returned value is const). Could you perhaps clarify the comment, and perhaps also
explain how String() is different to LastData().




>   nsRefPtr<IMETextTxn> txn = new IMETextTxn();
> 
>-  nsresult rv = txn->Init(mIMETextNode, mIMETextOffset, mIMEBufferLength,
>+  nsresult rv = txn->Init(mIMETextNode, mIMETextOffset,
>+                          mComposition->String().Length(),
>                           mIMETextRangeList, aStringToInsert, this);
I guess mComposition can't be null here. Could you add some comment why it is always valid here.




>-    mIMEBufferLength = widgetTextEvent->theText.Length();
>+    // XXX This approach is ugly, we should sort out the text event handling.
>+    mComposition->EditorDidHandleTextEvent();
yeah, it is error prone.
Should we have a stack class (TextEventHandlingMarker or somesuch) which calls WillHandle* in ctor and DidHandle* in dtor
and then just have TextEventHandlingMarker tehm(mComposition); 




>-    int32_t oldCompStrLength = mEditor->GetIMEBufferLength();
>+    nsRefPtr<TextComposition> composition = mEditor->GetComposition();
>+    int32_t oldCompStrLength = composition ? composition->String().Length() : 0;

I don't see reason for nsRefPtr. Wouldn't TextComposition* work just fine.
Attachment #8368407 - Flags: review?(bugs) → review+
Attachment #8367764 - Flags: review?(bugs) → review+
Attachment #8369939 - Flags: review?(bugs) → review+
Thank you, smaug.

This can hide the EditorWillHandleTextEvent() and EditorDidHandleTextEvent() by the nested class.
Attachment #8374592 - Flags: review?(bugs)
Attachment #8374592 - Flags: review?(bugs) → review+
Depends on: 1163304
You need to log in before you can comment on or make changes to this bug.