966157 - Implement remote NS_QUERY_EDITOR_RECT

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Makoto Kato [:m_kato]]

need dor e10s tsf support

Comment 1

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

Hmm, it seems this is difficult...

If there is a layout change notifier of a frame, we can cache it at that time. However, I think that it's pretty difficult to implement it because it needs to observe all ancestor frames since some of ancestors might move the editor frame by scroll.

Roc, how do you think about this?

Comment 2

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

FYI: If we can implement frame position change listener, we can get rid of high frequent timer for calling OnLayoutChange() in nsTextStore.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's not that difficult to get a notification after every reflow or scroll operation. That should be good enough.

Comment 4

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> It's not that difficult to get a notification after every reflow or scroll
> operation. That should be good enough.

I was thinking that when ScrollFrameHelper::AsyncScrollPortEvent::Run() is called,
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#3294

we can send notification with something new feature. But I have no idea for now. If we send notification each descendant of the scrollable frame, it probably causes making damage to the performance. But it access to nsIMEStateManager directly isn't smart. Let me know if you have good idea about this.

And is it enough to check only nsIFrame::Reflow() of the editor's root frame? Is there a case only one of ancestors changes child frame position without calling Reflow()?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Frame positions only change during Reflow and in ScrollFrameHelper::ScrollToImpl.

You only need to track the geometry of the focused element, right? So a single "frame geometry in this document changed" callback to nsIMEStateManager, called after reflow and after ScrollToImpl, should be pretty cheap, since you just need to save the new position of the focused element and return.

Comment 6

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

> You only need to track the geometry of the focused element, right?

Indeed. Thank you, it's very helpful to me. I'll file a new bug for implementing it and I'll request review to you.

Comment 7

[Makoto Kato [:m_kato]]

Attachment: remote EDITOR_RECT

Comment 8

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

Comment on attachment 8395555 [details] [diff] [review]
remote EDITOR_RECT

>@@ -1329,16 +1348,22 @@ TabParent::HandleQueryContentEvent(Widge
>         break;
>       }
> 
>       aEvent.mReply.mOffset = mIMECompositionRectOffset;
>       aEvent.mReply.mRect = mIMECaretRect - GetChildProcessOffset();
>       aEvent.mSucceeded = true;
>     }
>     break;
>+  case NS_QUERY_EDITOR_RECT:
>+    {
>+      aEvent.mReply.mRect = mIMEEditorRect - GetChildProcessOffset();
>+      aEvent.mSucceeded = true;
>+    }
>+    break;

I guess that you need to update the comment above this method.

>+template<>
>+struct ParamTraits<mozilla::widget::IMEMessage>
>+{
>+  typedef mozilla::widget::IMEMessage paramType;
>+
>+  static void Write(Message* aMsg, const paramType& aParam)
>+  {
>+    WriteParam(aMsg, static_cast<uint8_t>(aParam));
>+  }
>+
>+  static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
>+  {
>+    uint8_t message;
>+    if (!ReadParam(aMsg, aIter, &message)) {
>+      return false;
>+    }
>+    *aResult = static_cast<mozilla::widget::IMEMessage>(message);
>+    return true;
>+  }
>+};

IMEMessage is currently, int8_t... I think that we should do "typedef int8_t IMEMessageInt;" above the definition of IMEMessage in nsIWidget.h and use it in MOZ_ENUM_TYPE() and here, like this:
http://mxr.mozilla.org/mozilla-central/source/widget/EventForwards.h#50

Comment 9

[Chris Peterson [:cpeterson]]

Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".

Comment 11

[Makoto Kato [:m_kato]]

ah, I forget this!!!!.  I will rebase and review...

Comment 12

[Jim Mathies [:jimm]]

Can we land this and resolve this bug out?

Comment 13

[Makoto Kato [:m_kato]]

Attachment: Part 1. Implement remote NS_QUERY_EDITOR_RECT

Comment 14

[Makoto Kato [:m_kato]]

