Closed Bug 960877 Opened 6 years ago Closed 6 years ago

e10s must update IME info cache into TabParent

Categories

(Core :: DOM: Events, defect)

x86
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: m_kato, Assigned: m_kato)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

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.
Attached patch always update cache w/o B2G (obsolete) — Splinter Review
Attached patch always update cache if not B2G (obsolete) — Splinter Review
Attachment #8361555 - Attachment is obsolete: true
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.
Attachment #8361573 - Flags: review?(masayuki)
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.
Attachment #8361573 - Flags: review?(masayuki)
Blocks: e10s-ime-tsf
Comment on attachment 8365783 [details] [diff] [review]
Part 1. Add helper method to nsIMEUpdatePreference

add simple helper functions to nsIMEUpdatePreference to check flags
Attachment #8365783 - Flags: review?(masayuki)
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.
Attachment #8365785 - Flags: review?(masayuki)
Comment on attachment 8365783 [details] [diff] [review]
Part 1. Add helper method to nsIMEUpdatePreference

Thanks!!
Attachment #8365783 - Flags: review?(masayuki) → review+
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.
Attachment #8365786 - Flags: review?(masayuki)
Attachment #8365785 - Flags: review?(masayuki) → review+
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.
Attachment #8365786 - Flags: review?(masayuki)
Comment on attachment 8367113 [details] [diff] [review]
Part 3. Always notice IME info to parent process if not B2G

inherit parent's setting.
Attachment #8367113 - Flags: review?(masayuki)
Comment on attachment 8367113 [details] [diff] [review]
Part 3. Always notice IME info to parent process if not B2G

Thanks!
Attachment #8367113 - Flags: review?(masayuki) → review+
Makoto-san, could you land them as far as possible? Especially, the part.1 helps my next work.
Depends on: 1050553
You need to log in before you can comment on or make changes to this bug.