1142380 - Make touch working in b2g if APZ is disabled

Status: RESOLVED WONTFIX

(Core Graveyard :: Widget: Gonk, defect)

Description

[Kershaw Chang [:kershaw]]

Please take a look at bug 1124087 comment #6.
In order to test nested-oop iframe in b2g, we need to make touch working if APZ is disabled.

Comment 1

[Kershaw Chang [:kershaw]]

kats, Could you please provide a patch as you mentioned at bug 1124087 comment #6?
Thanks.

Comment 2

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

Attachment: Make B2G work without APZ

This should do the job. I don't want to land this patch but you can apply it locally and work with it as needed. I hope to make some changes to the code in the next few days so that B2G will work with APZ disabled without having to apply this. I'm not sure exactly when I'll get around to it though.

Comment 3

[Kershaw Chang [:kershaw]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> Created attachment 8577264 [details] [diff] [review]
> Make B2G work without APZ
> 
> This should do the job. I don't want to land this patch but you can apply it
> locally and work with it as needed. I hope to make some changes to the code
> in the next few days so that B2G will work with APZ disabled without having
> to apply this. I'm not sure exactly when I'll get around to it though.

Thanks. It works on my flame.

Comment 4

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

Attachment: Part 1 - Cleanup some widget code

I've had some patches to fix this sitting in my local repo for a while now, and even if we don't care about B2G with APZ disabled the cleanup bits are worth landing, I think.

Comment 5

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

Attachment: Part 2 - Avoid a crash from passing garbage over IPC

Comment 6

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

Attachment: Part 3 - Allow using B2G with APZ disabled

I don't know if we actually want to land this part.

Comment 7

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

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

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8614832 [details] [diff] [review]
Part 1 - Cleanup some widget code

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

::: widget/gonk/nsWindow.cpp
@@ +214,5 @@
>  
> +    if (gFocusedWindow) {
> +        gFocusedWindow->DispatchAPZAwareEvent(aInput);
> +        NS_DispatchToMainThread(NS_NewRunnableFunction([=]() {
> +            gFocusedWindow->UserActivity();

We're moving the UserActivity() call from before ProcessUntransformedAPZEvent() to after. We're also creating a second task for it.

Is this desirable?

::: widget/nsBaseWidget.cpp
@@ +1098,5 @@
>    return status;
>  }
>  
> +// This is a version of DispatchAPZAwareEvent intended for use on
> +// platforms where the main thread is NOT the APZ controller thread.

Please move this comment into the header, and add a comment to the other overload that says it's meant for platforms where the two threads are the same.

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8614833 [details] [diff] [review]
Part 2 - Avoid a crash from passing garbage over IPC

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

Out of curiosity, how does the garbage cause crashes?

Comment 10

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

(In reply to Botond Ballo [:botond] from comment #8)
> > +    if (gFocusedWindow) {
> > +        gFocusedWindow->DispatchAPZAwareEvent(aInput);
> > +        NS_DispatchToMainThread(NS_NewRunnableFunction([=]() {
> > +            gFocusedWindow->UserActivity();
> 
> We're moving the UserActivity() call from before
> ProcessUntransformedAPZEvent() to after. We're also creating a second task
> for it.
> 
> Is this desirable?

I can move the UserActivity() call up to before, and probably make a static task that we reuse. That reduces the overhead to an extra pointer in the task queue, which I don't think is so bad.

> ::: widget/nsBaseWidget.cpp
> @@ +1098,5 @@
> >    return status;
> >  }
> >  
> > +// This is a version of DispatchAPZAwareEvent intended for use on
> > +// platforms where the main thread is NOT the APZ controller thread.
> 
> Please move this comment into the header, and add a comment to the other
> overload that says it's meant for platforms where the two threads are the
> same.

Ok, I can do that.

(In reply to Botond Ballo [:botond] from comment #9)
> 
> Out of curiosity, how does the garbage cause crashes?

Apparently there's some IPC code that checks for valid values or something. It's the first time I've encountered it, but I got a crash with a message like this:

[Child 2579] ###!!! ABORT: IPDL error [PBrowserChild]: \"Error deserializing 'nsEventStatus'\". abort()ing as a result.: file /home/kats/zspace/B2G-flame-kk/gecko/ipc/glue/ProtocolUtils.cpp, line 327") at /home/kats/zspace/B2G-flame-kk/gecko/memory/mozalloc/mozalloc_abort.cpp:33

Comment 11

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

(Further up the stack the error was coming from this code in the auto-generated PBrowserChild.cpp:

            if ((!(Read((&(aApzResponse)), (&(msg__)), (&(iter__)))))) {
                FatalError("Error deserializing 'nsEventStatus'");
                return MsgValueError;
            }

Comment 12

[Botond Ballo [:botond]]

Ah - ContiguousEnumSerializer returns false from Read() if the enum value is not within the range of valid values. Very nice!

Comment 13

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

Comment on attachment 8614832 [details] [diff] [review]
Part 1 - Cleanup some widget code

Dropping this review for now, there might be additional cleanup that we can do as discussed on IRC (e.g. merging DispatchInputEvent with DispatchAPZAwareEvent) but it's low-priority.

Comment 14

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

Comment on attachment 8614833 [details] [diff] [review]
Part 2 - Avoid a crash from passing garbage over IPC

Moved this patch to bug 1173719 and landed it.

Comment 15

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

I don't think we want to support B2G without APZ. The cleanup patch here isn't sufficiently useful to warrant spending more time on it right now. If we touch that code again in the future we can clean it up then.