1377653 - [e10s] Refine event dispatching state between processes

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, enhancement)

Description

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

Currently, event dispatching state between processes are manged with some bool flags.  However, it's difficult to understand especially if we need to add new flags.  So, I'd like to clean up it before fixing bug 1257617.

Comment 1

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c03a0d55c9be16b72071fdbbd5a1e4af738f6148

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=468674818823eaa61caef40d8574e2ec40c99e1e

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=76bf049a55c61bcbb8df344751c0c62c986a81da

Comment 4

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=03f0777f5552efd2b20dd64149b6ca017ddc43ff

Comment 5

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

Attachment: Bug 1377653 - part1: Get rid of WidgetKeyboardEvent::mInputMethodAppState

WidgetKeyboardEvent::mInputMethodAppState was used by B2G for managing keyboard event propagation state between chrome process and IME process.  Therefore, we can remove it now.

Review commit: https://reviewboard.mozilla.org/r/153942/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153942/

Comment 6

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

Attachment: Bug 1377653 - part2: Add helper methods to WidgetEvent and BaseEventFlags to manage propagation state between parent process and remote process

Currently, we have 2 bool flags (and optional 2 bool flags with related purpose) for managing propagation state between parent process and remote process.  However, it's really complicated.  Actually, setting these flags and referring the flags is usually follow explanation.

So, for making simpler, WidgetEvent and BaseEventFlags should have some utility methods for making them as self documented code.

This patch moves WidgetKeyboardEvent::mIsReserved to BaseEventFlags::mIsReservedByChrome.  That allows us to manage the cross process event propagation state in same place.

Review commit: https://reviewboard.mozilla.org/r/153944/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153944/

Comment 7

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

Attachment: Bug 1377653 - part3: WidgetEvent::mFlags should have a bool flag if it's been posted to at least one remote process

Currently, it's not been managed yet that whether an event is posted to at least one remote process.  So, for managing the state, BaseEventFlags should have a new bool flag and WidgetEvent and BaseEventFlags should have helper methods for it.

Review commit: https://reviewboard.mozilla.org/r/153946/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153946/

Comment 8

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8882977 [details]
Bug 1377653 - part1: Get rid of WidgetKeyboardEvent::mInputMethodAppState

https://reviewboard.mozilla.org/r/153942/#review159096

Comment 9

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8882978 [details]
Bug 1377653 - part2: Add helper methods to WidgetEvent and BaseEventFlags to manage propagation state between parent process and remote process

https://reviewboard.mozilla.org/r/153944/#review159098

