Closed Bug 1184004 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Content Processes, defect)

40 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
e10s + ---
firefox42 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

(Keywords: inputmethod)

Attachments

(3 files)

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.
For making nsGUIEventIPC.h simpler, we should give a name to each struct in IMENotification.
Attachment #8634168 - Flags: review?(bugs)
Then, we can make nsGUIEventIPC.h simpler and clearer.
Attachment #8634171 - Flags: review?(bugs)
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
Attachment #8634176 - Flags: review?(bugs)
Attachment #8634168 - Flags: review?(bugs) → review+
Attachment #8634171 - Flags: review?(bugs) → review+
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.
Attachment #8634176 - Flags: review?(bugs)
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.
Attachment #8634176 - Flags: review?(bugs)
Attachment #8634176 - Flags: review?(bugs) → review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/0e22c42d98dda844249b116de80a638f8ecff397
changeset:  0e22c42d98dda844249b116de80a638f8ecff397
user:       Masayuki Nakano <masayuki@d-toybox.com>
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 <masayuki@d-toybox.com>
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 <masayuki@d-toybox.com>
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
You need to log in before you can comment on or make changes to this bug.