Attachment: Part 2. Requrie NOTIFY_POSITION_CHANGE

Comment 15

[Makoto Kato [:m_kato]]

Attachment: Part 1. Implement remote NS_QUERY_EDITOR_RECT

Comment 16

[Makoto Kato [:m_kato]]

Attachment: Part 2. Add NotifyIME method in TabParent

Comment 17

[Makoto Kato [:m_kato]]

Attachment: Part 3. Child process should listen NOTIFY_IME_OF_POSITION_CHANGE

Comment 18

[Makoto Kato [:m_kato]]

Comment on attachment 8519805 [details] [diff] [review]
Part 1. Implement remote NS_QUERY_EDITOR_RECT

implement simple NS_QUERY_EDITOR_RECT

Comment 19

[Makoto Kato [:m_kato]]

Comment on attachment 8519809 [details] [diff] [review]
Part 2. Add NotifyIME method in TabParent

for ime simple events of e10s, we shouldn't create e10s method/APIs per commands.  I want to create simple reply API for IME notification.

Comment 20

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

Comment on attachment 8519805 [details] [diff] [review]
Part 1. Implement remote NS_QUERY_EDITOR_RECT

Thanks!

Comment 21

[Makoto Kato [:m_kato]]

Comment on attachment 8519810 [details] [diff] [review]
Part 3. Child process should listen NOTIFY_IME_OF_POSITION_CHANGE

listen position change

Comment 22

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

Comment on attachment 8519809 [details] [diff] [review]
Part 2. Add NotifyIME method in TabParent

I guess that you will use this actually. If you forgot to include PuppetWidget part, please request review again.

Comment 23

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

Comment on attachment 8519810 [details] [diff] [review]
Part 3. Child process should listen NOTIFY_IME_OF_POSITION_CHANGE

> nsresult
>-PuppetWidget::NotifyIMEOfUpdateComposition()
>+PuppetWidget::NotifyIMEOfCompositionRects()

>   nsRefPtr<TextComposition> textComposition =
>     IMEStateManager::GetTextCompositionFor(this);
>-  NS_ENSURE_TRUE(textComposition, NS_ERROR_FAILURE);
>+  if (!textComposition) {
>+    return NS_OK;
>+  }

Could you explain with comment when we hit this?

>+nsresult
>+PuppetWidget::NotifyIMEOfUpdateComposition(
>+                const IMENotification& aIMENotification)
>+{
>+  NotifyIMEOfCompositionRects();
>+
>+  NS_ENSURE_TRUE(mTabChild, NS_ERROR_FAILURE);
>+
>+  if (!mTabChild->SendNotifyIME(aIMENotification)) {
>+    return NS_ERROR_FAILURE;
>+  }
>   return NS_OK;
> }

nit: Why do you need to call NotifyIMEOfCompositionRects() when mTabChild is null?

>@@ -574,17 +593,18 @@ PuppetWidget::NotifyIMEOfEditorRect()
> 
> nsIMEUpdatePreference
> PuppetWidget::GetIMEUpdatePreference()
> {
> #ifdef MOZ_CROSS_PROCESS_IME
>   // e10s requires IME information cache into TabParent
>   return nsIMEUpdatePreference(mIMEPreferenceOfParent.mWantUpdates |
>                                nsIMEUpdatePreference::NOTIFY_SELECTION_CHANGE |
>-                               nsIMEUpdatePreference::NOTIFY_TEXT_CHANGE);
>+                               nsIMEUpdatePreference::NOTIFY_TEXT_CHANGE |
>+                               nsIMEUpdatePreference::NOTIFY_POSITION_CHANGE );

Although, we should reduce NOTIFY_POSITION_CHANGE count for performance later...

