David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Description • 10 years ago

I'd like to factor out the code on Mac that introduces a GeckoContentController for the chrome process, which will let us do apzc for chrome, get events to apz faster, and allow apz when not using e10s. (Though, I do not expect apz to help us much without e10s.)

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 2 • 10 years ago

Factor out APZCTreeManager creation to nsBaseWidget.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 3 • 10 years ago

Actually, chrome process support should be a separate bug. This patch just moves the event handling back to nsWindow where it belongs.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 4 • 10 years ago

Comment on attachment 8534868 [details] [diff] [review]
patch

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

I don't understand the purpose of this change. Currently the scroll wheel events are delivered to the APZCTM via TabParent, and you want to move that into nsWindow so it happens earlier/for the root process as well. I get that, but a lot of the changes in this patch don't really help with that goal.

- The change to nsWindow.cpp makes sense, but I would expect there to be a corresponding change to TabParent.cpp to prevent sending the mouse wheel events into the APZCTM again.
- Moving the code from APZCTreeManager.cpp into APZCCallbackHelper.cpp doesn't provide any benefit; it would be fine to leave that in APZCTM. When I said to put the reusable code in APZCCallbackHelper I was referring to e.g. code invoked from RecvMouseWheelEvent in TabChild.cpp. Similar code will need to run in the chrome process as well for those cases where the wheel event happens on async-scrollable parts of the chrome.
- Also in retrospect the InputQueue::ReceiveScrollWheelInput function should just always return nsEventStatus_eConsumeDoDefault unconditionally. (See the documentation on APZCTreeManager::ReceiveInputEvent for what this means). There are probably some cases in which it would be more precise to return nsEventStatus_eIgnore but as none of the call sites actually care about this return value we should be fine with just always returning DoDefault instead, as that satisfies the ReceiveInputEvent return value contract. With that you shouldn't need any of the other changes to InputQueue.*

Thoughts? I might be missing the entire purpose of this change or something. Clearing review flag for now.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 5 • 10 years ago

Address comments

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 6 • 10 years ago

Comment on attachment 8535266 [details] [diff] [review]
patch v2

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

This is much better, thanks! However after I thinking about it I realized two things:
1) It can be simplified further, in that I don't think the changes to APZCTreeManager.* are needed at all if you invoke mAPZC->ReceiveInputEvent(*aEvent->AsInputEvent(), nullptr, nullptr) from the nsWindow call site. That should work fine and will be cleaner because there's no need to expose ProcessWheelEvent publicly.
2) We'll need to start calling mAPZC->ContentReceivedInputBlock from nsWindow in some cases, otherwise the wheel event may not get processed until after a delay. This is exactly the same work that needs to be done for bug 920036 which I'm starting to tackle now. Let me think through this a bit more and get back to you.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 7 • 10 years ago

Comment on attachment 8535266 [details] [diff] [review]
patch v2

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

Ok, so I'm happy with the conceptual approach taken in this patch (I posted some WIPs to bug 920036 that I used to convince myself that this approach is sound). However as I mentioned in the previous comment I think the changes to APZCTreeManager.* in this patch are not necessary and are actually harmful, so I would like to see those removed. Instead just call mAPZC->ReceiveInputEvent(...) in nsWindow.cpp. Everything else looks fine in this patch. r+ with comments addressed, but if you disagree please let me know and/or post a new patch.

There is also the issue of having to call mAPZC->ContentReceivedInputBlock and so on to eliminate the 300ms delay and properly handle wheel event listeners in content but you said on IRC that you were aware of that and would like to land this anyway, and I have no real objections to that. It's something we'll have to resolve before we can turn on APZ on windows though.

::: widget/windows/nsWindow.cpp
@@ +3743,5 @@
>  {
> +  if (mAPZC && aEvent->mClass == eWheelEventClass) {
> +    nsEventStatus status = mAPZC->ProcessWheelEvent(*aEvent->AsWheelEvent());
> +    if (status != nsEventStatus_eIgnore)
> +      return true;

nit: braces

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 8 • 10 years ago

This is kats' patch from bug 920036 part 1, stealing (with permission) and r+'ing here.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 9 • 10 years ago

Similar to the other patch, with comments fixed and using InputAPZContext.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 10 • 10 years ago

(style fix)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 11 • 10 years ago

Comment on attachment 8536813 [details] [diff] [review]
part 2, forward from nsWindow

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

Awesome, thanks!

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 12 • 10 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2187c2cdeea9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c61753316ddb

	Carsten Book [:Tomcat]
	Comment 13 • 10 years ago

sorry had to back this out for memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=4668200&repo=mozilla-inbound

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 14 • 10 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/91fd3f8df7ed                 
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/358dc1ca192b                 

Memory leak ended up being bogus, ScrollableLayerGuid shouldn't have COUNT_CTOR/COUNT_DTOR.

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/91fd3f8df7ed
https://hg.mozilla.org/mozilla-central/rev/358dc1ca192b