Closed Bug 961704 Opened 6 years ago Closed 6 years ago

[TSF] Add an option for nsTextStateManager can ignore text change and selection change which are caused by composition

Categories

(Core :: Widget: Win32, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

TSF doesn't want to be notified text change and selection change which are caused by dispatching text event.

The cost of notifying them isn't cheap because nsContentEventHandler needs to work hard. So, ignoring them during dispatching text event makes the performance during composition.
Hmm, this bug also needs to make TextComposition refcountable for checking whether at selection change and text change are caused during dispatching composition/text event.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Even with the patches, text change notification and selection change notification may occur since JS or something can change contents at handling compositionstart, compositionupdate, compositionend, text and/or input events. We need more work later.
Attachment #8379544 - Flags: review?(bugs)
Comment on attachment 8379545 [details] [diff] [review]
part.2 Add an option to nsIMEUpdatePreference which prevents to be notified selection/text changes caused by composition

This patch adds NOTIFY_CHANGES_CAUSED_BY_COMPOSITION. If this is NOT set, nsTextStateManager doesn't send notification if the selection/text change is caused by editor handling composition.

This reduces the performance of TSF mode.

On the other hand, e10s needs to cache text contents and selection every time. Therefore, PuppetWidget sets the flag always. However, if parent's widget doesn't have the flag, Neither PupperWidget nor TabParent doesn't send text/selection change notification to parent's widget.

Note that this change makes it possible that nsTextStore handles selection/text changes which are caused by JS or something at composition events, text event and/or input event handled. But we need more work for it. I'll file a bug later.
Attachment #8379545 - Flags: review?(bugs)
FYI: I wan thinking about the new flag as NOTIFY_CHANGES_*NOT*_CAUSED_BY_COMPOSITION.

It's indeed simpler for implementing nsIWidget::GetIMEUpdatePreferene(). However, it makes other parts which might be modified more frequently complicated. Therefore, I named it NOTIFY_CHANGES_CAUSED_BY_COMPOSITION.

Note that nsIWidget::GetIMEUpdatePreference() isn't modified so many times. It should be modified only when widget needs new notification.
Comment on attachment 8379544 [details] [diff] [review]
part.1 nsTextStateManager should use new helper methods of nsIMEUpdatePreference

Should mUpdatePreference be set whenever mWidget is set to non-null value?
It is a bit odd to see mUpdatePreference being set in ObserveEditableNode.

but I see this is just replacing existing code with something a bit different looking.
Attachment #8379544 - Flags: review?(bugs) → review+
Comment on attachment 8379545 [details] [diff] [review]
part.2 Add an option to nsIMEUpdatePreference which prevents to be notified selection/text changes caused by composition

>   /**
>+   * Returns true when editor handles an event for modifying composition
>+   * string.
>+   */

Maybe something like
Returns true while editor is handling an event which is modifying the composition string.

>+  // mIsEditorHandlingEvent is true during editor modifies composition
>+  // string by a dispatched event.
And then something similar comment here too.

>+  enum MOZ_ENUM_TYPE(Notifications)
>   {
>-    NOTIFY_NOTHING           = 0x00,
>-    NOTIFY_SELECTION_CHANGE  = 0x01,
>-    NOTIFY_TEXT_CHANGE       = 0x02,
>-    NOTIFY_DURING_DEACTIVE   = 0x80
>+    NOTIFY_NOTHING                       = 0,
>+    NOTIFY_SELECTION_CHANGE              = 1 << 0,
>+    NOTIFY_TEXT_CHANGE                   = 1 << 1,
>+    // Following values indicate when widget needs or doesn't need notification.
>+    NOTIFY_CHANGES_CAUSED_BY_COMPOSITION = 1 << 6,
>+    // NOTE: NOTIFY_DURING_DEACTIVE hasn't supported environments which can
>+    //       exists two or more compositions.  E.g., Mac and Linux (GTK).
Could you clarify this comment. Do you mean
".. isn't supported in environments where two or more simultaneous compositions are possible. E.g...."
or what?
                         nsIMEUpdatePreference::NOTIFY_TEXT_CHANGE);
>+  return nsIMEUpdatePreference(
>+           mIMEPreferenceOfParent.mWantUpdates |
>+           nsIMEUpdatePreference::NOTIFY_SELECTION_CHANGE |
>+           nsIMEUpdatePreference::NOTIFY_TEXT_CHANGE |
>+           nsIMEUpdatePreference::NOTIFY_CHANGES_CAUSED_BY_COMPOSITION);
I would prefer we don't copy all the flags in every widget.
Could we have some default flags somewhere, probably as a static
member variable/method in nsIMEUpdatePreference, and then each widget implementation
would add and perhaps remove flags from it.
(Hopefully with some comment why certain thing is added or removed to/from the default)

Could I see that fixed? I think it would make this code a bit easier to maintain.
Attachment #8379545 - Flags: review?(bugs) → review-
How about this? Ctors includes the flag always. And if a widget doesn't need the flag, it needs to remove the flag explicitly.
Attachment #8379545 - Attachment is obsolete: true
Attachment #8381189 - Flags: review?(bugs)
Attachment #8381189 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/1289de3c12fe
https://hg.mozilla.org/mozilla-central/rev/5abffd2c5d9c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.