960866 - Make nsEditor store composition with TextComposition

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

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

Currently, nsEditor manages composition itself. However, now, there is TextComposition class. Let's make nsEditor use it.

Comment 1

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

Attachment: part.1 nsEditor should store a pointer of TextComposition while composition

While there is a composition, nsEditor should hold mozilla::TextComposition which is alive during compositionstart - compositionend.

Comment 2

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

Attachment: part.2 Remove nsEditor::mInIMEMode

mInIMEMode is true during compositionstart - compositionend.

I.e., this equals to !!mComposition. Therefore, we can remove it.

Comment 3

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

Attachment: part.3 Remove nsEditor::mIsIMEComposing

nsEditor::mIsIMEComposing is true while the editor has uncommitted composition string.

We can move it to mozilla::TextComposition with a new method EditorWillHandleTextEvent().

Comment 4

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

Attachment: part.4 Remove nsEditor::mIMEBufferLength

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

Comment 5

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

Attachment: part.5 nsEditor shouldn't allow composition and text events created by chrome JS

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

Comment 6

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

Attachment: part.6 Emulate editor behavior in mozilla::TextComposition after focused editor is destroyed during composition

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.

Comment 7

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

Attachment: All changes by the patches

Comment 8

[(no longer active)]

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.

Comment 9

[(no longer active)]

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.

Comment 10

[(no longer active)]

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?

Comment 11

[(no longer active)]

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.

Comment 12

[(no longer active)]

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.

Comment 13

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

Attachment: part.1 nsEditor should store a pointer of TextComposition while composition

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

Comment 14

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

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

Comment 15

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

Attachment: part.2 Remove nsEditor::mInIMEMode

Comment 16

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

Attachment: part.3 Remove nsEditor::mIsIMEComposing (r=ehsan)

Comment 17

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

Attachment: part.4 Remove nsEditor::mIMEBufferLength (r=ehsan)

Comment 18

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

Attachment: part.6 mozilla::TextComposition should holds a weak reference to composition handling editor during composition

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.

Comment 19

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

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.

Comment 20

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

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 21

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

Comment on attachment 8368389 [details] [diff] [review]
part.1 nsEditor should store a pointer of TextComposition while composition

See comment 13 for the detail.

Comment 22

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

Comment on attachment 8368405 [details] [diff] [review]
part.2 Remove nsEditor::mInIMEMode

This has minor change for EnsureComposition().

Comment 23

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

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.

Comment 24

[(no longer active)]

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

Comment 25

[(no longer active)]

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

[(no longer active)]

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.

Comment 27

[(no longer active)]

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

Comment 28

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

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.

Comment 29

[(no longer active)]

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

Comment 30

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

(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

Comment 31

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

Attachment: part.1 nsEditor should store a pointer of TextComposition while composition

This patch really ensures to store mComposition (if it fails, it crashes).

Comment 32

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

Attachment: part.2 Remove nsEditor::mInIMEMode (r=ehsan)

merged with new part.1 patch.

Comment 33

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

Attachment: part.6 mozilla::TextComposition should holds a weak reference to composition handling editor during composition (r=ehsan)

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=dd188a8dc92c

Comment 34

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

Attachment: part.1 nsEditor should store a pointer of TextComposition while composition

Sorry for the spam. Fixing a nit.

Comment 35

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

Attachment: part.1 nsEditor should store a pointer of TextComposition while composition

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

Comment 36

[(no longer active)]

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!

Comment 37

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

Smaug:

Could you review this if you're available? This bug blocks some bugs which I'm working on.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Sorry about delay. Will review tomorrow.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Would it be possible to have a patch which contains all the changes. Easier to see the big picture that way.

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Or I just review these patches now.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 42

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

Attachment: part.4 Remove nsEditor::mIMEBufferLength (r=ehsan+smaug)

Comment 43

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

Attachment: part.7 Use stack class for ensuring to call EditorWillHandleTextEvent() and EditorDidHandleTextEvent()

Thank you, smaug.

This can hide the EditorWillHandleTextEvent() and EditorDidHandleTextEvent() by the nested class.

Comment 44

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/184e8ed6ca48
https://hg.mozilla.org/integration/mozilla-inbound/rev/41d17ebf8454
https://hg.mozilla.org/integration/mozilla-inbound/rev/984a2e65a52f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5159f90e44ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/48191ef36ffc
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a547a926c48
https://hg.mozilla.org/integration/mozilla-inbound/rev/29de65dfd9c7

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/184e8ed6ca48
https://hg.mozilla.org/mozilla-central/rev/41d17ebf8454
https://hg.mozilla.org/mozilla-central/rev/984a2e65a52f
https://hg.mozilla.org/mozilla-central/rev/5159f90e44ef
https://hg.mozilla.org/mozilla-central/rev/48191ef36ffc
https://hg.mozilla.org/mozilla-central/rev/5a547a926c48
https://hg.mozilla.org/mozilla-central/rev/29de65dfd9c7