Implement remote NS_QUERY_EDITOR_RECT

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

(Blocks 1 bug)

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

Firefox Tracking Flags

(e10sm4+)

Details

Attachments

(4 attachments, 4 obsolete attachments)

need dor e10s tsf support
Assignee: nobody → m_kato
Blocks: e10s-ime-tsf
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.
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: --- → +
ah, I forget this!!!!.  I will rebase and review...
Blocks: 1050644
Can we land this and resolve this bug out?
Flags: needinfo?(m_kato)
Flags: needinfo?(m_kato)
Attachment #8517407 - Attachment is obsolete: true
Attachment #8517405 - Attachment is obsolete: true
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)
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+
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?
(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.
(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.
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!
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)
(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.
Attachment #8519809 - Attachment is obsolete: true
Attachment #8519810 - Attachment is obsolete: true
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+
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.
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+
Depends on: 1014554
You need to log in before you can comment on or make changes to this bug.