Closed Bug 1184986 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Content Processes, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
e10s + ---
firefox42 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 2 open bugs)

Details

(Keywords: inputmethod)

Attachments

(1 file)

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.
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.
Attachment #8636564 - Flags: review?(bugs)
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()) {
Attachment #8636564 - Flags: review?(bugs) → review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/d75fd3b93ae8be1e704f3d24cc4100f8d93973da
changeset:  d75fd3b93ae8be1e704f3d24cc4100f8d93973da
user:       Masayuki Nakano <masayuki@d-toybox.com>
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
https://hg.mozilla.org/mozilla-central/rev/d75fd3b93ae8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Summary: [e10s][TSF] ContentCache shouldn't notify IME of layout change until all sending events are handled → [e10s][TS_E_NOLAYOUT][TSF] ContentCache shouldn't notify IME of layout change until all sending events are handled
You need to log in before you can comment on or make changes to this bug.