975260 - e10s IME should send all each character rect for NS_QUERY_TEXT_RECT

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Makoto Kato [:m_kato]]

Now, we add a cache that is the rect of first character of composition.  But this way cannot handle TSF implementation since TSF requests several length.

To support TSF, we should send all each character rect like Chromium. (see ImeCompositionRangeChanged on chromium source code)

Comment 1

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

Did you confirm that chromium stores all characters in the editor? I'm not sure Chromium stores all character rects in the focused editor. As far as I read the code roughly, it seems that Chromium just stores the rects of characters of composition string.
http://mxr.mozilla.org/chromium/source/src/win8/metro_driver/ime/text_store.cc#555
Looks like that chromium lies the focused text store is empty while there is no composition string.

If we will do it, I guess it makes big damage to the performance at every modification of text, every layout change, e.g., resizing textarea by the resizer.

I don't think that it's lower cost than e10s allows synchronous query of content process. Before you will work on this, I strongly hope that e10s guys explain why this approach is better than synchronous query. We should cc e10s guys to this bug.

Comment 2

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

FYI: If we need to fix this bug with all characters for bug 301451 (Mac).

I don't think that it's problem even if busy content process causes chrome process waiting its response while users are operating on content. I believe that users complain about no response of the content even if we use only async communication because they want to do something on the busy content process.

Comment 3

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

> FYI: If we need to fix this bug with all characters for bug 301451 (Mac).

I meant that if we need to fix this bug, we need to store all characters for bug 301451 (Mac).

Comment 4

[Makoto Kato [:m_kato]]

This means composing string, not all text into editor.

Comment 5

[Makoto Kato [:m_kato]]

Even if IMM32, IMR_QUERYCHARPOSITION needs all rect of composition string.  If it needs IPC per this message, it becomes performance issue.  Chromium IMM32 implementation for IMR_QUERYCHARPOSITION seems to be use cache rect by ImeCompositionRangeChanged's IPC.

Comment 6

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

I agree with caching the rects only for composition string which may improve performance rather than we use synchronous communication between chrome and content. However, I still believe that we need "slower path" for unusual cases.

Comment 7

[Chris Peterson [:cpeterson]]

Makoto, the e10s team is preparing to enable e10s on the Nightly channel for a couple days of testing (maybe next week). Andreas is pushing to increase e10s testing soon, even with known issues such as lack of printing or a11y.

IME currently does not work with e10s, so we are discussing (in bug 1063669) whether we can disable e10s on systems using IME. As an alternative, Masayuki suggested that fixing this NS_QUERY_TEXT_RECT bug would let IME and e10s work well enough for most Nightly users. (Manually disabling e10s is always an option for them.)

Do you have time to investigate this bug next week? If you can fix this bug, do you think IME will work (well enough) that we wouldn't need to disable e10s?

Comment 8

[Makoto Kato [:m_kato]]

Attachment: Send each compositon character rect to chrome process

Comment 9

[Makoto Kato [:m_kato]]

Attachment: Send each compositon character rect to chrome process

Comment 10

[Makoto Kato [:m_kato]]

(In reply to Chris Peterson (:cpeterson) from comment #7)
> Makoto, the e10s team is preparing to enable e10s on the Nightly channel for
> a couple days of testing (maybe next week). Andreas is pushing to increase
> e10s testing soon, even with known issues such as lack of printing or a11y.
> 
> IME currently does not work with e10s, so we are discussing (in bug 1063669)
> whether we can disable e10s on systems using IME. As an alternative,
> Masayuki suggested that fixing this NS_QUERY_TEXT_RECT bug would let IME and
> e10s work well enough for most Nightly users. (Manually disabling e10s is
> always an option for them.)
> 
> Do you have time to investigate this bug next week? If you can fix this bug,
> do you think IME will work (well enough) that we wouldn't need to disable
> e10s?

If IMM32, IME already works even if this bug isn't fixed.  (but chararacter at position message doesn't work).   To set candidate window to correct position on TSF mode, this bug and EDITOR_RECT supports is required.

About EDITOR_RECT issue, I will work this weekend.  (It needs rebase and re-send review to masayuki).

