965685 - Merge nsIWidget::NotifyIMEOfTextChange() and nsIWidget::NotifyIME()

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

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

nsIWidget::NotifyIMEOfTextChange() takes specific arguments. Therefore, it is independent method from nsIWidget::NotifyIME().

However, I'd like to add common argument to nsIWidget::NotifyIME() in other bug. If nsIWidget::NotifyIME() takes a struct for its argument, I can do it easy.

I'm thinking it as:

struct NotificationToIME
{
  enum Message
  {
    NOTIFY_IME_OF_...,
    ...
  };

  Message mMessage;

  // NOTIFY_IME_OF_TEXT_CHANGE specific data
  uint32_t mStartOffset;
  uint32_t mOldEndOffset;
  uint32_t mNewEndOffset;
};

nsIWidget::NotifyIME(const NotificationToIME& aNotificationToIME);

Comment 1

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

Attachment: Patch

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=9acc34c3aaf4
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=e42933daa9d5

Comment 2

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

Attachment: Patch

I think that with this change, we can send:

* new selection range at NOTIFY_IME_OF_SELECTION_CHANGE
* new character bounding boxes of composition string at NOTIFY_IME_OF_COMPOSITION_UPDATE

Such additional information will reduce the cost of cross process communication of e10s since widget don't need to dispatch query events for the notification.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8374746 [details] [diff] [review]
Patch

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

::: dom/events/nsIMEStateManager.h
@@ +118,1 @@
>                              nsIWidget* aWidget);

Add "typedef mozilla::widget::IMEMessage IMEMessage;" to this class so it doesn't have to use these prefixes...

::: widget/nsIWidget.h
@@ +487,5 @@
> +
> +  // NOTIFY_IME_OF_TEXT_CHANGE specific data
> +  uint32_t mStartOffset;
> +  uint32_t mOldEndOffset;
> +  uint32_t mNewEndOffset;

Put these three fields in a struct, and put that struct in a union so that the other message types can use other branches of the union.

Comment 4

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

Comment on attachment 8374746 [details] [diff] [review]
Patch

Looks good, but would like to see a new patch with roc's comments fixed.

Comment 5

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

Attachment: Patch

The member names become longer. If you have better name, let me know.

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/43e799fdb80b

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/43e799fdb80b