1162009 - POINTER_CANCEL does not work in e10s

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Maksim Lebedev]

POINTER_CANCEL event was not sended into content.

Comment 1

[Maksim Lebedev]

Some investigation:
> APZEventState::ProcessTouchEvent(..., nsEventStatus aApzResponse)
> ...
>   if (... && aApzResponse == nsEventStatus_eConsumeDoDefault && ...) {
>     WidgetTouchEvent cancelEvent(aEvent);
>     cancelEvent.message = NS_TOUCH_CANCEL;
> ...
>     cancelEvent.widget->DispatchEvent(&cancelEvent, status);
>   }
> }
And
> TabChild::RecvRealTouchEvent()
> {
> ...
>   mAPZEventState->ProcessTouchEvent(localEvent, aGuid, aInputBlockId, nsEventStatus_eIgnore);
> }
As result NS_TOUCH_CANCEL cannot be sended.

Comment 2

[Maksim Lebedev]

Look's like, obvious place for solution is
> TabChild::RecvRealTouchEvent()
> {
> ...
>   nsEventStatus status = APZCCallbackHelper::DispatchWidgetEvent(localEvent);
> ...
>   mAPZEventState->ProcessTouchEvent(localEvent, aGuid, aInputBlockId, status); // Using status
> ...
> }
But, unfortunately it does not help.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment 1 makes no sense to me. It's not at all obvious to me why this is happening and what bug 1154739 has to do with it. Either you are getting POINTER_CANCEL with e10s and APZ enabled or you are not. Either way, the same should have been happening before 1154739, it's just that APZ was taking effect in non-e10s windows as well.

Comment 4

[Maksim Lebedev]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> Comment 1 makes no sense to me. It's not at all obvious to me why this is
> happening and what bug 1154739 has to do with it. Either you are getting
> POINTER_CANCEL with e10s and APZ enabled or you are not. Either way, the
> same should have been happening before 1154739, it's just that APZ was
> taking effect in non-e10s windows as well.
Before bug 1154739 we tested at non-e10s window, and all worked correctly.
After bug 1154739 the most part of pointer events tests have became failed.
POINTER_CANCEL event stoped dispatching and comment 1 shows why it happens.
Maybe bug 1154739 is not once reason, but looks like, it is first reason.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

(In reply to Maksim Lebedev from comment #4)
> Before bug 1154739 we tested at non-e10s window, and all worked correctly.

Were you testing with APZ enabled?

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Ok, I understand what's going on now. It only works when APZ is enabled, and previously it worked in non-e10s code paths and didn't work in e10s. Since APZ is now only enabled in e10s codepaths, there's no working code path.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

To fix this you need to add aApzResponse to the InputAPZContext at [1], pull it out of the context at [2], send it over IPC at [3], and then use it in the call at [4].

[1] http://mxr.mozilla.org/mozilla-central/source/widget/nsBaseWidget.cpp?rev=94eb248b77b0#1004
[2] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp?rev=94eb248b77b0#1621
[3] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp?rev=94eb248b77b0#1632
[4] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=94eb248b77b0#2405

Comment 8

[Maksim Lebedev]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> To fix this you need to add aApzResponse to the InputAPZContext at [1], pull it out 
> of the context at [2], send it over IPC at [3], and then use it in the call at [4].
Thanks, it helps a lot.

Comment 9

[Maksim Lebedev]

Attachment: touch_cancel_e10s_ver1.diff

+ Add using correct response from APZ to send TOUCH_CANCEL event on e10s

Comment 10

[Maksim Lebedev]

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

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8603229 [details] [diff] [review]
touch_cancel_e10s_ver1.diff

Review of attachment 8603229 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good but please use nsEventStatus as the type all the way through instead of int32_t.

Comment 12

[Maksim Lebedev]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> This looks pretty good but please use nsEventStatus as the type all the way
> through instead of int32_t.
I don't know IPDL very well, but looks like, it cannot use nsEventStatus. I got issues in this case:
> ...\ipc\chromium\src\chrome/common/ipc_message_utils.h(121) : error C2039: 'Read' : 
> is not a member of 'IPC::ParamTraits<P>'
> with
> [
>     P=nsEventStatus
> ]
> ...\obj-i686-pc-mingw32\ipc\ipdl\_ipdlheaders\mozilla/dom/PBrowserChild.h(1170)
> : see reference to function template instantiation 'bool IPC::ReadParam<T>(const IPC::Message *,
> void **,P *)' being  compiled with
> [
>     T=nsEventStatus
>     P=nsEventStatus
> ]
> ...\obj-i686-pc-mingw32\ipc\ipdl\PBrowserChild.cpp(3146) : see reference to function template
> instantiation 'bool mozilla::dom::PBrowserChild::Read<nsEventStatus>(T *,
> const mozilla::dom::PBrowser Child::Message *,void **)' being compiled with
> [
>     T=nsEventStatus
> ]

