1184986 - [e10s][TS_E_NOLAYOUT][TSF] ContentCache shouldn't notify IME of layout change until all sending events are handled

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

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

I was thinking that we don't have problem to notify layout change even if all events haven't been handled by the content process yet. However, I found it's wrong.

nsTextStore returns TS_E_NO_LAYOUT if queried text rect hasn't been computed yet. After that, we need to notify TSF of layout change when it's available. However, while two or more changes are being sent, we notify TSF of layout change *before* all layout is computed.

So, this must be a cause of making TSF confused.

Comment 1

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

Attachment: NOTIFY_IME_OF_POSITION_CHANGE should be put off until all sending events are recieved by child process

Looks like that this prevents a lot of odd behavior of TSF (Window) because IME won't hit unexpected content (i.e., not including the latest composition string) at querying the content when layout change is notified.

Comment 2

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

Comment on attachment 8636564 [details] [diff] [review]
NOTIFY_IME_OF_POSITION_CHANGE should be put off until all sending events are recieved by child process

>@@ -1008,29 +1011,38 @@ ContentCacheInParent::FlushPendingNotifi
>   if (mPendingSelectionChange.HasNotification()) {
>     IMENotification notification(mPendingSelectionChange);
>     if (!aWidget->Destroyed()) {
>       mPendingSelectionChange.Clear();
>       IMEStateManager::NotifyIME(notification, aWidget, true);
>     }
>   }
> 
>+  if (mPendingLayoutChange.HasNotification()) {
>+    IMENotification notification(mPendingLayoutChange);
>+    if (!aWidget->Destroyed()) {
>+      mPendingLayoutChange.Clear();
>+      IMEStateManager::NotifyIME(notification, aWidget, true);
>+    }
>+  }
>+
>   // Finally, send composition update notification because it notifies IME of
>   // finishing handling whole sending events.
>   if (mPendingCompositionUpdate.HasNotification()) {
>     IMENotification notification(mPendingCompositionUpdate);
>     if (!aWidget->Destroyed()) {
>       mPendingCompositionUpdate.Clear();
>       IMEStateManager::NotifyIME(notification, aWidget, true);
>     }
>   }
Perhaps you could add a comment why if (mPendingLayoutChange.HasNotification()) { is where it is.
Like why it isn't after if (mPendingCompositionUpdate.HasNotification()) { and why it is after if (mPendingSelectionChange.HasNotification()) {

Comment 3

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

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/d75fd3b93ae8be1e704f3d24cc4100f8d93973da
changeset:  d75fd3b93ae8be1e704f3d24cc4100f8d93973da
user:       Masayuki Nakano <[e-mail]>
date:       Wed Jul 22 14:15:06 2015 +0900
description:
Bug 1184986 NOTIFY_IME_OF_POSITION_CHANGE should be put off until all sending events are recieved by child process r=smaug

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d75fd3b93ae8