Comment 11

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

Comment on attachment 8487852 [details] [diff] [review]
Send each compositon character rect to chrome process

>   /**
>+   * the offset of first composition string
>+   */
>+  uint32_t OffsetOfStartComposition() const { return mCompositionStartOffset; }
>+

The value counts linebreaks as native's. So, NativeOffsetOfStartComposition() is better for this users.

>   case NS_QUERY_CARET_RECT:
>     {
>-      if (aEvent.mInput.mOffset != mIMECompositionRectOffset) {
>+      if (aEvent.mInput.mOffset != mIMECaretOffset) {
>         break;
>       }
> 
>-      aEvent.mReply.mOffset = mIMECompositionRectOffset;
>+      aEvent.mReply.mOffset = mIMECaretOffset;
>       aEvent.mReply.mRect = mIMECaretRect - GetChildProcessOffset();
>       aEvent.mSucceeded = true;
>     }
>     break;

I don't think that this is the best. Anybody can query caret rect of any offsets. If there is no caret at the queried position, ContentEventHandler computes the caret rect from char rect.

So, I believe that we can improve this case. However, this bug is urgent and I guess that you don't find major issues with this patch. So, I'll work on it later in another bug. This is okay for now.

>@@ -520,31 +520,39 @@ PuppetWidget::NotifyIMEOfUpdateCompositi
> 
>   NS_ENSURE_TRUE(mTabChild, NS_ERROR_FAILURE);
> 
>   nsRefPtr<TextComposition> textComposition =
>     IMEStateManager::GetTextCompositionFor(this);
>   NS_ENSURE_TRUE(textComposition, NS_ERROR_FAILURE);
> 
>   nsEventStatus status;
>-  uint32_t offset = textComposition->OffsetOfTargetClause();
>-  WidgetQueryContentEvent textRect(true, NS_QUERY_TEXT_RECT, this);
>-  InitEvent(textRect, nullptr);
>-  textRect.InitForQueryTextRect(offset, 1);
>-  DispatchEvent(&textRect, status);
>-  NS_ENSURE_TRUE(textRect.mSucceeded, NS_ERROR_FAILURE);
>+  nsTArray<nsIntRect> textRectArray(textComposition->String().Length());

nsAutoTArray<nsIntRect, 10>?

>+  uint32_t startOffset = textComposition->OffsetOfStartComposition();
>+  uint32_t endOffset = textComposition->String().Length() + startOffset;
>+  for (uint32_t i = startOffset; i < endOffset; i++) {
>+    WidgetQueryContentEvent textRect(true, NS_QUERY_TEXT_RECT, this);
>+    InitEvent(textRect, nullptr);
>+    textRect.InitForQueryTextRect(i, 1);
>+    DispatchEvent(&textRect, status);
>+    NS_ENSURE_TRUE(textRect.mSucceeded, NS_ERROR_FAILURE);
> 
>+    textRectArray.AppendElement(textRect.mReply.mRect);
>+  }

I'm afraid about the performance of this but let's optimize it later if it's necessary. I guess that we should define NS_QUERY_EACH_CHAR_RECT.

And also I guess the rects can be cached by TextComposition.

Comment 12

[Chris Peterson [:cpeterson]]

Makoto: after you address Masayuki's review feedback, will your patch be ready to land? Does this bug need any follow-up patches? The e10s team is trying to close out our last few bugs for our M2 milestone.

If fixing this bug is good enough for Nightly testing of e10s, then bug 1063669 doesn't need to block our M2 milestone but this bug should.

Comment 13

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 9/13 - 9/15, JST) from comment #11)
> Comment on attachment 8487852 [details] [diff] [review]
> Send each compositon character rect to chrome process
> 
> >   /**
> >+   * the offset of first composition string
> >+   */
> >+  uint32_t OffsetOfStartComposition() const { return mCompositionStartOffset; }
> >+
> 
> The value counts linebreaks as native's. So,
> NativeOffsetOfStartComposition() is better for this users.
> 
> >   case NS_QUERY_CARET_RECT:
> >     {
> >-      if (aEvent.mInput.mOffset != mIMECompositionRectOffset) {
> >+      if (aEvent.mInput.mOffset != mIMECaretOffset) {
> >         break;
> >       }
> > 
> >-      aEvent.mReply.mOffset = mIMECompositionRectOffset;
> >+      aEvent.mReply.mOffset = mIMECaretOffset;
> >       aEvent.mReply.mRect = mIMECaretRect - GetChildProcessOffset();
> >       aEvent.mSucceeded = true;
> >     }
> >     break;
> 
> I don't think that this is the best. Anybody can query caret rect of any
> offsets. If there is no caret at the queried position, ContentEventHandler
> computes the caret rect from char rect.
> 
> So, I believe that we can improve this case. However, this bug is urgent and
> I guess that you don't find major issues with this patch. So, I'll work on
> it later in another bug. This is okay for now.

OK. I will file bug.

> 
> >@@ -520,31 +520,39 @@ PuppetWidget::NotifyIMEOfUpdateCompositi
> > 
> >   NS_ENSURE_TRUE(mTabChild, NS_ERROR_FAILURE);
> > 
> >   nsRefPtr<TextComposition> textComposition =
> >     IMEStateManager::GetTextCompositionFor(this);
> >   NS_ENSURE_TRUE(textComposition, NS_ERROR_FAILURE);
> > 
> >   nsEventStatus status;
> >-  uint32_t offset = textComposition->OffsetOfTargetClause();
> >-  WidgetQueryContentEvent textRect(true, NS_QUERY_TEXT_RECT, this);
> >-  InitEvent(textRect, nullptr);
> >-  textRect.InitForQueryTextRect(offset, 1);
> >-  DispatchEvent(&textRect, status);
> >-  NS_ENSURE_TRUE(textRect.mSucceeded, NS_ERROR_FAILURE);
> >+  nsTArray<nsIntRect> textRectArray(textComposition->String().Length());
> 
> nsAutoTArray<nsIntRect, 10>?

This array is fixed length.

> >+  uint32_t startOffset = textComposition->OffsetOfStartComposition();
> >+  uint32_t endOffset = textComposition->String().Length() + startOffset;
> >+  for (uint32_t i = startOffset; i < endOffset; i++) {
> >+    WidgetQueryContentEvent textRect(true, NS_QUERY_TEXT_RECT, this);
> >+    InitEvent(textRect, nullptr);
> >+    textRect.InitForQueryTextRect(i, 1);
> >+    DispatchEvent(&textRect, status);
> >+    NS_ENSURE_TRUE(textRect.mSucceeded, NS_ERROR_FAILURE);
> > 
> >+    textRectArray.AppendElement(textRect.mReply.mRect);
> >+  }
> 
> I'm afraid about the performance of this but let's optimize it later if it's
> necessary. I guess that we should define NS_QUERY_EACH_CHAR_RECT.
> 
> And also I guess the rects can be cached by TextComposition.

filed as bug 1067230

Comment 14

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00cc87d0fbf8

Comment 15

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 9/13 - 9/15, JST) from comment #11)
> >   case NS_QUERY_CARET_RECT:
> >     {
> >-      if (aEvent.mInput.mOffset != mIMECompositionRectOffset) {
> >+      if (aEvent.mInput.mOffset != mIMECaretOffset) {
> >         break;
> >       }
> > 
> >-      aEvent.mReply.mOffset = mIMECompositionRectOffset;
> >+      aEvent.mReply.mOffset = mIMECaretOffset;
> >       aEvent.mReply.mRect = mIMECaretRect - GetChildProcessOffset();
> >       aEvent.mSucceeded = true;
> >     }
> >     break;
> 
> I don't think that this is the best. Anybody can query caret rect of any
> offsets. If there is no caret at the queried position, ContentEventHandler
> computes the caret rect from char rect.
> 
> So, I believe that we can improve this case. However, this bug is urgent and
> I guess that you don't find major issues with this patch. So, I'll work on
> it later in another bug. This is okay for now.

Filed as Bug 1067236.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/00cc87d0fbf8