Closed Bug 1176954 Opened 4 years ago Closed 4 years ago

[e10s][TSF][GTK?] TabParent shouldn't notify IME until all sending events are handled

Categories

(Core :: DOM: Content Processes, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
e10s + ---
firefox41 --- affected
firefox42 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 2 open bugs)

Details

(Keywords: inputmethod)

Attachments

(8 files, 8 obsolete files)

4.54 KB, patch
smaug
: review+
Details | Diff | Splinter Review
15.55 KB, patch
smaug
: review+
Details | Diff | Splinter Review
19.81 KB, patch
Details | Diff | Splinter Review
4.73 KB, patch
Details | Diff | Splinter Review
40.98 KB, patch
Details | Diff | Splinter Review
68.74 KB, patch
Details | Diff | Splinter Review
8.00 KB, patch
Details | Diff | Splinter Review
7.15 KB, patch
smaug
: review+
Details | Diff | Splinter Review
If content process is busy, TabParent may send two or more events before the first sending event is handled.

Then, if TabParent notifies IME of text change or something at the first event handled, native IME expected that all sending events are handled. So, if IME tries to query content, TabParent still has old content which only handles the first event.

For preventing this issue, TabParent should store sending events and when it's handled by editor, it should be notified that and remove the handled event from the queue. Then, all events are handled, start notifying IME.
For managing pending event count in TabParent, TabChild should notify TabParent of receiving an event. (Although, it could be synchronous.)
Attachment #8631569 - Flags: review?(bugs)
ContentCache in TabParent should count the number of pending event which are sent but not received yet.
Attachment #8631570 - Flags: review?(bugs)
When there is no pending event, ContentCache should notify IME of pending notifications. I think that only selection change, text change and composition update is enough. Even if it's not enough, we can append other notifications such as position change easy.

Note that this patch doesn't merge multiple text change ranges before sending a text change notification. It will be fixed by following patches.
Attachment #8631572 - Flags: review?(bugs)
Currently, we have the code of merging text change ranges in IMEContentObserver. But we should move it into IMENotification.

This patch names the struct of mTextChangeData as TextChangeDataBase. We need constructor for that later, but structs which have constructor cannot be a member of union. Therefore, its name has "Base" postfix.

And its members should be same name as IMEContentObserver::TextChangeData which were named by you ;-)

The comment of each member are just copied from IMEContentObserver::TextChangeData definition.
Attachment #8631573 - Flags: review?(bugs)
Instead of |bool mStored|, we can use mStartOffset, mRemovedEndOffset and mAddedEndOffset for indicating if the members are initialized. mStartOffset must be same or less than the others. So, setting mStartOffset UINT32_MAX and m*EndOffset 0 can indicate "uninitialized".

This state is necessary when merging with another instance.
Attachment #8631576 - Flags: review?(bugs)
Now, we can drop IMEContentObserver::TextChangeData and make it use IMENotification::TextChangeData(Base).
Attachment #8631578 - Flags: review?(bugs)
Finally, we can move merge method and test method of that into IMENotification::TextChangeDataBase. Then, we can fix the pending issue of part.3.
Attachment #8631579 - Flags: review?(bugs)
Comment on attachment 8631569 [details] [diff] [review]
part.1 Child process should notify its parent process when it dispatches composition or selection event into the DOM tree

(it is not clear to me why any of IME stuff needs to use prio(urgent), but that is a separate bug)


OnEventReceived() is way too generic name. What event? DOM Event, something else?

I think the cleanest API would be to explicitly have
OnSeclectionEventReceived()/OnCompositionEventReceived() in PBrowser. Sure, it is a bit redundant, but makes code easier to understand.
Or otherwise hint in the name that it is just about some particular events.
Attachment #8631569 - Flags: review?(bugs) → review-
Comment on attachment 8631570 [details] [diff] [review]
part.2 ContentCache in parent process should manage how many events are sent but not yet received

