1184449 - [IMM] nsIMM32Handler should cache selection notified by NOTIFY_IME_OF_SELECTION_CHANGE

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Comment 1

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

Attachment: part.1 IMENotifiation::SelectionChangeData should store selected string

First, it's useful if NOTIFY_IME_OF_SELECTION_CHANGE notifies widget of new selected string instead of just its length.

Although, I don't like using nsString* in union due to the risk (nsString has constructor, therefore, it's not available as a part of union). However, I have no better idea.

Comment 2

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

Attachment: part.2 nsIMM32Handler should store selection range as far as possible

Comment 3

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

Comment on attachment 8635104 [details] [diff] [review]
part.1 IMENotifiation::SelectionChangeData should store selected string

>   IMENotification notification(NOTIFY_IME_OF_SELECTION_CHANGE);
>-  notification.mSelectionChangeData.mOffset =
>-    selection.mReply.mOffset;
>-  notification.mSelectionChangeData.mLength =
>-    selection.mReply.mString.Length();
>+  notification.mSelectionChangeData.mOffset = selection.mReply.mOffset;
>+  *notification.mSelectionChangeData.mString = selection.mReply.mString;
oh, we guarantee this kind of notification has mString pointing to a nsString... ok

> struct ParamTraits<mozilla::widget::IMENotification::SelectionChangeData>
> {
>   typedef mozilla::widget::IMENotification::SelectionChangeData paramType;
> 
>   static void Write(Message* aMsg, const paramType& aParam)
>   {
>     WriteParam(aMsg, aParam.mOffset);
>-    WriteParam(aMsg, aParam.mLength);
>+    WriteParam(aMsg, *aParam.mString);

could we MOZ_ASSERT somewhere here that mString is non-null


>   static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
>   {
>+    aResult->mString = new nsString();
Hmm, it is a bit unfortunate that we can't really assert here the mString points to null before assignment.
>+  void Assign(const IMENotification& aOther)
>+  {
>+    Clear();
>+    mMessage = aOther.mMessage;
>+    switch (mMessage) {
>+      case NOTIFY_IME_OF_SELECTION_CHANGE:
>+        mSelectionChangeData = aOther.mSelectionChangeData;
>+        // mString should be different instance because of ownership issue.
>+        mSelectionChangeData.mString = new nsString();
>+        *mSelectionChangeData.mString = aOther.mSelectionChangeData.String();

Couldn't you do something like.
mSelectionChangeData.mString = new nsString(aOther.mSelectionChangeData.String());

>   void Clear()
>   {
>+    if (mMessage == NOTIFY_IME_OF_SELECTION_CHANGE) {
>+      delete mSelectionChangeData.mString;
Please assert before deletion that mString is non-null.
and set mSelectionChangeData.mString to nullptr after deletion.

Comment 4

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

Comment on attachment 8635105 [details] [diff] [review]
part.2 nsIMM32Handler should store selection range as far as possible

Thank you, smaug.

Could you review this, Kimura-san?

nsIMM32Handler's singleton instance should cache the latest selection information for performance since NOTIFY_IME_OF_SELECTION_CHANGE have the latest selection information.

However, nsIMM32Handler is created when it handles some WM_IME_* is received but WM_IME_REQUEST *could* be received even without focused editor. Therefore,

* NOTIFY_IME_OF_SELECTION_CHANGE may be notified when there is no singleton instance, in this case, we cannot (and shouldn't) cache the selection.

* NOTIFY_IME_OF_SELECTION_CHANGE is notified only during NOTIFY_IME_OF_FOCUS and NOTIFY_IME_OF_BLUR, therefore, nsIMM32Handler cannot cache the latest selection after NOTIFY_IME_OF_BLUR. sHasFocus becomes true during the notifications. When it's true, nsIMM32Handler can use mSelection. Otherwise, they should use temporary instance and need to use NS_QUERY_SELECTED_TEXT.

BTW, I was thinking what's the best name of Selection::EnsureToInit(). Is Selection::EnsureValidSelection() better?

Comment 5

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

And note that nsIMM32Handler needs to receive NOTIFY_IME_OF_FOCUS and NOTIFY_IME_OF_BLUR even during a TIP is active in TSF mode since user may switches active TIP to IMM-IME during an editor has focus. Therefore, even when IMM-IME isn't active, nsIMM32Handler needs to modify sHasFocus.

Comment 6

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

Attachment: part.1 IMENotifiation::SelectionChangeData should store selected string (r=smaug)

Comment 7

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

Attachment: part.2 nsIMM32Handler should store selection range as far as possible

Renaming Selection::EnsureToInit() to Selection::EnsureValidSelection().

Comment 8

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

Comment on attachment 8635680 [details] [diff] [review]
part.2 nsIMM32Handler should store selection range as far as possible

Oops, I found a bug in Selection::Clear() (setting mIsValid true).

I'll post new patch after testing this patch again.

Comment 9

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

Attachment: part.1 IMENotifiation::SelectionChangeData should store selected string (r=smaug)

merged with the latest m-c.

Comment 10

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

Attachment: part.2 nsIMM32Handler should store selection range as far as possible

I'm very sorry for the spams.

Comment 11

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

Comment on attachment 8635709 [details] [diff] [review]
part.2 nsIMM32Handler should store selection range as far as possible

Perhaps, emk-san is busy now. And I want to fix this ASAP. Kato-san, could you review this? If you can, I'll land this without emk-san's review.

Comment 12

[Makoto Kato [:m_kato]]

Comment on attachment 8635709 [details] [diff] [review]
part.2 nsIMM32Handler should store selection range as far as possible

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

::: widget/windows/nsIMM32Handler.cpp
@@ +2600,5 @@
> +nsIMM32Handler::Selection::IsValid() const
> +{
> +  if (!mIsValid) {
> +    return false;
> +  }

If mOffset is UINT32_MAX, we should return false, too.

@@ +2609,5 @@
> +
> +bool
> +nsIMM32Handler::Selection::Update(const IMENotification& aIMENotification)
> +{
> +  Clear();

Remove Clear() because mOffset and mIsValid is overridden after.

::: widget/windows/nsIMM32Handler.h
@@ +391,5 @@
>  
> +  struct Selection
> +  {
> +    uint32_t mOffset;
> +    nsString mString;

For alignment, the following order is better

  nsString mString;
  uint32_t mOffset;

Comment 13

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

Comment on attachment 8635709 [details] [diff] [review]
part.2 nsIMM32Handler should store selection range as far as possible

Thank you, Makoto-san!

Comment 14

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

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/56319240dff61410b6dffc7ae80ba2cd41f9e1eb
changeset:  56319240dff61410b6dffc7ae80ba2cd41f9e1eb
user:       Masayuki Nakano <[e-mail]>
date:       Wed Jul 22 12:40:32 2015 +0900
description:
Bug 1184449 part.1 IMENotifiation::SelectionChangeData should store selected string r=smaug

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/4f684751728862e83dff92bd4eee5282f9a3f78e
changeset:  4f684751728862e83dff92bd4eee5282f9a3f78e
user:       Masayuki Nakano <[e-mail]>
date:       Wed Jul 22 12:40:32 2015 +0900
description:
Bug 1184449 part.2 nsIMM32Handler should store selection range as far as possible r=m_kato

Comment 15

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

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/2ab28c08a7744aeeddbd2255dbc7d2a87c21106e
changeset:  2ab28c08a7744aeeddbd2255dbc7d2a87c21106e
user:       Masayuki Nakano <[e-mail]>
date:       Wed Jul 22 13:28:23 2015 +0900
description:
Bug 1184449 part.3 Fix the bustage of Mac OS X r=myself

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/56319240dff6
https://hg.mozilla.org/mozilla-central/rev/4f6847517288
https://hg.mozilla.org/mozilla-central/rev/2ab28c08a774

Comment 17

[Masatoshi Kimura [:emk]]

>  Selection GetSelection() { return sHasFocus ? mSelection : Selection(); }

This function will return *a copy of* mSelection if sHasFocus is true.

>    Selection& selection = GetSelection();
>    if (!selection.EnsureValidSelection(aWindow)) {

So the above code will never initialize nsIMM32Handler::mSelection even if mSelection is not initialized yet because |selection| refers a copy of mSelection. Is this an intended behavior?

Comment 18

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

(In reply to Masatoshi Kimura [:emk] from comment #17)
> >  Selection GetSelection() { return sHasFocus ? mSelection : Selection(); }
> 
> This function will return *a copy of* mSelection if sHasFocus is true.
> 
> >    Selection& selection = GetSelection();
> >    if (!selection.EnsureValidSelection(aWindow)) {
> 
> So the above code will never initialize nsIMM32Handler::mSelection even if
> mSelection is not initialized yet because |selection| refers a copy of
> mSelection. Is this an intended behavior?

Ah, that's not intentional... I'll file a follow up bug. Thanks!