Makoto Kato [:m_kato] Assignee
	Description • 11 years ago

For candidate window of IME, GTK and Windows (IMM32) implementation calculate position/rect after dispatching text event.

But on e10s, dispatch is async, so NS_QUERY_* may not return correct value even if we implement it for remote code (this isn't implemented yet).

So we should notify composition rect to widget after text event.  It can becomes e10s aware by this.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 4 • 11 years ago

Aren't NS_QUERY_* events dispatched synchronously??

Additionally, on Mac, Cocoa may query character rect not only for candidate window. Similarly, on Windows, TSF have similar API and there is IMR_QUERYCHARPOSITION of WM_IME_REQUEST.

How about them?

	Makoto Kato [:m_kato] Assignee
	Comment 5 • 11 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #4)
> Aren't NS_QUERY_* events dispatched synchronously??

IPC doesn't support sync message from chrome to content.  So no easy way to implement NS_QUERY_TEXT_RECT for remote.  To implement NS_QUERY_TEXT_RECT for remote, we need build text rect arrays per text event on content process, then pass it to parent process as cache.

Do you have another idea?

> Additionally, on Mac, Cocoa may query character rect not only for candidate
> window. Similarly, on Windows, TSF have similar API and there is
> IMR_QUERYCHARPOSITION of WM_IME_REQUEST.

Yeah.  It has same problem.  After fixing this issue, I will think this issue.

	Makoto Kato [:m_kato] Assignee
	Comment 9 • 11 years ago

Comment on attachment 8338369 [details] [diff] [review]
Part 1. Notify compostion to widget

Notify update composition to widget with selected composition offset.

For e10s support, content process has to notify composition info to chrome process.

	Makoto Kato [:m_kato] Assignee
	Comment 10 • 11 years ago

Comment on attachment 8338370 [details] [diff] [review]
Part 2. Implement NS_QUERY_TEXT_RECT for remote

Cocoa calls FirstRectForCharacterRange for composition/candidate window, we need implement NS_QUERY_TEXT_RECT for it.  Cocoa always queries selected position, so we caches it to TabParent.

	Makoto Kato [:m_kato] Assignee
	Comment 11 • 11 years ago

Comment on attachment 8338371 [details] [diff] [review]
Part 3. Implement NS_QUERY_CARET_RECT for remote

It needs NS_QUERY_CARET_RECT for Cocoa.  Reason is same as NS_QUERY_TEXT_RECT.

	Makoto Kato [:m_kato] Assignee
	Comment 12 • 11 years ago

Windows (IMM32) and GTK patches will be coming tomorrow..

TSF is difficult to implement e10s for this.   after fixed this bug, I will investigate it.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 13 • 11 years ago

(In reply to Makoto Kato (:m_kato) from comment #12)
> TSF is difficult to implement e10s for this.   after fixed this bug, I will
> investigate it.

I think that e10s should implement synchronous communication from content process to chrome process for supporting all features of IME (E.g., mouse handling). So, I believe that you should file it after this.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 14 • 11 years ago

Comment on attachment 8338369 [details] [diff] [review]
Part 1. Notify compostion to widget

Could you attach patches created with -U 8 -p next time?

>--- a/content/events/src/TextComposition.cpp	Mon Nov 25 14:13:56 2013 -0500
>+++ b/content/events/src/TextComposition.cpp	Tue Nov 26 10:30:18 2013 +0900
>@@ -12,6 +12,7 @@
> #include "nsIMEStateManager.h"
> #include "nsIPresShell.h"
> #include "nsPresContext.h"
>+#include "nsIPrivateTextRange.h"
> #include "mozilla/MiscEvents.h"
> #include "mozilla/TextEvents.h"

You don't need to include it. You should use NS_TEXTRANGE_* instead of nsIPrivateTextRange::TEXTRANGE_*.

>@@ -56,6 +59,52 @@
> 
>   nsEventDispatcher::Dispatch(mNode, mPresContext,
>                               aEvent, nullptr, aStatus, aCallBack);
>+
>+  // Notify composition rect to widget if possible
>+  NotitySelectedCompositionRect(aEvent);
>+}
>+
>+void
>+TextComposition::NotitySelectedCompositionRect(WidgetGUIEvent* aEvent)
>+{
>+  nsCOMPtr<nsIWidget> widget = mPresContext->GetRootWidget();
>+  nsEventStatus status;
>+  uint32_t offset = 0;
>+
>+  // When compositon start, notify the rect of first offset character.
>+  // When not compositon start, notify the rect of selected composition
>+  // string if text event.
>+  if (aEvent->message == NS_COMPOSITION_START) {
>+    // Update composition start offset
>+    WidgetQueryContentEvent selectedTextEvent(true,
>+                                              NS_QUERY_SELECTED_TEXT,
>+                                              widget);
>+    widget->DispatchEvent(&selectedTextEvent, status);
>+    if (selectedTextEvent.mSucceeded) {
>+      mCompositionStartOffset = selectedTextEvent.mReply.mOffset;
>+    } else {
>+      // XXX Unknown offset
>+      NS_WARNING("Cannot get start offset of IME composition");
>+      mCompositionStartOffset = 0;
>+    }
>+  } else {
>+    if (aEvent->eventStructType != NS_TEXT_EVENT) {
>+      return;
>+    }
>+
>+    WidgetTextEvent *textEvent = aEvent->AsTextEvent();
>+    for (uint32_t i = 0; i < textEvent->rangeCount; i++) {
>+      TextRange& range = textEvent->rangeArray[i];
>+      if (range.mRangeType == nsIPrivateTextRange::TEXTRANGE_SELECTEDRAWTEXT ||
>+          range.mRangeType ==
>+            nsIPrivateTextRange::TEXTRANGE_SELECTEDCONVERTEDTEXT) {
>+        offset = range.mStartOffset;
>+        break;
>+      }
>+    }
>+  }
>+
>+  widget->NotifyIMEUpdateComposition(offset + mCompositionStartOffset);
> }

Looks like that this code is only needed by e10s. So, I don't like this to run every time since query content event may be expensive if HTML editor has a lot of contents. And also I don't like to add new IME API to nsIWidget as far as possible.

NotifyIMEUpdateComposition() only implemented on PuppetWidget actually. So, I think that you should add new notification to mozilla::widget::NotificationToIME:
http://mxr.mozilla.org/mozilla-central/source/widget/nsIWidget.h#439
and use nsIWidget::NotifyIME():
http://mxr.mozilla.org/mozilla-central/source/widget/nsIWidget.h#1594

Then, only PuppetWidget should handle the new notification with TextComposition instance. You should add a method to nsIMEStateManager like TextComposition* GetTextComposition(nsIWidget* aWidget);.

Then, you can add simple method like uint32_t Offset(); to TextComposition. It should cache the offset at called first time.

>diff -r 53d55d2d0a25 widget/xpwidgets/PuppetWidget.cpp
>--- a/widget/xpwidgets/PuppetWidget.cpp	Mon Nov 25 14:13:56 2013 -0500
>+++ b/widget/xpwidgets/PuppetWidget.cpp	Tue Nov 26 10:30:18 2013 +0900
>@@ -392,6 +392,25 @@
>   }
> }
> 
>+void
>+PuppetWidget::NotifyIMEUpdateComposition(const uint32_t aSelectedOffset)
>+{
>+  if (!mTabChild)
>+    return;

nit: use {} or NS_ENSURE_TRUE_VOID()?

>+  nsEventStatus status;
>+  WidgetQueryContentEvent textRect(true, NS_QUERY_TEXT_RECT, this);
>+  InitEvent(textRect, nullptr);
>+  textRect.InitForQueryTextRect(aSelectedOffset, 1);
>+  DispatchEvent(&textRect, status);
>+  if (!textRect.mSucceeded) {
>+    return;
>+  }

Let's use NS_ENSURE_TRUE_VOID() if it should succeed.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 15 • 11 years ago

Comment on attachment 8338370 [details] [diff] [review]
Part 2. Implement NS_QUERY_TEXT_RECT for remote

>--- a/content/events/src/nsEventStateManager.cpp	Thu Nov 07 09:11:11 2013 +0900
>+++ b/content/events/src/nsEventStateManager.cpp	Tue Nov 26 10:35:11 2013 +0900
>@@ -1220,7 +1220,8 @@
>     break;
>   case NS_QUERY_TEXT_RECT:
>     {
>-      // XXX remote event
>+      if (RemoteQueryContentEvent(aEvent))
>+        break;

nit: Use {}

>--- a/dom/ipc/TabParent.cpp	Thu Nov 07 09:11:11 2013 +0900
>+++ b/dom/ipc/TabParent.cpp	Tue Nov 26 10:35:11 2013 +0900
>@@ -968,6 +973,35 @@
>   return true;
> }
> 
>+nsIntPoint
>+TabParent::GetChildProcessOffset()
>+{
>+  // The "toplevel widget" in child processes is always at position
>+  // 0,0.  Map the event coordinates to match that.
>+
>+  nsIntPoint offset(0, 0);
>+  nsRefPtr<nsFrameLoader> frameLoader = GetFrameLoader();
>+  if (!frameLoader) {
>+    return offset;
>+  }
>+  nsIFrame* targetFrame = frameLoader->GetPrimaryFrameOfOwningContent();
>+  if (!targetFrame) {
>+    return offset;
>+  }
>+
>+  // Find out how far we're offset from the nearest widget.
>+  nsCOMPtr<nsIWidget> widget = GetWidget();
>+  if (!widget) {
>+    return offset;
>+  }
>+  nsPoint pt = nsLayoutUtils::GetEventCoordinatesRelativeTo(widget,
>+                                                            nsIntPoint(0, 0),
>+                                                            targetFrame);
>+
>+  return LayoutDeviceIntPoint::ToUntyped(LayoutDeviceIntPoint::FromAppUnitsToNearest(
>+           pt, targetFrame->PresContext()->AppUnitsPerDevPixel()));
>+}
>+

I'm not familiar with the code of coordinates. Please look for other reviewer for this.

>diff -r c97f09747560 dom/ipc/TabParent.h
>--- a/dom/ipc/TabParent.h	Thu Nov 07 09:11:11 2013 +0900
>+++ b/dom/ipc/TabParent.h	Tue Nov 26 10:35:11 2013 +0900
>@@ -308,6 +308,7 @@
> 
>     bool ShouldDelayDialogs();
>     bool AllowContentIME();
>+    nsIntPoint GetChildProcessOffset();
> 
>     virtual PRenderFrameParent* AllocPRenderFrameParent(ScrollingBehavior* aScrolling,
>                                                         TextureFactoryIdentifier* aTextureFactoryIdentifier,
>@@ -327,6 +328,9 @@
>     uint32_t mIMECompositionStart;
>     uint32_t mIMESeqno;
> 
>+    uint32_t mCompositionRectOffset;
>+    nsIntRect mCompositionRect;

Looks like IME related members should be named as mIME*.

r=masayuki except TabParent::GetChildProcessOffset().

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 16 • 11 years ago

Comment on attachment 8338371 [details] [diff] [review]
Part 3. Implement NS_QUERY_CARET_RECT for remote

>--- a/content/events/src/nsEventStateManager.cpp	Fri Nov 22 11:58:20 2013 +0900
>+++ b/content/events/src/nsEventStateManager.cpp	Tue Nov 26 10:37:17 2013 +0900
>@@ -1213,7 +1213,8 @@
>     break;
>   case NS_QUERY_CARET_RECT:
>     {
>-      // XXX remote event
>+      if (RemoteQueryContentEvent(aEvent))
>+        break;

nit: Use {}.

	Makoto Kato [:m_kato] Assignee
	Comment 17 • 11 years ago

I forget attaching platform specific code as part 4 and part 5... sorry.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #14)
> Comment on attachment 8338369 [details] [diff] [review]
> Part 1. Notify compostion to widget
> 
> Could you attach patches created with -U 8 -p next time?
> 
> >--- a/content/events/src/TextComposition.cpp	Mon Nov 25 14:13:56 2013 -0500
> >+++ b/content/events/src/TextComposition.cpp	Tue Nov 26 10:30:18 2013 +0900
> >@@ -12,6 +12,7 @@
> > #include "nsIMEStateManager.h"
> > #include "nsIPresShell.h"
> > #include "nsPresContext.h"
> >+#include "nsIPrivateTextRange.h"
> > #include "mozilla/MiscEvents.h"
> > #include "mozilla/TextEvents.h"
> 
> You don't need to include it. You should use NS_TEXTRANGE_* instead of
> nsIPrivateTextRange::TEXTRANGE_*.
> 
> >@@ -56,6 +59,52 @@
> > 
> >   nsEventDispatcher::Dispatch(mNode, mPresContext,
> >                               aEvent, nullptr, aStatus, aCallBack);
> >+
> >+  // Notify composition rect to widget if possible
> >+  NotitySelectedCompositionRect(aEvent);
> >+}
> >+
> >+void
> >+TextComposition::NotitySelectedCompositionRect(WidgetGUIEvent* aEvent)
> >+{
> >+  nsCOMPtr<nsIWidget> widget = mPresContext->GetRootWidget();
> >+  nsEventStatus status;
> >+  uint32_t offset = 0;
> >+
> >+  // When compositon start, notify the rect of first offset character.
> >+  // When not compositon start, notify the rect of selected composition
> >+  // string if text event.
> >+  if (aEvent->message == NS_COMPOSITION_START) {
> >+    // Update composition start offset
> >+    WidgetQueryContentEvent selectedTextEvent(true,
> >+                                              NS_QUERY_SELECTED_TEXT,
> >+                                              widget);
> >+    widget->DispatchEvent(&selectedTextEvent, status);
> >+    if (selectedTextEvent.mSucceeded) {
> >+      mCompositionStartOffset = selectedTextEvent.mReply.mOffset;
> >+    } else {
> >+      // XXX Unknown offset
> >+      NS_WARNING("Cannot get start offset of IME composition");
> >+      mCompositionStartOffset = 0;
> >+    }
> >+  } else {
> >+    if (aEvent->eventStructType != NS_TEXT_EVENT) {
> >+      return;
> >+    }
> >+
> >+    WidgetTextEvent *textEvent = aEvent->AsTextEvent();
> >+    for (uint32_t i = 0; i < textEvent->rangeCount; i++) {
> >+      TextRange& range = textEvent->rangeArray[i];
> >+      if (range.mRangeType == nsIPrivateTextRange::TEXTRANGE_SELECTEDRAWTEXT ||
> >+          range.mRangeType ==
> >+            nsIPrivateTextRange::TEXTRANGE_SELECTEDCONVERTEDTEXT) {
> >+        offset = range.mStartOffset;
> >+        break;
> >+      }
> >+    }
> >+  }
> >+
> >+  widget->NotifyIMEUpdateComposition(offset + mCompositionStartOffset);
> > }
> 
> Looks like that this code is only needed by e10s. So, I don't like this to
> run every time since query content event may be expensive if HTML editor has
> a lot of contents. And also I don't like to add new IME API to nsIWidget as
> far as possible.
> 
> NotifyIMEUpdateComposition() only implemented on PuppetWidget actually. So,
> I think that you should add new notification to
> mozilla::widget::NotificationToIME:
> http://mxr.mozilla.org/mozilla-central/source/widget/nsIWidget.h#439
> and use nsIWidget::NotifyIME():
> http://mxr.mozilla.org/mozilla-central/source/widget/nsIWidget.h#1594

NotificationToIME has no parameter.  But I want to notify IME candidate information of content to chrome.  Part 4 and Part 5 patches will use NotifyIMEUpdateComposition, then it can share calculating candidate area logic.

But if NS_QUERY_*'s cache always hits, it is unnecessary.

	Makoto Kato [:m_kato] Assignee
	Comment 18 • 11 years ago

this is depends on part 1's API.  Original concept that use part 1.

	Makoto Kato [:m_kato] Assignee
	Comment 19 • 11 years ago

this depends on part 1's API.  Original concept that use part 1.

	Makoto Kato [:m_kato] Assignee
	Comment 21 • 11 years ago

DispatchEvent is async, so current code is too early to call SetCursorPosition since tab parent's cache may not be updated on e10s.  So we use NotifyIME notification to update candidate window.

	Makoto Kato [:m_kato] Assignee
	Comment 23 • 11 years ago

Comment on attachment 8348753 [details] [diff] [review]
Part 4. GTK Implementation

garbage is added...

	Makoto Kato [:m_kato] Assignee
	Comment 24 • 11 years ago

DispatchTextEvent is async, we need wait for updating cache of Tab Parent for content process's info.

	Makoto Kato [:m_kato] Assignee
	Comment 26 • 11 years ago

Comment on attachment 8349856 [details] [diff] [review]
Part 5. Windows Implementaion

for Windows IMM32

	Makoto Kato [:m_kato] Assignee
	Comment 27 • 11 years ago

need include nsPresContext.h since it is used by template.

	Makoto Kato [:m_kato] Assignee
	Comment 28 • 11 years ago

sorry. this is correct.  need nsPresContext.h

	Makoto Kato [:m_kato] Assignee
	Comment 29 • 11 years ago

Comment on attachment 8348756 [details] [diff] [review]
Part 2. Support remote NS_QUERY_TEXT_RECT v2

need convert content rect to chrome's rect.  masayuki needs other reviewer.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 30 • 11 years ago

Comment on attachment 8351291 [details] [diff] [review]
Part 1. Notify IME Compostion to widget v2

>diff --git a/content/events/src/TextComposition.cpp b/content/events/src/TextComposition.cpp
>--- a/content/events/src/TextComposition.cpp
>+++ b/content/events/src/TextComposition.cpp
>@@ -21,26 +21,30 @@ namespace mozilla {
>  * TextComposition
>  ******************************************************************************/
> 
> TextComposition::TextComposition(nsPresContext* aPresContext,
>                                  nsINode* aNode,
>                                  WidgetGUIEvent* aEvent) :
>   mPresContext(aPresContext), mNode(aNode),
>   mNativeContext(aEvent->widget->GetInputContext().mNativeIMEContext),
>+  mCompositionStartOffset(0),
>+  mCompositionTargetOffset(0),

nit: no problem to write these new members in a line if it's not over 80 characters.

>@@ -51,16 +55,61 @@ TextComposition::DispatchEvent(WidgetGUI
>                                nsDispatchingCallback* aCallBack)
> {
>   if (aEvent->message == NS_COMPOSITION_UPDATE) {
>     mLastData = aEvent->AsCompositionEvent()->data;
>   }
> 
>   nsEventDispatcher::Dispatch(mNode, mPresContext,
>                               aEvent, nullptr, aStatus, aCallBack);
>+
>+  // Notify composition update to widget if possible
>+  NotityUpdateComposition(aEvent);
>+}
>+
>+void
>+TextComposition::NotityUpdateComposition(WidgetGUIEvent* aEvent)
>+{
>+  nsEventStatus status;
>+
>+  // When compositon start, notify the rect of first offset character.
>+  // When not compositon start, notify the rect of selected composition
>+  // string if text event.
>+  if (aEvent->message == NS_COMPOSITION_START) {
>+    nsCOMPtr<nsIWidget> widget = mPresContext->GetRootWidget();
>+    // Update composition start offset
>+    WidgetQueryContentEvent selectedTextEvent(true,
>+                                              NS_QUERY_SELECTED_TEXT,
>+                                              widget);
>+    widget->DispatchEvent(&selectedTextEvent, status);
>+    if (selectedTextEvent.mSucceeded) {
>+      mCompositionStartOffset = selectedTextEvent.mReply.mOffset;
>+    } else {
>+      // XXX Unknown offset

nit: Please remove this XXX comment, it's very clear ;-)

>+      NS_WARNING("Cannot get start offset of IME composition");
>+      mCompositionStartOffset = 0;
>+    }
>+    mCompositionTargetOffset = mCompositionStartOffset;
>+  } else {
>+    if (aEvent->eventStructType != NS_TEXT_EVENT) {
>+      return;
>+    }

Why don't you write this as |} else if (aEvent->eventStructType == NS_TEXT_EVENT) {| and add else block for quick return?

>+    WidgetTextEvent *textEvent = aEvent->AsTextEvent();
>+    for (uint32_t i = 0; i < textEvent->rangeCount; i++) {
>+      TextRange& range = textEvent->rangeArray[i];
>+      if (range.mRangeType == NS_TEXTRANGE_SELECTEDRAWTEXT ||
>+          range.mRangeType == NS_TEXTRANGE_SELECTEDCONVERTEDTEXT) {
>+        mCompositionTargetOffset = mCompositionStartOffset + range.mStartOffset;
>+        break;
>+      }
>+    }

I guess that mCompositionTargetOffset should be mCompositionStartOffset if there is no selected range, isn't it?

FYI: Although, I think that this should be in WidgetTextEvent. However, as I said in Tokyo office, I'll remove text event in the future. So, you don't need to do so.

>diff --git a/content/events/src/TextComposition.h b/content/events/src/TextComposition.h
>--- a/content/events/src/TextComposition.h
>+++ b/content/events/src/TextComposition.h
>@@ -61,47 +62,60 @@ public:
>   void SynthesizeCommit(bool aDiscard);
> 
>   /**
>    * Send a notification to IME.  It depends on the IME or platform spec what
>    * will occur (or not occur).
>    */
>   nsresult NotifyIME(widget::NotificationToIME aNotification);
> 
>+  /**
>+   * return the offset of compositon start position
>+   */
>+  uint32_t Offset() const { return mCompositionTargetOffset; }

Ah, then, could you rename the method to OffsetOfTargetClause()? And make it clearer the offset is from start of the editor.

>+  uint32_t mCompositionStartOffset;
>+  uint32_t mCompositionTargetOffset;

Could you add explanation for them? E.g.,

// Offset of the composition string from start of the editor

// Offset of the selected clause of the composition string from start of the editor

>diff --git a/widget/nsIWidget.h b/widget/nsIWidget.h
>--- a/widget/nsIWidget.h
>+++ b/widget/nsIWidget.h
>@@ -445,17 +445,18 @@ enum NotificationToIME {
>   NOTIFY_IME_OF_CURSOR_POS_CHANGED,
>   // An editable content is getting focus
>   NOTIFY_IME_OF_FOCUS,
>   // An editable content is losing focus
>   NOTIFY_IME_OF_BLUR,
>   // Selection in the focused editable content is changed
>   NOTIFY_IME_OF_SELECTION_CHANGE,
>   REQUEST_TO_COMMIT_COMPOSITION,
>-  REQUEST_TO_CANCEL_COMPOSITION
>+  REQUEST_TO_CANCEL_COMPOSITION,
>+  NOTIFY_IME_UPDATE_COMPOSITION

NOTIFY_IME_OF_COMPOSITION_UPDATED is better. And explain it, e.g., "Composition string has been updated".

Otherwise, LGTM.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 31 • 11 years ago

Comment on attachment 8349856 [details] [diff] [review]
Part 5. Windows Implementaion

>diff -r 8928edcb33e0 widget/windows/nsIMM32Handler.cpp
>--- a/widget/windows/nsIMM32Handler.cpp	Tue Dec 17 23:32:40 2013 +0900
>+++ b/widget/windows/nsIMM32Handler.cpp	Wed Dec 18 17:18:26 2013 +0900
>@@ -235,16 +235,29 @@ nsIMM32Handler::CancelComposition(nsWind
>     ::ImmNotifyIME(IMEContext.get(), NI_COMPOSITIONSTR, CPS_CANCEL, 0);
>   }
> 
>   if (associated) {
>     IMEContext.Disassociate();
>   }
> }
> 
>+// static
>+void
>+nsIMM32Handler::UpdateComposition(nsWindow* aWindow)

I think OnCompositionUpdated() is better.

>+{
>+  if (!gIMM32Handler) {
>+    return;
>+  }

This must be called while composition. So, gIMM32Handler must not be nullptr. Therefore, this should be NS_ENSURE_TRUE_VOID() or MOZ_ASSERT(). Either is okay.

>+  nsIMEContext IMEContext(aWindow->GetWindowHandle());
>+  gIMM32Handler->SetIMERelatedWindowsPos(aWindow, IMEContext);

Actually, this might not have problem even when windowless plugin has focus because composition events nor text events never fired while windowless plugin has focus. However, JS may set focus to a windowless plugin. For safer code, could you check aWindow->PluginHasFocus()? If it's true, you shouldn't do nothing. (This isn't your mistake, current code also has this bug).

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 32 • 11 years ago

Comment on attachment 8351291 [details] [diff] [review]
Part 1. Notify IME Compostion to widget v2

>@@ -51,16 +55,61 @@ TextComposition::DispatchEvent(WidgetGUI
>                                nsDispatchingCallback* aCallBack)
> {
>   if (aEvent->message == NS_COMPOSITION_UPDATE) {
>     mLastData = aEvent->AsCompositionEvent()->data;
>   }
> 
>   nsEventDispatcher::Dispatch(mNode, mPresContext,
>                               aEvent, nullptr, aStatus, aCallBack);
>+
>+  // Notify composition update to widget if possible
>+  NotityUpdateComposition(aEvent);
>+}

Oops, I realized that this is safe but the composition has already finished by JS (e.g., blur()) at calling NotityUpdateComposition().

>diff --git a/content/events/src/nsIMEStateManager.cpp b/content/events/src/nsIMEStateManager.cpp
>--- a/content/events/src/nsIMEStateManager.cpp
>+++ b/content/events/src/nsIMEStateManager.cpp
>@@ -590,16 +590,17 @@ nsIMEStateManager::NotifyIME(Notificatio
> 
>   TextComposition* composition = nullptr;
>   if (sTextCompositions) {
>     composition = sTextCompositions->GetCompositionFor(aWidget);
>   }
>   if (!composition || !composition->IsSynthesizedForTests()) {
>     switch (aNotification) {
>       case NOTIFY_IME_OF_CURSOR_POS_CHANGED:
>+      case NOTIFY_IME_UPDATE_COMPOSITION:
>         return aWidget->NotifyIME(aNotification);
>       case REQUEST_TO_COMMIT_COMPOSITION:
>       case REQUEST_TO_CANCEL_COMPOSITION:
>         return composition ? aWidget->NotifyIME(aNotification) : NS_OK;

Therefore, the new notification should be called only when the composition isn't nullptr. So, move the new case below REQUEST_TO_CANCEL_COMPOSITION.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 33 • 11 years ago

Comment on attachment 8348756 [details] [diff] [review]
Part 2. Support remote NS_QUERY_TEXT_RECT v2

But could you perhaps upload also a patch which contains all the changes.
Hard to see the big picture from this small patch.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 36 • 11 years ago

Comment on attachment 8359604 [details] [diff] [review]
Part 1. Notify IME Compostion to widget v3.1

>+  } else {
>+    WidgetTextEvent *textEvent = aEvent->AsTextEvent();

nit: WidgetTextEvent* textEvent (position of *)

>     /**
>+     * Notifies chrome that there is a IME compostion rect updated

nit: an IME

>   nsresult NotifyIMEOfFocusChange(bool aFocus);
>   nsresult NotifyIMEOfSelectionChange();
>+  nsresult NotifyIMEUpdateComposition();

Should be NotifyIMEOfUpdateComposition().

Thank you for your great work!

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 37 • 11 years ago

Comment on attachment 8359604 [details] [diff] [review]
Part 1. Notify IME Compostion to widget v3.1

>+  /**
>+   * return the offset of compositon start position
>+   */
>+  uint32_t OffsetOfTargetClause() const { return mCompositionTargetOffset; }

Oops, please update this comment before landing. E.g., "the offset of first selected clause or start of composition"

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 38 • 11 years ago

And could you file a blocker bug of bug 926798 for implementing a way of synchronous communication between chrome and content? I still believe that e10s team needs to implement it for supporting IME without any regressions.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 39 • 11 years ago

Don't we have rpc semantics for chrome->content? (Though, even that should be avoided if possible)

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 40 • 11 years ago

We have "urgent" messages that are synchronous and go from chrome to content. We should think very hard before using them though. I don't know how IME works. Why do we need sync messages from chrome to content?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 41 • 11 years ago

IME queries contents information for some purpose, for example,

* Queries caret or selected clause position for showing candidate window or helper window notifying the user of current input mode.
* Queries whether a position is on a clause of composition string for changing selected clause by pointing device.
* Queries selected string for starting reconversion.
* Queries text around composition string for deciding the order of items in candidate window.
* Queries text around caret for deciding proper character of south Asian language.

I.e., when IME requests to query content information, we need to return the result synchronously.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 42 • 11 years ago

Thanks very much. Can you point me to an example where this happens? How hard would it be to rewrite it to be asynchronous?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 43 • 11 years ago

For example, nsTextStore::GetTextExt() is called by IME for querying the rect of specific character(s). For returning the result to the our param of this method, we need to dispatch query event synchronously.
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsTextStore.cpp#2311

This is what we cannot cache the result before this method is called since we cannot store the rect of all characters of the focused editor which may have long text and may be changed the location by scroll.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 44 • 11 years ago

FYI: the patches of this bug do store some information of the focused editor to the puppet widget as far as possible. I.e., even we will land them, e10s supports only minimum function of IME (and also e10s makes fixing IME bugs more difficult).

	Makoto Kato [:m_kato] Assignee
	Comment 45 • 11 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #38)
> And could you file a blocker bug of bug 926798 for implementing a way of
> synchronous communication between chrome and content? I still believe that
> e10s team needs to implement it for supporting IME without any regressions.

I don't create a meta bug for e10s IME.  If tab parent can have a cache for composition information and can forecast the information that IME requires, it may be unnecessary to use synchronous communication.

To implement for TSF, I have to create new bug for TSF-specific after this bug.


(In reply to Bill McCloskey (:billm) from comment #42)
> Thanks very much. Can you point me to an example where this happens? How
> hard would it be to rewrite it to be asynchronous?

NS_QUERY_* event is used by OS's widget on chrome process.  These query text/composition/etc information in content process.  If all composition data is always cached into TabParent, it can implement asynchronous only.

I don't investigate TSF on e10s yet, but TSF may require it to access content information as long as I discuss with Nakano-san.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 46 • 11 years ago

(In reply to Makoto Kato (:m_kato) from comment #45)
> (In reply to Bill McCloskey (:billm) from comment #42)
> > Thanks very much. Can you point me to an example where this happens? How
> > hard would it be to rewrite it to be asynchronous?
> 
> NS_QUERY_* event is used by OS's widget on chrome process.  These query
> text/composition/etc information in content process.  If all composition
> data is always cached into TabParent, it can implement asynchronous only.

Yep, but on Mac, the behavior depends on IME. Current your patch assumes all IMEs behave same as you checked IMEs. If some IME request unexpected char's rect in the future (or some IMEs which we don't know now), it may be difficult to fix it.

Caching useful values improves performance. Therefore, your patch works forever. But we should have slower path for unexpected behavior of IME.

	Makoto Kato [:m_kato] Assignee
	Comment 47 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a228a62d0b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f09f17e27e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/9961f372f396
https://hg.mozilla.org/integration/mozilla-inbound/rev/eece07fce955
https://hg.mozilla.org/integration/mozilla-inbound/rev/2abc599e2d4f

	Ryan VanderMeulen [:RyanVM]
	Comment 48 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/2a228a62d0b8
https://hg.mozilla.org/mozilla-central/rev/8f09f17e27e9
https://hg.mozilla.org/mozilla-central/rev/9961f372f396
https://hg.mozilla.org/mozilla-central/rev/eece07fce955
https://hg.mozilla.org/mozilla-central/rev/2abc599e2d4f

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 49 • 11 years ago

Thanks very much for this work. Based on what you have said above, it sounds like this is what still needs to be done:

1. Implement TSF support.
2. Handle IME events that ask for data that is not currently cached.

If that's correct, it would be very helpful if you guys could file bugs for these issues.

> For example, nsTextStore::GetTextExt() is called by IME for querying the rect of specific
> character(s). For returning the result to the our param of this method, we need to dispatch
> query event synchronously.
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsTextStore.cpp#2311

It sounds like this method is part of TSF? I'm having a difficult time understanding how methods like this are used. It sounds like TSF is useful both for Japanese/Chinese input as well as screen readers?

The problem with synchronous messages from the parent is that they can ruin our performance. I would be willing to use synchronous messages for screen readers, maybe, since they're pretty rare. But I assume most of Asia uses IME, and it would be very unfortunate to lose one of the main benefits of electrolysis for all those users.

	Makoto Kato [:m_kato] Assignee
	Comment 50 • 11 years ago

(In reply to Bill McCloskey (:billm) from comment #49)
> Thanks very much for this work. Based on what you have said above, it sounds
> like this is what still needs to be done:
> 
> 1. Implement TSF support.
> 2. Handle IME events that ask for data that is not currently cached.
> 
> If that's correct, it would be very helpful if you guys could file bugs for
> these issues.

Yes, I filed TSF bug as bug 960836.  For caching, I will handle after TSF minimum implementation.

> > For example, nsTextStore::GetTextExt() is called by IME for querying the rect of specific
> > character(s). For returning the result to the our param of this method, we need to dispatch
> > query event synchronously.
> > http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsTextStore.cpp#2311
> 
> It sounds like this method is part of TSF? I'm having a difficult time
> understanding how methods like this are used. It sounds like TSF is useful
> both for Japanese/Chinese input as well as screen readers?

Yes, this is for TSF.