1184004 - [e10s] Methods to notify IME should use IMENotification at IPC

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

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

Currently, methods to notify IME take a lot of arguments which is necessary to create IMENotification in parent process. I don't know why we don't send just IMENotification or its data struct directly, though, I think that we should do that.

Comment 1

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

Attachment: part.1 Give a name to each struct in IMENotification

For making nsGUIEventIPC.h simpler, we should give a name to each struct in IMENotification.

Comment 2

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

Attachment: part.2 Make all structs in IMENotification IPC-aware

Then, we can make nsGUIEventIPC.h simpler and clearer.

Comment 3

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

Attachment: part.3 IPC methods to notify IME should use IMENotification for its argument

Finally, we should make NotifyIME() related methods of IPC simpler.

I renamed PuppetWidget::NotifyIMEOfUpdateComposition() to PuppetWidget::NotifyIMEOfCompositionUpdate() since the notification is named NOTIFY_IME_OF_COMPOSITION_UPDATE.

Additionally, I sorted out PuppetWidget::InitIMEState(). It calls TabParent::RecvNotifyIMEFocus() as blur. However, in such case, NOTIFY_IME_OF_BLUR will be ignored in IMEStateManager::NotifyIME() because IME shouldn't have focus (i.e., IMEStateManager must not have sent NOTIFY_IME_OF_FOCUS before that). Then, RecvNotifyIMEFocus() does only mContentCache.AssignContent(). Therefore, it's enough PuppetWidget to call SendUpdateContentCache().
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#2046

Comment 4

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

Comment on attachment 8634176 [details] [diff] [review]
part.3 IPC methods to notify IME should use IMENotification for its argument

> ContentCacheInParent::MaybeNotifyIME(nsIWidget* aWidget,
>-                                     IMENotification& aNotification)
>+                                     const IMENotification& aNotification)
> {
>-  if (aNotification.mMessage == NOTIFY_IME_OF_SELECTION_CHANGE) {
>-    aNotification.mSelectionChangeData.mOffset = mSelection.StartOffset();
>-    aNotification.mSelectionChangeData.mLength = mSelection.Length();
>-    aNotification.mSelectionChangeData.mReversed = mSelection.Reversed();
>-    aNotification.mSelectionChangeData.SetWritingMode(mSelection.mWritingMode);
>-  }
>-
I don't understand this change. We used to update aNotification with whatever data we had for selection, but
here we end up using aNotification as such.


Please explain and ask review again.

Comment 5

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

Comment on attachment 8634176 [details] [diff] [review]
part.3 IPC methods to notify IME should use IMENotification for its argument

mSelection was initialized before calling the method:
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp?rev=91d6e262b662&mark=2031-2031#2022

The result must be same as mSelectionChangeDtata because aContentCache.mSelection was initialized with aIMENotification.mSelectionChangeData:
http://mxr.mozilla.org/mozilla-central/source/widget/PuppetWidget.cpp?rev=119f3f9c406f&mark=790-795#775

Therefore, we don't need to set mSelectionChangeData again. We can use coming data from child process.

Comment 6

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

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/0e22c42d98dda844249b116de80a638f8ecff397
changeset:  0e22c42d98dda844249b116de80a638f8ecff397
user:       Masayuki Nakano <[e-mail]>
date:       Fri Jul 17 13:30:01 2015 +0900
description:
Bug 1184004 part.1 Give a name to each struct in IMENotification r=smaug

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/314c6be265d0630c31fcc6459a89970e3616bbe6
changeset:  314c6be265d0630c31fcc6459a89970e3616bbe6
user:       Masayuki Nakano <[e-mail]>
date:       Fri Jul 17 13:30:01 2015 +0900
description:
Bug 1184004 part.2 Make all structs in IMENotification IPC-aware r=smaug

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/9a31fa80ee8c647940dd075d27181ab9d688cc52
changeset:  9a31fa80ee8c647940dd075d27181ab9d688cc52
user:       Masayuki Nakano <[e-mail]>
date:       Fri Jul 17 13:30:01 2015 +0900
description:
Bug 1184004 part.3 IPC methods to notify IME should use IMENotification for its argument r=smaug

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0e22c42d98dd
https://hg.mozilla.org/mozilla-central/rev/314c6be265d0
https://hg.mozilla.org/mozilla-central/rev/9a31fa80ee8c