So I would add the OnFooEventReceived also to ContentCacheInParent, and then
both implementations would just --mPendingEvents;

And perhaps mPendingEvents could be called mPendingCompositionAndSelectionEvents;
A bit long, but clear.

With those, r+
Attachment #8631570 - Flags: review?(bugs) → review+
Comment on attachment 8631572 [details] [diff] [review]
part.3 Don't send selection change, text change nor composition update notification to IME from TabParent until all events sent to the child process is received by it

> TabParent::RecvOnEventReceived()
> {
>-  mContentCache.OnEventReceived();
>+  nsCOMPtr<nsIWidget> widget = GetWidget();
>+  if (!widget) {
>+    return true;
>+  }
>+
>+  // While calling OnEventReceived(), TabParent *might* be destroyed since
>+  // it may send notifications to IME.
>+  nsRefPtr<TabParent> kungFuDeathGrip(this);
>+  mContentCache.OnEventReceived(widget);
>   return true;
> }
So this could be in those RecvOnFooEventReceived() methods, or in some helper



>+ContentCacheInParent::OnEventReceived(nsIWidget* aWidget)
> {
>   MOZ_LOG(sContentCacheLog, LogLevel::Info,
>-    ("ContentCacheInParent: 0x%p OnEventReceived(), mPendingEvents=%u",
>-     this, mPendingEvents));
>+    ("ContentCacheInParent: 0x%p OnEventReceived(aWidget=0x%p), "
>+     "mPendingEvents=%u",
>+     this, aWidget, mPendingEvents));
> 
>   MOZ_RELEASE_ASSERT(mPendingEvents > 0);
>-  mPendingEvents--;
>+  if (--mPendingEvents) {
>+    return;
>+  }
>+
>+  FlushPendingNotifications(aWidget);
> }
And this too... 
Hmm, I'm not quite sure what would be cleanest setup for this all, and least error prone, so not code duplication.
OnEventReceived is too generic. Perhaps add some enum
EventTypeNeedingAck and then pass that to SendOnEventReceived and so on.
It wouldn't be useful for anything else bug debugging, and making code more readable.
Or do you have better ideas?

