1096220 - Optimize NOTIFY_IME_OF_POSITION_CHANGE

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Makoto Kato [:m_kato]]

NOTIFY_IME_OF_POSITION_CHANGE uses reflow observer.  So when selection change, this will is also fired.

We should not fire this notification when modifying composition string such as input.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: WIP

but not full test yet.

Comment 2

[Makoto Kato [:m_kato]]

Attachment: NOTIFY_IME_OF_POSITION_CHANGE shouldn't be fired if editor rect isn't changed

Comment 3

[Makoto Kato [:m_kato]]

Comment on attachment 8523478 [details] [diff] [review]
NOTIFY_IME_OF_POSITION_CHANGE shouldn't be fired if editor rect isn't changed

Since Position Change notification uses relow observer, even if string or selection is changed, it will be fired.

It should be only fired editable area is changed.

Comment 4

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

Comment on attachment 8523478 [details] [diff] [review]
NOTIFY_IME_OF_POSITION_CHANGE shouldn't be fired if editor rect isn't changed

Although, I've not tested this patch yet, but I guess this is wrong because:

1. This don't notify widget of scrolled *in* editor.
2. This don't notify widget of top level widget (i.e., a window) movement.

I'm not sure #2 is notified by this, but if it's notified now, widget will stop moving candidate window by dragging of window.

Comment 5

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #4)
> Comment on attachment 8523478 [details] [diff] [review]
> NOTIFY_IME_OF_POSITION_CHANGE shouldn't be fired if editor rect isn't changed
> 
> Although, I've not tested this patch yet, but I guess this is wrong because:
> 
> 1. This don't notify widget of scrolled *in* editor.

I should only check reflow case, not scroll changed.

> 2. This don't notify widget of top level widget (i.e., a window) movement.

nsBaseWidget will call NOTIFY_IME_OF_POSITON_CHANGE directly, so even if this fix, it is always fired.

Comment 6

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

m_kato: ping... what's this state?

Comment 7

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #6)
> m_kato: ping... what's this state?

I have no item to optimize this now.  Nakano-san, do you still have any item?

Comment 8

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

No, I don't. However, in current implementation, we have two optimization in e10s mode (one optimization in non-e10s mode).

One is, IMEContentObserver does not send NOTIFY_IME_OF_* until finishing native event handling <https://dxr.mozilla.org/mozilla-central/rev/052656fc513c05da969590ac5934abd67271a897/layout/base/nsPresShell.cpp#8498-8504> and multiple notifications are merged for each type of notification. Therefore, IMEContentObserver cuts a lot of NOTIFY_IME_OF_POSITION_CHANGE during an edit action.

The other is, ContentCacheInParent does not send NOTIFY_IME_OF_* until remote process handles all posted composition and selection events <https://dxr.mozilla.org/mozilla-central/rev/052656fc513c05da969590ac5934abd67271a897/widget/ContentCache.cpp#1197-1200>.

So, I think that we can now close this bug as FIXED since other bug succeeded to reduce the count of NOTIFY_IME_OF_POSITION_CHANGE for every edit. How do you think? Should we keep struggling with reducing the notification more in some cases?

Comment 9

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #8)
> So, I think that we can now close this bug as FIXED since other bug
> succeeded to reduce the count of NOTIFY_IME_OF_POSITION_CHANGE for every
> edit. How do you think? Should we keep struggling with reducing the
> notification more in some cases?

Let's close this as FIXED.  When we want to add new optimization, we will file a new bug.