960877 - e10s must update IME info cache into TabParent

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Makoto Kato [:m_kato]]

follow up bug 926798 and bug 935821.

If e10s, we should set nsIMEUpdatePreference::NOTIFY_TEXT_CHANGE and nsIMEUpdatePreference::NOTIFY_SELECTION_CHANGE to update IME information cache into TabParent correctly.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: always update cache w/o B2G

Comment 2

[Makoto Kato [:m_kato]]

Attachment: always update cache if not B2G

Comment 3

[Makoto Kato [:m_kato]]

Comment on attachment 8361573 [details] [diff] [review]
always update cache if not B2G

follow up e10s IME support.  TabParent's IME information has to be updated if e10s.  But B2G is unnecessary.

Comment 4

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

Comment on attachment 8361573 [details] [diff] [review]
always update cache if not B2G

>diff --git a/widget/xpwidgets/PuppetWidget.h b/widget/xpwidgets/PuppetWidget.h
>--- a/widget/xpwidgets/PuppetWidget.h
>+++ b/widget/xpwidgets/PuppetWidget.h
>@@ -207,17 +207,18 @@ private:
>   nsIntRegion mDirtyRegion;
>   nsRevocableEventPtr<PaintTask> mPaintTask;
>   bool mEnabled;
>   bool mVisible;
>   // XXX/cjones: keeping this around until we teach LayerManager to do
>   // retained-content-only transactions
>   nsRefPtr<gfxASurface> mSurface;
>   // IME
>-  nsIMEUpdatePreference mIMEPreference;
>+  static nsIMEUpdatePreference sIMEPreference;
>+  nsIMEUpdatePreference mIMEPreferenceOnChrome;

I might have understood them correctly (hopefully!), but it was too hard for me to understand the difference.

I like better that sIMEPreference is renamed to sIMEUpdatePreferenceOfChild and mIMEPreferenceOnChrome is renamed to mIMEUpdatePreferenceOfParent. I think that content and chrome actually match child and parent, but I guess that it's not correct logically.

>diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp
>--- a/dom/ipc/TabParent.cpp
>+++ b/dom/ipc/TabParent.cpp
>@@ -1030,17 +1030,21 @@ TabParent::RecvNotifyIMESelection(const 
> {
>   nsCOMPtr<nsIWidget> widget = GetWidget();
>   if (!widget)
>     return true;
> 
>   if (aSeqno == mIMESeqno) {
>     mIMESelectionAnchor = aAnchor;
>     mIMESelectionFocus = aFocus;
>-    widget->NotifyIME(NOTIFY_IME_OF_SELECTION_CHANGE);
>+
>+    if (widget->GetIMEUpdatePreference().mWantUpdates &
>+          nsIMEUpdatePreference::NOTIFY_SELECTION_CHANGE) {
>+      widget->NotifyIME(NOTIFY_IME_OF_SELECTION_CHANGE);
>+    }

I think that the bit flag computation looks redundant and somebody will take some mistakes in the future. I'll create helper methods to nsIMEUpdatePreference...

>diff --git a/widget/xpwidgets/PuppetWidget.cpp b/widget/xpwidgets/PuppetWidget.cpp
>--- a/widget/xpwidgets/PuppetWidget.cpp
>+++ b/widget/xpwidgets/PuppetWidget.cpp
>@@ -44,16 +44,24 @@ nsIWidget::CreatePuppetWidget(TabChild* 
> 
>   nsCOMPtr<nsIWidget> widget = new PuppetWidget(aTabChild);
>   return widget.forget();
> }
> 
> namespace mozilla {
> namespace widget {
> 
>+#ifdef MOZ_CROSS_PROCESS_IME
>+nsIMEUpdatePreference PuppetWidget::sIMEPreference(
>+                        nsIMEUpdatePreference::NOTIFY_SELECTION_CHANGE |
>+                        nsIMEUpdatePreference::NOTIFY_TEXT_CHANGE, true);
>+#else
>+nsIMEUpdatePreference PuppetWidget::sIMEPreference;
>+#endif

Looks like that sIMEPreference is only used for the result of PuppetWidget::GetIMEUpdatePreference() and the values are never modified.

First, I don't think this needs to be static variable in class global. I think that it's enough PuppetWidget::GetIMEUpdatePreference() creates the instance directly at called.

Additionally, now, mWantUpdates has a new value implemented in bug 835262. So, I think that PuppetWidget::GetIMEUpdatePreference() should be:

nsIMEUpdatePreference
PuppetWidget::GetIMEUpdatePreference()
{
#ifdef MOZ_CROSS_PROCESS_IME
  return nsIMEUpdatePreference(mIMEUpdatePreferenceOfParent.mWantsUpdate |
                               nsIMEUpdatePreference::NOTIFY_SELECTION_CHANGE |
                               nsIMEUpdatePreference::NOTIFY_TEXT_CHANGE, true);
#else
  return mIMEUpdatePreferenceOfParent;
#endif
}

Isn't it? Then, you can rid of the static variable.

Comment 5

[Makoto Kato [:m_kato]]

Attachment: Part 1. Add helper method to nsIMEUpdatePreference

Comment 6

[Makoto Kato [:m_kato]]

Attachment: Part 2. Remove mWantHints from nsIMEUpdatePreference

Comment 7

[Makoto Kato [:m_kato]]

Attachment: Part 3. Always notice IME info to parent process if not B2G

Comment 8

[Makoto Kato [:m_kato]]

Comment on attachment 8365783 [details] [diff] [review]
Part 1. Add helper method to nsIMEUpdatePreference

add simple helper functions to nsIMEUpdatePreference to check flags

Comment 9

[Makoto Kato [:m_kato]]

Comment on attachment 8365785 [details] [diff] [review]
Part 2. Remove mWantHints from nsIMEUpdatePreference

When gecko wants to support OS level IME on e10s platform, MOZ_CROSS_PROCESS_IME is defined and parent process uses NS_QUERY_* to access content data.

So mWantHints is useless.  We should remove it.

Comment 10

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

Comment on attachment 8365783 [details] [diff] [review]
Part 1. Add helper method to nsIMEUpdatePreference

Thanks!!

Comment 11

[Makoto Kato [:m_kato]]

Comment on attachment 8365786 [details] [diff] [review]
Part 3. Always notice IME info to parent process if not B2G

always notice text information to parent process.

Comment 12

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

Comment on attachment 8365786 [details] [diff] [review]
Part 3. Always notice IME info to parent process if not B2G

> nsIMEUpdatePreference
> PuppetWidget::GetIMEUpdatePreference()
> {
>-  return mIMEPreference;
>+#ifdef MOZ_CROSS_PROESS_IME
>+  // e10s requires IME information cache into TabParent
>+  return nsIMEUpdatePreference(nsIMEUpdatePreference::NOTIFY_SELECTION_CHANGE |
>+                               nsIMEUpdatePreference::NOTIFY_TEXT_CHANGE);

Why don't you use parent's hints too?

I.e.,
mIMEPreferenceOfParent.mWantHints |
nsIMEUpdatePreference::NOTIFY_SELECTION_CHANGE |
nsIMEUpdatePreference::NOTIFY_TEXT_CHANGE

This change looks break bug 835262.

Comment 13

[Makoto Kato [:m_kato]]

Attachment: Part 3. Always notice IME info to parent process if not B2G

Comment 14

[Makoto Kato [:m_kato]]

Comment on attachment 8367113 [details] [diff] [review]
Part 3. Always notice IME info to parent process if not B2G

inherit parent's setting.

Comment 15

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

Comment on attachment 8367113 [details] [diff] [review]
Part 3. Always notice IME info to parent process if not B2G

Thanks!

Comment 16

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

Makoto-san, could you land them as far as possible? Especially, the part.1 helps my next work.

Comment 17

[Makoto Kato [:m_kato]]

ah, I just landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/68444ba6d0a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f52cf83ac72
https://hg.mozilla.org/integration/mozilla-inbound/rev/40db7da176c2

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/68444ba6d0a0
https://hg.mozilla.org/mozilla-central/rev/9f52cf83ac72
https://hg.mozilla.org/mozilla-central/rev/40db7da176c2