Closed Bug 805767 Opened 7 years ago Closed 7 years ago

nsIMEManager::CreateTextStateManager() should use nsIWidget::GetIMEUpdatePreference() rather than the result of OnIMEFocusChange()

Categories

(Core :: Widget, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod)

Attachments

(3 files, 1 obsolete file)

Currently, nsIMEStateManager::CreateTextStateManager() decides if it needs to create nsTextStateManager instance from the result of nsIWidget::OnIMEFocusChange().

Some platforms returns NS_ERROR_NOT_IMPLEMENTED even if OnIMEFocusChange() is implemented on them but they don't need nsTextStateManager notifications. This is very odd.

Now, we have nsIWidget::GetIMEUpdatePreference(). nsIMEStateManager::CreateTextStateManager() should use this.
I believe that the result of nsIWidget::OnIMEFocusChange() should be simple.

The result indicates if nsTextStateManager should observe selection changes or text changes in the focused editor. This is very ugly since this is a hack for bug 496360.

On the other hand, we've added nsIWidget::GetIMEUpdatePreference(). The result's mWantUpdates indicates same as the result of OnIMEFocusChange().

So, nsTextStateManager can use GetIMEUpdatePreference().
Attachment #680585 - Flags: review?(bugs)
We should return nsIMEUpdatePreference(true, false) if TSF is enabled. Otherwise, nsIMEUpdatePreference(false, false).

If the first argument is true, nsTextStateManager will call nsIWidget::OnIMESelectionChange() and nsIWidget::OnIMETextChange(). So, IMM doesn't need them.
Attachment #680586 - Flags: review?(VYV03354)
OnIMEFocusChange() should return NS_OK if it's implemented actually.

We can make the result of OnIMEFocusChange() void. But I think that we shouldn't change interface if we don't have enough reason.
Attachment #680587 - Flags: review?(roc)
Status: NEW → ASSIGNED
Attachment #680586 - Flags: review?(VYV03354) → review+
Comment on attachment 680585 [details] [diff] [review]
part.1 nsTextStateManager should use nsIWidget::GetIMEUpdatePreference() instead of the result of nsIWidget::OnIMEFocusChange()


>   if (mNeedIMEStateInit) {
>     uint32_t chromeSeqno;
>-    mTabChild->SendNotifyIMEFocus(false, &mIMEPreference, &chromeSeqno);
>+    mTabChild->SendNotifyIMEFocus(false, &chromeSeqno);
>     mIMELastBlurSeqno = mIMELastReceivedSeqno = chromeSeqno;
>+    mTabChild->SendGetIMEUpdatePreference(&mIMEPreference);

So this doesn't look quite optimal.
Sending messages to parent isn't super fast, especially sync.
Attachment #680585 - Flags: review?(bugs) → review-
Then, we should not change the dom/ipc code and PuppetWidget::GetIMEUpdatePreference() should return the cached value. This is enough for nsTextStateManager.
Attachment #680585 - Attachment is obsolete: true
Attachment #680969 - Flags: review?(bugs)
Attachment #680969 - Flags: review?(bugs) → review+
You need to log in before you can comment on or make changes to this bug.