>+ContentCacheInParent::FlushPendingNotifications(nsIWidget* aWidget)
>+{
>+  MOZ_ASSERT(!mPendingEvents);
>+
>+  // New notifications which are notified during flushing pending notifications
>+  // should be merged again.
>+  mPendingEvents++;
>+
>+  nsCOMPtr<nsIWidget> kungFuDeathGrip(aWidget);
>+
>+  if (mPendingTextChange.HasNotification()) {
>+    IMENotification notification(mPendingTextChange);
>+    if (!aWidget->Destroyed()) {
>+      mPendingTextChange.Clear();
>+      IMEStateManager::NotifyIME(notification, aWidget, true);
>+    }
>+  }
>+
>+  if (mPendingSelectionChange.HasNotification()) {
>+    IMENotification notification(mPendingSelectionChange);
>+    if (!aWidget->Destroyed()) {
>+      mPendingSelectionChange.Clear();
>+      IMEStateManager::NotifyIME(notification, aWidget, true);
>+    }
>+  }
>+
>+  if (mPendingCompositionUpdate.HasNotification()) {
>+    IMENotification notification(mPendingCompositionUpdate);
>+    if (!aWidget->Destroyed()) {
>+      mPendingCompositionUpdate.Clear();
>+      IMEStateManager::NotifyIME(notification, aWidget, true);
>+    }
>+  }

Why do we do these in this order? Shouldn't we keep the right order?
Attachment #8631572 - Flags: review?(bugs) → review-
Attachment #8631573 - Flags: review?(bugs) → review+
Comment on attachment 8631576 [details] [diff] [review]
part.5 IMENotification::TextChangeDataBase should have a state which indicates that it's not initialized

IsStored sounds a bit odd to my non-native-English speaker's ears ;)
Perhaps IsValid()
Attachment #8631576 - Flags: review?(bugs) → review+
Comment on attachment 8631578 [details] [diff] [review]
part.6 IMEContentObserver should use IMENotification::TextChangeData

Hmm,  mCausedOnlyByComposition changes to mCausedByComposition.
I guess that is ok.

s/IsStored()/IsValid()/ here too.
I don't feel too strongly about that, but IMO IsValid() is more clear.


Why do we need struct TextChangeData? It doesn't seem to add anything useful to
TextChangeDataBase.
Maybe the next patch explains. tentative r+, assuming TextChangeData is actually useful.
Attachment #8631578 - Flags: review?(bugs) → review+
> EventTypeNeedingAck and then pass that to SendOnEventReceived and so on.
> It wouldn't be useful for anything else bug debugging, and making code more readable.
> Or do you have better ideas?

How about to send WidgetGUIEvent or just its message?
Flags: needinfo?(bugs)
Comment on attachment 8631578 [details] [diff] [review]
part.6 IMEContentObserver should use IMENotification::TextChangeData

no, I don't understand struct TextChangeData. The next patch adds something to
*Base, but not to TextChangeData. Why we need TextChangeData?
Why not just TextChangeDataBase everywhere (and rename it to TextChangeData)
Attachment #8631578 - Flags: review+ → review-
>>+ContentCacheInParent::FlushPendingNotifications(nsIWidget* aWidget)
>>+{
>>+  MOZ_ASSERT(!mPendingEvents);
>>+
>>+  // New notifications which are notified during flushing pending notifications
>>+  // should be merged again.
>>+  mPendingEvents++;
>>+
>>+  nsCOMPtr<nsIWidget> kungFuDeathGrip(aWidget);
>>+
>>+  if (mPendingTextChange.HasNotification()) {
>>+    IMENotification notification(mPendingTextChange);
>>+    if (!aWidget->Destroyed()) {
>>+      mPendingTextChange.Clear();
>>+      IMEStateManager::NotifyIME(notification, aWidget, true);
>>+    }
>>+  }
>>+
>>+  if (mPendingSelectionChange.HasNotification()) {
>>+    IMENotification notification(mPendingSelectionChange);
>>+    if (!aWidget->Destroyed()) {
>>+      mPendingSelectionChange.Clear();
>>+      IMEStateManager::NotifyIME(notification, aWidget, true);
>>+    }
>>+  }
>>+
>>+  if (mPendingCompositionUpdate.HasNotification()) {
>>+    IMENotification notification(mPendingCompositionUpdate);
>>+    if (!aWidget->Destroyed()) {
>>+      mPendingCompositionUpdate.Clear();
>>+      IMEStateManager::NotifyIME(notification, aWidget, true);
>>+    }
>>+  }
> 
> Why do we do these in this order? Shouldn't we keep the right order?

Yes. New selection range which is notified by selection change notification is in the latest text content. Therefore, text change notification must be notified before that.

Composition update notification indicates that the dispatched events are completely handled including sending notifications. So, this must be the last. Therefore, only this order is right.
(In reply to Olli Pettay [:smaug] from comment #14)
> Comment on attachment 8631578 [details] [diff] [review]
> part.6 IMEContentObserver should use IMENotification::TextChangeData
> 
> no, I don't understand struct TextChangeData. The next patch adds something
> to
> *Base, but not to TextChangeData. Why we need TextChangeData?
> Why not just TextChangeDataBase everywhere (and rename it to TextChangeData)

Because constructor guarantees to clear the data if it's used as a member of a class. And also it's used by creating temporary instance with specific ranges in TextChangeDataBase::Test().
Why can't TextChangeDataBase ctor guarantee to clear the data? And same with specific ranges.
Why can't all the functionality, which isn't much, moved to the *Base?
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #17)
> Why can't TextChangeDataBase ctor guarantee to clear the data? And same with
> specific ranges.
> Why can't all the functionality, which isn't much, moved to the *Base?

Because it's used in union of IMENotification. Any structs/classes cannot have constructor in union.
Comment on attachment 8631578 [details] [diff] [review]
part.6 IMEContentObserver should use IMENotification::TextChangeData

I see. Ok, add a comment why we need TextChangeData..
Attachment #8631578 - Flags: review- → review+
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #13)
> > EventTypeNeedingAck and then pass that to SendOnEventReceived and so on.
> > It wouldn't be useful for anything else bug debugging, and making code more readable.
> > Or do you have better ideas?
> 
> How about to send WidgetGUIEvent or just its message?
Well, if it is used just for these two types of events, it feels odd if some random message would be sent. Whoever reads the code has hard time to understand which events need OnEventReceived.
That is the concern I have - how to make the code easier to read.
If we're pretty sure it will be needed only for Composition or Selection events, 
perhaps OnCompositionOrSelectionEventReceived() or some such?
(In reply to Olli Pettay [:smaug] from comment #20)
> perhaps OnCompositionOrSelectionEventReceived() or some such?

Yeah, my first patch used such name, but I worry about that in the future, if we need to send the notification for the other events too, the name becomes odd (or needs to change longer).

So, I think that if the method takes WidgetGUIEvent or its message, we can do check the message in RecvOnEventReceived(). Then, I believe that it's enough easy to read.

E.g.,

TabParent::RecvOnEventReceived(const WidgetGUIEvent& aEvent)
{
  switch (aEvent.message) {
    case NS_COMPOSITION_START:
    case NS_COMPOSITION_CHANGE:
    case NS_COMPOSITION_COMMIT:
    case NS_COMPOSITION_COMMIT_AS_IS:
    case NS_SELECTION_SET:
      mContentCache.OnEventReceived();
      break;
    default:
      break;
  }
}

# Or, this switch statement should be in mContentCache.OnEventReceived().
Flags: needinfo?(bugs)
is OnEventNeedingAckReceived too long?
Flags: needinfo?(bugs)
Attachment #8631579 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #22)
> is OnEventNeedingAckReceived too long?

No, I don't think so, but is it clear to the other developers? Isn't it still unclear what events are notified by the method? Or is it enough?
Well, then some comment somewhere. The "*NeedingAck" is IMO rather clear that it isn't some generic
method which should be called for all the events, but only for some special ones.
Oops, I forget to call ContentCacheInParent::OnSelectionEvent() from TabParent::SendSelectionEvent().

Just a mistake, carrying over the r+.
Attachment #8631934 - Attachment is obsolete: true
Attachment #8632020 - Flags: review?(bugs) → review+
Attachment #8631923 - Flags: review?(bugs) → review+
Comment on attachment 8631937 [details] [diff] [review]
part.3 Don't send selection change, text change nor composition update notification to IME from TabParent until all events sent to the child process is received by it

>+ContentCacheInParent::MaybeNotifyIME(nsIWidget* aWidget,
>+                                     IMENotification& aNotification)
> {
>-  if (NS_WARN_IF(aNotification.mMessage != NOTIFY_IME_OF_SELECTION_CHANGE)) {
>+  if (aNotification.mMessage == NOTIFY_IME_OF_SELECTION_CHANGE) {
>+    aNotification.mSelectionChangeData.mOffset = mSelection.StartOffset();
>+    aNotification.mSelectionChangeData.mLength = mSelection.Length();
>+    aNotification.mSelectionChangeData.mReversed = mSelection.Reversed();
>+    aNotification.mSelectionChangeData.SetWritingMode(mSelection.mWritingMode);
>+  }
This is a bit odd that one mMessage type is initialized here but not others.
But I guess it is fine, given that InitNotification does that without the patch.


>+ContentCacheInParent::FlushPendingNotifications(nsIWidget* aWidget)
>+{
>+  MOZ_ASSERT(!mPendingEventsNeedingAck);
>+
>+  // New notifications which are notified during flushing pending notifications
>+  // should be merged again.
>+  mPendingEventsNeedingAck++;
>+
>+  nsCOMPtr<nsIWidget> kungFuDeathGrip(aWidget);
>+
>+  if (mPendingTextChange.HasNotification()) {
>+    IMENotification notification(mPendingTextChange);
>+    if (!aWidget->Destroyed()) {
>+      mPendingTextChange.Clear();
>+      IMEStateManager::NotifyIME(notification, aWidget, true);
>+    }
>+  }
>+
>+  // First, text change notification should be sent because selection change
>+  // notification notifies IME of current selection range in the latest content.
>+  // So, IME may need the latest content before that.
Shouldn't this comment be above the previous 'if', since this talks mostly about textchange

>+  if (mPendingSelectionChange.HasNotification()) {
>+    IMENotification notification(mPendingSelectionChange);
>+    if (!aWidget->Destroyed()) {
>+      mPendingSelectionChange.Clear();
>+      IMEStateManager::NotifyIME(notification, aWidget, true);
>+    }
>+  }
>+
>+  if (mPendingCompositionUpdate.HasNotification()) {
>+    IMENotification notification(mPendingCompositionUpdate);
>+    if (!aWidget->Destroyed()) {
>+      mPendingCompositionUpdate.Clear();
>+      IMEStateManager::NotifyIME(notification, aWidget, true);
>+    }
>+  }
>+
>+  // Finally, send composition update notification because it notifies IME of
>+  // finishing handling whole sending events.
And this calks about compositionupdate, which is done above, so perhaps move also this to be above the
previous 'if'
Attachment #8631937 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #36)
> Comment on attachment 8631937 [details] [diff] [review]
> part.3 Don't send selection change, text change nor composition update
> notification to IME from TabParent until all events sent to the child
> process is received by it
> 
> >+ContentCacheInParent::MaybeNotifyIME(nsIWidget* aWidget,
> >+                                     IMENotification& aNotification)
> > {
> >-  if (NS_WARN_IF(aNotification.mMessage != NOTIFY_IME_OF_SELECTION_CHANGE)) {
> >+  if (aNotification.mMessage == NOTIFY_IME_OF_SELECTION_CHANGE) {
> >+    aNotification.mSelectionChangeData.mOffset = mSelection.StartOffset();
> >+    aNotification.mSelectionChangeData.mLength = mSelection.Length();
> >+    aNotification.mSelectionChangeData.mReversed = mSelection.Reversed();
> >+    aNotification.mSelectionChangeData.SetWritingMode(mSelection.mWritingMode);
> >+  }
> This is a bit odd that one mMessage type is initialized here but not others.
> But I guess it is fine, given that InitNotification does that without the
> patch.

Yeah. But only the selection data is in ContentCacheInParent. I wonder why IMENotification isn't used at IPC.
Comment on attachment 8631923 [details] [diff] [review]
part.1 Child process should notify its parent process when it dispatches composition or selection event into the DOM tree

>  bool
>  TabChild::RecvCompositionEvent(const WidgetCompositionEvent& event)
>  {
> +  unused << SendOnEventNeedingAckReceived();
>    WidgetCompositionEvent localEvent(event);
>    localEvent.widget = mPuppetWidget;
>    APZCCallbackHelper::DispatchWidgetEvent(localEvent);
>    return true;
>  }
>  
>  bool
>  TabChild::RecvSelectionEvent(const WidgetSelectionEvent& event)
>  {
> +  unused << SendOnEventNeedingAckReceived();
>    WidgetSelectionEvent localEvent(event);
>    localEvent.widget = mPuppetWidget;
>    APZCCallbackHelper::DispatchWidgetEvent(localEvent);
>    return true;
>  }

Oh, I realized this now. Sending notification before dispatching the event into the tree may cause TabParent notifying IME of the previous state of the sending event. So, they should be notified after dispatching the event...
You need to log in before you can comment on or make changes to this bug.