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

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({inputmethod})

Trunk
mozilla30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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);
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #8367839 - Attachment is obsolete: true
Attachment #8374746 - Flags: superreview?(roc)
Attachment #8374746 - Flags: review?(bugs)
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.
Attachment #8374746 - Flags: superreview?(roc) → superreview-
Comment on attachment 8374746 [details] [diff] [review]
Patch

Looks good, but would like to see a new patch with roc's comments fixed.
Attachment #8374746 - Flags: review?(bugs)
Posted patch PatchSplinter Review
The member names become longer. If you have better name, let me know.
Attachment #8374746 - Attachment is obsolete: true
Attachment #8377111 - Flags: superreview?(roc)
Attachment #8377111 - Flags: review?(bugs)
Attachment #8377111 - Flags: superreview?(roc) → superreview+
Attachment #8377111 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/43e799fdb80b
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.