Comment 13

[Botond Ballo [:botond]]

You need to add support for serializing nsEventStatus using ContiguousEnumSerializer. See how it's done for other enumerations here [1]. (You may need to add a sentinel value to nsEventStatus.)

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/ipc/GfxMessageUtils.h?rev=f3bc38042efe#193

Comment 14

[Maksim Lebedev]

(In reply to Botond Ballo [:botond] from comment #13)
> See how it's done for other enumerations here [1].
Thanks, it helped.

Comment 15

[Maksim Lebedev]

Attachment: touch_cancel_e10s_ver2.diff

+ Added SENTINEL_VALUE into enum nsEventStatus
+ Added ContiguousEnumSerializer for nsEventStatus
+ (Send/Recv)RealTouch(Move)Event uint32_t aApzResponse -> nsEventStatus

Suggestions and comments and objections are very welcome.

Comment 16

[Maksim Lebedev]

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

Comment 17

[Dmitriy Barkalov]

Tested on win8 opt. Pointercancel tests (https://github.com/w3c/web-platform-tests/tree/master/pointerevents) all passes

Comment 18

[Botond Ballo [:botond]]

Comment on attachment 8605117 [details] [diff] [review]
touch_cancel_e10s_ver2.diff

Review of attachment 8605117 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/EventForwards.h
@@ +24,5 @@
>    nsEventStatus_eConsumeNoDefault,
>    // The event is consumed, but do default processing
> +  nsEventStatus_eConsumeDoDefault,
> +  // Value is not for use, only for serialization
> +  SENTINEL_VALUE

Please name it nsEventStatus_eSentinel, for consistency with the other names (also, prefixing with nsEventStatus_ is important because this isn't an enum class, so otherwise we'd be adding a name like SENTINEL_VALUE to the global namespace).

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8605117 [details] [diff] [review]
touch_cancel_e10s_ver2.diff

Review of attachment 8605117 [details] [diff] [review]:
-----------------------------------------------------------------

Just a couple of minor comment changes, and what botond said.

::: dom/ipc/TabParent.h
@@ +502,5 @@
>      // null.
>      // |aOutInputBlockId| will contain the identifier of the input block
>      // that this event was added to, if there was one. aOutInputBlockId may
>      // be null.
>      void ApzAwareEventRoutingToChild(ScrollableLayerGuid* aOutTargetGuid,

Update the comment to also say:

// |aOutApzResponse| will contain the response that the APZ gave
// when processing the input block; this is used for generating
// appropriate pointercancel events.

::: gfx/layers/apz/util/InputAPZContext.h
@@ +12,5 @@
>  namespace mozilla {
>  namespace layers {
>  
> +// InputAPZContext is used to communicate the ScrollableLayerGuid and
> +// input block ID and ApzResponse from nsIWidget to RenderFrameParent.

"...ScrollableLayerGuid, input block ID, and APZ response from nsIWidget...."

Comment 20

[Maksim Lebedev]

Attachment: touch_cancel_e10s_ver3.diff

+ nsEventStatus::SENTINEL_VALUE -> nsEventStatus::nsEventStatus_eSentinel
+ Changed some comments

Comment 21

[Maksim Lebedev]

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

Comment 22

[Botond Ballo [:botond]]

Comment on attachment 8605865 [details] [diff] [review]
touch_cancel_e10s_ver3.diff

Review of attachment 8605865 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8605865 [details] [diff] [review]
touch_cancel_e10s_ver3.diff

Review of attachment 8605865 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/TabParent.h
@@ +511,2 @@
>      bool mMarkedDestroying;
> +    // When true, the TabParent is invalid and we should not send IPC messages anymore.

Try to avoid changing stuff not related to your changes.

Comment 24

[Maksim Lebedev]

(In reply to Kartikaya Gupta from comment #23)
> Try to avoid changing stuff not related to your changes.
It was very nearest code.
Should I change patch again?

Comment 25

[Kartikaya Gupta (email:[e-mail])]

No, don't bother

Comment 26

[Maksim Lebedev]

If there are no objections, I put checkin-needed flag.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c5b4478608d

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8c5b4478608d