543398 - Drop nsTextEventReply and nsIPrivateCompositionEvent

Status: RESOLVED FIXED

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

Description

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

Attachment: Patch v1.0

After bug 520732, we can drop nsTextEventReply and nsIPrivateCompositionEvent.

See also bug 528435.

Comment 1

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

After I fix this bug, we can:

* drop nsIPlaintextEditor::eEditorUseAsyncUpdatesMask
* drop nsCompositionEvent by replacing to nsInputEvent

and also we might be able to drop nsIPrivateTextEvent and  nsIPrivateTextRangeList because we can access to the information by nsIPrivateDOMEvent::GetInternalNSEvent() safely.

Comment 2

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

Why we can drop nsIPlaintextEditor::eEditorUseAsyncUpdatesMask?
Textformcontrol uses it in nsTextControlFrame::SetValue for performance
reasons.

Comment 3

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

(In reply to comment #2)
> Why we can drop nsIPlaintextEditor::eEditorUseAsyncUpdatesMask?
> Textformcontrol uses it in nsTextControlFrame::SetValue for performance
> reasons.

Looks like the synchronous mode is only needed for IME which will be dropped by this patch. So, I guess that nsEditor won't need the synchronous mode. If some code need the synchronous mode, cannot they use FlushPendingNotifications instead?

The flags are only referred from:
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#4411
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#2545

Comment 4

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

At least at the moment textcontrolframe uses sync mode normally:
see http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsTextControlFrame.cpp#1395

::SetValue is exception.

Comment 5

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

Thank you for the point.

Looks like when an editor is modified, it should be updated immediately. Otherwise, asynchronous mode is enough, probably. However, such change isn't small, I guess. So, I won't work for it, please ignore that. Thanks.

Comment 6

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

Attachment: Patch v2.0

Comment 7

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

Could you explain the patch a bit, especially why the following isn't valid 
anymore - why we don't need the coordinates anymore?
- // XXX The IME stuff needs caret coordinates
- // XXX_kin: synchronously, but the editor could be using async
- // XXX_kin: updates (reflows and paints) for performance reasons.
- // XXX_kin: In order to give IME what it needs, we have to temporarily
- // XXX_kin: switch to sync updating during this call so that the
- // XXX_kin: nsAutoUpdateViewBatch can force sync reflows and paints
- // XXX_kin: so that we get back accurate caret coordinates.

Comment 8

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

Sorry.

1. Currently, widget can query the caret position (or other character rect) at anytime. So, the event reply isn't needed right now. Therefore, editor don't need to asynchronous update for the caret position at the beginning a composition.

2. IME will query the caret position or first character rect or something. Then, our query event flush the pending reflow.

Comment 9

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

(In reply to comment #8)
> Sorry.
> 
> 1. Currently, widget can query the caret position (or other character rect) at
> anytime. So, the event reply isn't needed right now. Therefore, editor don't
> need to asynchronous update for the caret position at the beginning a
> composition.
Editor has *synchronous* update for the caret position.
So currently widget can do synchronous query for the caret position?
And that does flush the layout or something?

> 2. IME will query the caret position or first character rect or something.
> Then, our query event flush the pending reflow.
So when IME queries caret position, there is a flush. Ok, sounds good.

Comment 10

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

(In reply to comment #9)
> (In reply to comment #8)
> > Sorry.
> > 
> > 1. Currently, widget can query the caret position (or other character rect) at
> > anytime. So, the event reply isn't needed right now. Therefore, editor don't
> > need to asynchronous update for the caret position at the beginning a
> > composition.
> Editor has *synchronous* update for the caret position.

Ah, I meant "synchronous", sorry.

> So currently widget can do synchronous query for the caret position?
> And that does flush the layout or something?

The query content event flush layout forcedly.

http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsContentEventHandler.cpp#87

So, the widget can do synchronous query.

Comment 11

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

Comment on attachment 433685 [details] [diff] [review]
Patch v2.0

I assume you have tested this on all platforms.

Comment 12

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

Attachment: Patch v2.0.1

merged to latest trunk.

Comment 13

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

http://hg.mozilla.org/mozilla-central/rev/bd0b0915864b