>+nsresult
>+PuppetWidget::NotifyIMEOfPositionChange(
>+                const IMENotification& aIMENotification)
>+{
>+#ifndef MOZ_CROSS_PROCESS_IME
>+  return NS_OK;
>+#endif
>+
>+  NotifyIMEOfEditorRect();
>+  NotifyIMEOfCompositionRects();

Hmm, this causes run IPC twice (and +1 below). Cannot optimize them to a communication?

>+
>+  if (mIMEPreferenceOfParent.WantPositionChanged()) {
>+    if (!mTabChild) {
>+      return NS_ERROR_FAILURE;
>+    }
>+    if (!mTabChild->SendNotifyIME(aIMENotification)) {
>+      return NS_ERROR_FAILURE;
>+    }

Why don't you use NS_WARN_IF() for these failures?

Comment 24

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

Do you plan part.4 and later? I don't see why you need part.2.

Comment 25

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #24)
> Do you plan part.4 and later? I don't see why you need part.2.

Oops, you use it in PuppetWidget::NotifyIMEOfPositionChange(), sorry for the spam.

Comment 26

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #23)
> Comment on attachment 8519810 [details] [diff] [review]
> Part 3. Child process should listen NOTIFY_IME_OF_POSITION_CHANGE
> 
> > nsresult
> >-PuppetWidget::NotifyIMEOfUpdateComposition()
> >+PuppetWidget::NotifyIMEOfCompositionRects()
> 
> >   nsRefPtr<TextComposition> textComposition =
> >     IMEStateManager::GetTextCompositionFor(this);
> >-  NS_ENSURE_TRUE(textComposition, NS_ERROR_FAILURE);
> >+  if (!textComposition) {
> >+    return NS_OK;
> >+  }
> 
> Could you explain with comment when we hit this?

To avoid warning spam.  If no composition string, textComposition will be null.

> >+nsresult
> >+PuppetWidget::NotifyIMEOfUpdateComposition(
> >+                const IMENotification& aIMENotification)
> >+{
> >+  NotifyIMEOfCompositionRects();
> >+
> >+  NS_ENSURE_TRUE(mTabChild, NS_ERROR_FAILURE);
> >+
> >+  if (!mTabChild->SendNotifyIME(aIMENotification)) {
> >+    return NS_ERROR_FAILURE;
> >+  }
> >   return NS_OK;
> > }
> 
> nit: Why do you need to call NotifyIMEOfCompositionRects() when mTabChild is
> null?

I should change it.

> >@@ -574,17 +593,18 @@ PuppetWidget::NotifyIMEOfEditorRect()
> > 
> > nsIMEUpdatePreference
> > PuppetWidget::GetIMEUpdatePreference()
> > {
> > #ifdef MOZ_CROSS_PROCESS_IME
> >   // e10s requires IME information cache into TabParent
> >   return nsIMEUpdatePreference(mIMEPreferenceOfParent.mWantUpdates |
> >                                nsIMEUpdatePreference::NOTIFY_SELECTION_CHANGE |
> >-                               nsIMEUpdatePreference::NOTIFY_TEXT_CHANGE);
> >+                               nsIMEUpdatePreference::NOTIFY_TEXT_CHANGE |
> >+                               nsIMEUpdatePreference::NOTIFY_POSITION_CHANGE );
> 
> Although, we should reduce NOTIFY_POSITION_CHANGE count for performance
> later...
> 
> >+nsresult
> >+PuppetWidget::NotifyIMEOfPositionChange(
> >+                const IMENotification& aIMENotification)
> >+{
> >+#ifndef MOZ_CROSS_PROCESS_IME
> >+  return NS_OK;
> >+#endif
> >+
> >+  NotifyIMEOfEditorRect();
> >+  NotifyIMEOfCompositionRects();
> 
> Hmm, this causes run IPC twice (and +1 below). Cannot optimize them to a
> communication?

My concern is that number of IPC method will increment.  If no problem, I will create new method to TabParent and remove Part 2. patch.

Also, I should optimize position change message because it is fired by selection change too.  It will work as bug 1096220.

> >+
> >+  if (mIMEPreferenceOfParent.WantPositionChanged()) {
> >+    if (!mTabChild) {
> >+      return NS_ERROR_FAILURE;
> >+    }
> >+    if (!mTabChild->SendNotifyIME(aIMENotification)) {
> >+      return NS_ERROR_FAILURE;
> >+    }
> 
> Why don't you use NS_WARN_IF() for these failures?

OK, I will use it.

Comment 27

[Makoto Kato [:m_kato]]

If NotifyIMEOfPositionChange() doesn't call NotifyIMEOfCompositionRects(), no message spam.

Comment 28

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

(In reply to Makoto Kato (:m_kato) from comment #27)
> If NotifyIMEOfPositionChange() doesn't call NotifyIMEOfCompositionRects(),
> no message spam.

Yeah, it might be better to add IMENotification::mLayoutChangeData::mCausedByComposition.

Then, IMENotification::IsCausedByComposition() is available with NOTIFY_IME_OF_POSITION_CHANGE.

You can set it in PositionChangeEvent of IMEContentObserver.cpp. You can refer SelectionChangeEvent how to do it.

Comment 29

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

(In reply to Makoto Kato (:m_kato) from comment #26)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #23)
> > >+nsresult
> > >+PuppetWidget::NotifyIMEOfPositionChange(
> > >+                const IMENotification& aIMENotification)
> > >+{
> > >+#ifndef MOZ_CROSS_PROCESS_IME
> > >+  return NS_OK;
> > >+#endif
> > >+
> > >+  NotifyIMEOfEditorRect();
> > >+  NotifyIMEOfCompositionRects();
> > 
> > Hmm, this causes run IPC twice (and +1 below). Cannot optimize them to a
> > communication?
> 
> My concern is that number of IPC method will increment.  If no problem, I
> will create new method to TabParent and remove Part 2. patch.

Sorry, I don't understand. Do you want to merge them? If so, I agree with the approach. But does that decrease the count of cross process communication?

> Also, I should optimize position change message because it is fired by
> selection change too.  It will work as bug 1096220.

Thanks!

Comment 30

[Makoto Kato [:m_kato]]

Comment on attachment 8519810 [details] [diff] [review]
Part 3. Child process should listen NOTIFY_IME_OF_POSITION_CHANGE

I will update patch tomorrow.

Comment 31

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #28)
> Yeah, it might be better to add
> IMENotification::mLayoutChangeData::mCausedByComposition.
> 
> Then, IMENotification::IsCausedByComposition() is available with
> NOTIFY_IME_OF_POSITION_CHANGE.
> 
> You can set it in PositionChangeEvent of IMEContentObserver.cpp. You can
> refer SelectionChangeEvent how to do it.

Since this spam can be removed by bug 1096220, we don't need modification for warning spam.

Comment 32

[Makoto Kato [:m_kato]]

Attachment: Part 2. Child process should listen NOTIFY_IME_OF_POSITION_CHANGE

Comment 33

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

Comment on attachment 8524327 [details] [diff] [review]
Part 2. Child process should listen NOTIFY_IME_OF_POSITION_CHANGE

> bool
>+TabParent::RecvNotifyIMEPositionChange(
>+             const nsIntRect& aEditorRect,
>+             const InfallibleTArray<nsIntRect>& aCompositionRects,
>+             const nsIntRect& aCaretRect)
>+{
>+  mIMEEditorRect = aEditorRect;
>+  mIMECompositionRects = aCompositionRects;
>+  mIMECaretRect = aCaretRect;
>+
>+  nsCOMPtr<nsIWidget> widget = GetWidget();
>+  if (!widget) {
>+    return true;
>+  }
>+
>+  const nsIMEUpdatePreference updatePreference =
>+    widget->GetIMEUpdatePreference();
>+  if (updatePreference.WantPositionChanged()) {
>+    widget->NotifyIME(IMENotification(NOTIFY_IME_OF_POSITION_CHANGE));
>+  }

Although, it's not a scope of this bug, it may not make sense to check update preference from TabParent because nsIWidget::GetIMEUpdatePreference() is also a virtual method. So, if it wants a notification, two virtual calls are necessary. Ignoring in widget might be better.


>+  uint32_t startOffset;
>+  uint32_t targetCauseOffset;
>+  nsTArray<nsIntRect> textRectArray;

Why don't you use nsAutoTArray? Would it cause compile error?

>+bool
>+PuppetWidget::GetCompositionRects(uint32_t& aStartOffset,
>+                                  nsTArray<nsIntRect>& aTextRectArray,
>+                                  uint32_t& aTargetCauseOffset)

Perhaps, we should create optimized query event for this later.

> nsresult
> PuppetWidget::NotifyIMEOfEditorRect()
> {
> #ifndef MOZ_CROSS_PROCESS_IME
>   return NS_OK;
> #endif
>+  NS_ENSURE_TRUE(mTabChild, NS_ERROR_FAILURE);
> 
>+  nsIntRect rect;
>+  if (!GetEditorRect(rect)) {

Perhaps, using NS_WARN_IF() is better?

>+bool
>+PuppetWidget::GetEditorRect(nsIntRect& aRect)
>+{
>   nsEventStatus status;
>   WidgetQueryContentEvent editorRectEvent(true, NS_QUERY_EDITOR_RECT, this);
>   InitEvent(editorRectEvent);
>   DispatchEvent(&editorRectEvent, status);
>-  if (editorRectEvent.mSucceeded) {
>-    mTabChild->SendNotifyIMEEditorRect(editorRectEvent.mReply.mRect);
>+  if (!editorRectEvent.mSucceeded) {
>+    return false;

ditto.

>+nsresult
>+PuppetWidget::NotifyIMEOfPositionChange()
>+{
>+#ifndef MOZ_CROSS_PROCESS_IME
>+  return NS_OK;
>+#endif
>+  NS_ENSURE_TRUE(mTabChild, NS_ERROR_FAILURE);
>+
>+  nsIntRect editorRect;
>+  if (!GetEditorRect(editorRect)) {
>+    return NS_ERROR_FAILURE;

ditto.

Comment 34

[Makoto Kato [:m_kato]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/eb6981f3c136
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/212f295a955d

Comment 35

[Carsten Book [:Tomcat]]

sorry had to backout for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4644297&repo=mozilla-inbound

Comment 36

[Makoto Kato [:m_kato]]

humm, when focus isn't text frame, it hits assertion by caret rect query...

Comment 37

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

Why does it happen only (?) in e10s mode? If so, child process queries caret rect before or after moving focus?

Comment 38

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

Oh, looks hacky test...
http://mxr.mozilla.org/mozilla-central/source/layout/base/tests/bug558663.html?force=1#40

Comment 39

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

Although, I don't understand the assertion is kicked not always. However, in this case, the assertion is wrong. It's possible that there is no text frame in an editor. So, I think that it's okay to ignore the assertion with setting assertion count 0 - 2. It's a bug of ContentEventHandler.

Comment 40

[Makoto Kato [:m_kato]]

Attachment: Part 3. add expectAssertions for e10s

Comment 41

[Makoto Kato [:m_kato]]

Comment on attachment 8543707 [details] [diff] [review]
Part 3. add expectAssertions for e10s

use expectAssertions to avoid assertion issue on Linux.

Comment 42

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

Comment on attachment 8543707 [details] [diff] [review]
Part 3. add expectAssertions for e10s

Sorry for the delay. And happy new year, Kato-san ;-)

Comment 43

[Makoto Kato [:m_kato]]

  https://hg.mozilla.org/integration/mozilla-inbound/rev/2a997130c2e7
  https://hg.mozilla.org/integration/mozilla-inbound/rev/5e9067e7f6cd
  https://hg.mozilla.org/integration/mozilla-inbound/rev/8a01e921708c

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a997130c2e7
https://hg.mozilla.org/mozilla-central/rev/5e9067e7f6cd
https://hg.mozilla.org/mozilla-central/rev/8a01e921708c