Implement remote NS_QUERY_EDITOR_RECT

RESOLVED FIXED in mozilla37

Status

()

Core
DOM: Events
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla37
x86
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm4+)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Assignee)

Description

5 years ago
need dor e10s tsf support
(Assignee)

Updated

5 years ago
Assignee: nobody → m_kato
Blocks: 960836
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?
Flags: needinfo?(roc)
FYI: If we can implement frame position change listener, we can get rid of high frequent timer for calling OnLayoutChange() in nsTextStore.
It's not that difficult to get a notification after every reflow or scroll operation. That should be good enough.
Flags: needinfo?(roc)
(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()?
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
> 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.
(Assignee)

Comment 7

4 years ago
Created attachment 8395555 [details] [diff] [review]
remote EDITOR_RECT
(Assignee)

Updated

4 years ago
Attachment #8395555 - Flags: review?(masayuki)
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
Attachment #8395555 - Flags: review?(masayuki) → review+
Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".
tracking-e10s: --- → +
Blocks: 997462
(Assignee)

Comment 11

4 years ago
ah, I forget this!!!!.  I will rebase and review...
Blocks: 1050644
tracking-e10s: + → m4+

Comment 12

4 years ago
Can we land this and resolve this bug out?
Flags: needinfo?(m_kato)
(Assignee)

Comment 13

4 years ago
Created attachment 8517405 [details] [diff] [review]
Part 1.  Implement remote NS_QUERY_EDITOR_RECT
Flags: needinfo?(m_kato)
(Assignee)

Comment 14

4 years ago
Created attachment 8517407 [details] [diff] [review]
Part 2. Requrie NOTIFY_POSITION_CHANGE
(Assignee)

Updated

4 years ago
Attachment #8517407 - Attachment is obsolete: true
(Assignee)

Comment 15

4 years ago
Created attachment 8519805 [details] [diff] [review]
Part 1. Implement remote NS_QUERY_EDITOR_RECT
Attachment #8517405 - Attachment is obsolete: true
(Assignee)

Comment 16

4 years ago
Created attachment 8519809 [details] [diff] [review]
Part 2. Add NotifyIME method in TabParent
(Assignee)

Comment 17

4 years ago
Created attachment 8519810 [details] [diff] [review]
Part 3. Child process should listen NOTIFY_IME_OF_POSITION_CHANGE
(Assignee)

Comment 18

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

implement simple NS_QUERY_EDITOR_RECT
Attachment #8519805 - Attachment description: Part 1. Implement remote NS_QUERY_EDITOR_RECT"editorrect → Part 1. Implement remote NS_QUERY_EDITOR_RECT
Attachment #8519805 - Flags: review?(masayuki)
(Assignee)

Comment 19

4 years ago
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.
Attachment #8519809 - Flags: review?(masayuki)
Comment on attachment 8519805 [details] [diff] [review]
Part 1. Implement remote NS_QUERY_EDITOR_RECT

Thanks!
Attachment #8519805 - Flags: review?(masayuki) → review+
(Assignee)

Comment 21

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

listen position change
Attachment #8519810 - Flags: review?(masayuki)
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.
Attachment #8519809 - Flags: review?(masayuki) → review+
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?
Do you plan part.4 and later? I don't see why you need part.2.
(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.
(Assignee)

Comment 26

4 years ago
(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.
(Assignee)

Comment 27

4 years ago
If NotifyIMEOfPositionChange() doesn't call NotifyIMEOfCompositionRects(), no message spam.
(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.
(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!
(Assignee)

Comment 30

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

I will update patch tomorrow.
Attachment #8519810 - Flags: review?(masayuki)
(Assignee)

Comment 31

4 years ago
(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.
(Assignee)

Comment 32

4 years ago
Created attachment 8524327 [details] [diff] [review]
Part 2. Child process should listen NOTIFY_IME_OF_POSITION_CHANGE
Attachment #8519809 - Attachment is obsolete: true
Attachment #8519810 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8524327 - Flags: review?(masayuki)
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.
Attachment #8524327 - Flags: review?(masayuki) → review+
(Assignee)

Comment 36

4 years ago
humm, when focus isn't text frame, it hits assertion by caret rect query...
Why does it happen only (?) in e10s mode? If so, child process queries caret rect before or after moving focus?
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.
(Assignee)

Comment 40

4 years ago
Created attachment 8543707 [details] [diff] [review]
Part 3. add expectAssertions for e10s
(Assignee)

Comment 41

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

use expectAssertions to avoid assertion issue on Linux.
Attachment #8543707 - Flags: review?(masayuki)
Comment on attachment 8543707 [details] [diff] [review]
Part 3. add expectAssertions for e10s

Sorry for the delay. And happy new year, Kato-san ;-)
Attachment #8543707 - Flags: review?(masayuki) → review+
https://hg.mozilla.org/mozilla-central/rev/2a997130c2e7
https://hg.mozilla.org/mozilla-central/rev/5e9067e7f6cd
https://hg.mozilla.org/mozilla-central/rev/8a01e921708c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37

Updated

3 years ago
Depends on: 1014554
You need to log in before you can comment on or make changes to this bug.