::: dom/events/Event.cpp:460
(Diff revision 1)
>  }
>  
>  NS_IMETHODIMP
>  Event::StopCrossProcessForwarding()
>  {
> -  mEvent->StopCrossProcessForwarding();
> +  mEvent->PreventDispatchedToRemoteProcess();

I don't like PreventDispatchedToRemoteProcess().
StopCrossProcessForwarding is easier to understand, IMO.

::: dom/ipc/TabChild.cpp:1922
(Diff revision 1)
>    }
>  
>    // If a response is desired from the content process, resend the key event.
>    // If mAccessKeyForwardedToChild is set, then don't resend the key event yet
>    // as RecvHandleAccessKey will do this.
> -  if (localEvent.mFlags.mWantReplyFromContentProcess) {
> +  if (localEvent.NeedsToReplyToParentProcess()) {

Also this. I this the existing name is easier to understand.

Comment 10

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8882979 [details]
Bug 1377653 - part3: WidgetEvent::mFlags should have a bool flag if it's been posted to at least one remote process

https://reviewboard.mozilla.org/r/153946/#review159100

Calling MarkAsPostedToRemoteProcess(); everywhere separately is error prone.
Couldn't we use ParamTrait<WidgetEvent>::Write for this. It would always set the flag

::: widget/BasicEvents.h:158
(Diff revision 1)
>    // in the parent process after the content process has handled it. Useful
>    // for when the parent process need the know first how the event was used
>    // by content before handling it itself.
>    bool mWantReplyFromContentProcess : 1;
> +  // If mPostedToRemoteProcess is true, the event has been posted to the
> +  // remote process (but it's not handled yet if it's not a duplicated evnet

event

Comment 11

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=478f21ecf499d551996bdb171b3335dd2df81bf4

Comment 12

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=079d7daa9f3d188b50c0e878d4e209bfc63189bc

Comment 13

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=107ed23edab99ab02ccdf57feaf7b6d770a466c7

Comment 14

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2eab155443cfbb846e6af475b41b9cdba62cf458

Comment 15

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

Comment on attachment 8882977 [details]
Bug 1377653 - part1: Get rid of WidgetKeyboardEvent::mInputMethodAppState

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153942/diff/1-2/

Comment 16

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

Comment on attachment 8882978 [details]
Bug 1377653 - part2: Add helper methods to WidgetEvent and BaseEventFlags to manage propagation state between parent process and remote process

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153944/diff/1-2/

Comment 17

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

Comment on attachment 8882979 [details]
Bug 1377653 - part3: WidgetEvent::mFlags should have a bool flag if it's been posted to at least one remote process

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153946/diff/1-2/

Comment 18

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8882978 [details]
Bug 1377653 - part2: Add helper methods to WidgetEvent and BaseEventFlags to manage propagation state between parent process and remote process

https://reviewboard.mozilla.org/r/153944/#review159366

::: dom/xbl/nsXBLWindowKeyHandler.cpp:551
(Diff revision 2)
>  
>    if (!HasHandlerForEvent(aEvent)) {
>      return;
>    }
>  
> -  // Inform the child process that this is a event that we want a reply
> +  // If this event hadn't been marked as IsCrossProcessForwardingStopped,

s/hadn't been/wasn't/

::: widget/BasicEvents.h:246
(Diff revision 2)
> +  {
> +    mNoRemoteProcessDispatch = false;
> +    mWantReplyFromContentProcess = true;
> +    // Stop propagation in this process.  Other event handlers should handle
> +    // the event with reply event.
> +    StopImmediatePropagation();

This is super confusing. Why does MarkAsWaitingReplyFromRemoteProcess have this kind of side effect to call StopImmediatePropagation()?

So, please remove StopImmediatePropagation() and let the caller of MarkAsWaitingReplyFromRemoteProcess() to call also that when needed.

Comment 19

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8882979 [details]
Bug 1377653 - part3: WidgetEvent::mFlags should have a bool flag if it's been posted to at least one remote process

https://reviewboard.mozilla.org/r/153946/#review159370

It is still a bit unclear to me what is the end goal with this all. Doesn't seem to make code much easier to follow. But r+ anyhow, with those nits fixed.

::: commit-message-9f130:5
(Diff revision 2)
> +Bug 1377653 - part3: WidgetEvent::mFlags should have a bool flag if it's been posted to at least one remote process r?smaug
> +
> +Currently, it's not been managed yet that whether an event is posted to at least one remote process.  So, for managing the state, BaseEventFlags should have a new bool flag and WidgetEvent and BaseEventFlags should have helper methods for it.
> +
> +Additionally, this fixes a bug of nsGUIEventIPC.h. In a lot of ParamTraits, static_cast<Foo> is used for using base class's ParamTraits.  However, it causes creating temporary instance with copy constructor.  Therefore, WidgetEvent::MarkAsPostedToRemoteProcess() call in ParamTraits<mozilla::WidgetEvent>::Write() didn't work as expected and that must have made bad damage to the performance.

Well, probably not bad damage. More like very small, just one memcpy.

::: dom/ipc/TabParent.h:461
(Diff revision 2)
> +  void SendRealTouchEvent(WidgetTouchEvent& aEvent);
>  
> -  bool SendRealKeyEvent(mozilla::WidgetKeyboardEvent& aEvent);
> +  void SendPluginEvent(WidgetPluginEvent& aEvent);
>  
> -  bool SendRealTouchEvent(WidgetTouchEvent& aEvent);
> +  /**
> +   * Different from above Send*Event(), these methods return true if the

Confusing return value. 
But I don't have now good suggestions to improve this.
Perhaps using some enum for the return value would make this less weird.

::: dom/ipc/TabParent.cpp:1182
(Diff revision 2)
>    uint64_t blockId;
>    ApzAwareEventRoutingToChild(&guid, &blockId, nullptr);
>    aEvent.mRefPoint += GetChildProcessOffset();
> -  return PBrowserParent::SendMouseWheelEvent(aEvent, guid, blockId);
> +  DebugOnly<bool> ret =
> +    PBrowserParent::SendMouseWheelEvent(aEvent, guid, blockId);
> +  NS_ASSERTION(ret, "PBrowserParent::SendMouseWheelEvent() failed");

Very unusual style to use NS_ASSERTION _and_ MOZ_ASSERT.
Do we really need NS_ASSERTION ?

Comment 20

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

Comment on attachment 8882978 [details]
Bug 1377653 - part2: Add helper methods to WidgetEvent and BaseEventFlags to manage propagation state between parent process and remote process

https://reviewboard.mozilla.org/r/153944/#review159366

> This is super confusing. Why does MarkAsWaitingReplyFromRemoteProcess have this kind of side effect to call StopImmediatePropagation()?
> 
> So, please remove StopImmediatePropagation() and let the caller of MarkAsWaitingReplyFromRemoteProcess() to call also that when needed.

Hmm, I think that it's really odd to keep handling an event in the process even when a former listener wants reply from a remote process. On the other hand, indeed, it's hard to guess that it'll stop propagation from its name...

Comment 21

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

Comment on attachment 8882979 [details]
Bug 1377653 - part3: WidgetEvent::mFlags should have a bool flag if it's been posted to at least one remote process

https://reviewboard.mozilla.org/r/153946/#review159418

::: dom/ipc/TabParent.cpp:1182
(Diff revision 2)
>    uint64_t blockId;
>    ApzAwareEventRoutingToChild(&guid, &blockId, nullptr);
>    aEvent.mRefPoint += GetChildProcessOffset();
> -  return PBrowserParent::SendMouseWheelEvent(aEvent, guid, blockId);
> +  DebugOnly<bool> ret =
> +    PBrowserParent::SendMouseWheelEvent(aEvent, guid, blockId);
> +  NS_ASSERTION(ret, "PBrowserParent::SendMouseWheelEvent() failed");

Well, it must be unexpected result for the callers if it fails to send the event to a remote process without already destroyed TabParent or something. Therefore, I used NS_ASSERTION for showing to enter unexpected path, and using MOZ_ASSERT which detects a bug of calling MarkAsPostedToRemoteProcess in ParamTraits<WidgetEvent>::Write.

Is it also odd to use NS_WARNING_ASSERTION?

Comment 22

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=012de94a680b71cb10722a55b01313915a089c24

Comment 23

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

oh, perhaps NS_WARNING_ASSERTION would be better than NS_ASSERTION

Comment 24

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

Thanks, I'll use it.

Comment 25

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

Comment on attachment 8882979 [details]
Bug 1377653 - part3: WidgetEvent::mFlags should have a bool flag if it's been posted to at least one remote process

https://reviewboard.mozilla.org/r/153946/#review159370

> Well, probably not bad damage. More like very small, just one memcpy.

I'll remove it from the commit message.

> Confusing return value. 
> But I don't have now good suggestions to improve this.
> Perhaps using some enum for the return value would make this less weird.

I agree. I'll file a followup bug.

Comment 26

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

Comment on attachment 8882977 [details]
Bug 1377653 - part1: Get rid of WidgetKeyboardEvent::mInputMethodAppState

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153942/diff/2-3/

Comment 27

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

Comment on attachment 8882978 [details]
Bug 1377653 - part2: Add helper methods to WidgetEvent and BaseEventFlags to manage propagation state between parent process and remote process

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153944/diff/2-3/

Comment 28

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

Comment on attachment 8882979 [details]
Bug 1377653 - part3: WidgetEvent::mFlags should have a bool flag if it's been posted to at least one remote process

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153946/diff/2-3/

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/308c1f2050b7
part1: Get rid of WidgetKeyboardEvent::mInputMethodAppState r=smaug
https://hg.mozilla.org/integration/autoland/rev/13dbbebe646a
part2: Add helper methods to WidgetEvent and BaseEventFlags to manage propagation state between parent process and remote process r=smaug
https://hg.mozilla.org/integration/autoland/rev/44f5ac7a16e4
part3: WidgetEvent::mFlags should have a bool flag if it's been posted to at least one remote process r=smaug

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/308c1f2050b7
https://hg.mozilla.org/mozilla-central/rev/13dbbebe646a
https://hg.mozilla.org/mozilla-central/rev/44f5ac7a16e4