1172219 - [TSF][TS_E_NOLAYOUT][e10s] nsTextStore should put off to send notifications to TSF if it's dispatching composition events but hasn't received update composition notification

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

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

According to TSF spec, we can put off to answer GetTextExt() until our layout is completely prepared. So, I think that after NS_COMPOSITION_CHANGE, the layout may not be accessible especially in e10s mode. When ContentCache in TabParent is modified, widget should be notified the timing. Then, nsTextStore can notify TSF os layout change, text change and selection change.

This is experimental. I guess that some TIP might not work well. However, I believe that this is right approach for TSF.

Comment 1

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

Attachment: part.1 nsTextStore should notify IME of anything while events are being dispatched which may be handled asynchronously

Currently, IMEContentObserver sends notifications to IME only when it's safe to receive query content events and dispatch composition events. When it's run in e10s mode, TabParent may send notifications to widget which is received from PuppetWidget in remote process.

However, if there is a bug, nsTextStore will send some notifications to TSF when content is not available. If that occurs, TSF may be confused and stop working. For avoiding the worst scenario, nsTextStore should have another wall to block to send notifications when it's not safe.

This patch makes nsTextStore stores received notifications and flush them when it's safe to do that.

Comment 2

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

Attachment: part.2 Notify TSF of layout creation and destruction

Putting off layout change notifications may TSF believe that Gecko doesn't have layout. So, we should notify TSF of creating layout when nsTextStore is created (i.e., a text editor gets focus).

Comment 3

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

Attachment: part.3 Modify nsTextStore::mSelection with new selection in OnSelectionChangeInternal()

Additional patch for improving selection cache performance.

Now, selection change notification has new selection range. So, nsTextStore should use it instead of marking the selection dirty.

Comment 4

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

I gave up to return TS_E_NOLAYOUT after dispatching composition event and before receving NOTIFY_IME_OF_LAYOUT_CHANGE or NOTIFY_IME_OF_COMPOSITION_UPDATE because the TSF's bug around TS_E_NOLAYOUT and known TIPs do not work with such behavior...

Comment 5

[Masatoshi Kimura [:emk]]

Comment on attachment 8622353 [details] [diff] [review]
part.1 nsTextStore should notify IME of anything while events are being dispatched which may be handled asynchronously

Review of attachment 8622353 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsTextStore.cpp
@@ +1767,5 @@
> +           ("TSF: 0x%p   nsTextStore::MaybeFlushPendingNotifications(), "
> +            "putting off flushing pending notifications due to being the "
> +            "document locked...", this));
> +    return;
> +  }

Please add
if (mPendingDestroy) {
  Destroy();
  return;
}
here. Then you can avoid !mPendingDestroy checks below.

::: widget/windows/nsTextStore.h
@@ +786,5 @@
> +  // we shouldn't query layout information.  However, TS_E_NOLAYOUT bugs of
> +  // ITextStoreAPC::GetTextExt() blocks us to behave ideally.
> +  // For preventing it to be called, we should put off notifying TSF of
> +  // anything until layout information becomes available.
> +  bool                         mPutOffNotifyingTSF;

Could you use a word "delay" or something? Phrasal verbs are hard to understand for non-native speakers (at least for me).

Comment 6

[Masatoshi Kimura [:emk]]

Comment on attachment 8622353 [details] [diff] [review]
part.1 nsTextStore should notify IME of anything while events are being dispatched which may be handled asynchronously

> nsTextStore should notify IME of anything while events are being
> dispatched which may be handled asynchronously

By the way, I failed to parse this sentence. Could make the wording something better?

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/46a99800e86a
https://hg.mozilla.org/integration/mozilla-inbound/rev/3446822f321b
https://hg.mozilla.org/integration/mozilla-inbound/rev/842c06221e6a

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/46a99800e86a
https://hg.mozilla.org/mozilla-central/rev/3446822f321b
https://hg.mozilla.org/mozilla-central/rev/842c06221e6a