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 on attachment 8882977 [details] Bug 1377653 - part1: Get rid of WidgetKeyboardEvent::mInputMethodAppState https://reviewboard.mozilla.org/r/153942/#review159096

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 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 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 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 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 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?

oh, perhaps NS_WARNING_ASSERTION would be better than NS_ASSERTION

Thanks, I'll use it.

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.

Pushed by masayuki@d-toybox.com: 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

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