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.)

Factor out APZCTreeManager creation to nsBaseWidget.

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

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.

Address comments

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.

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

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

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

(style fix)

Comment on attachment 8536813 [details] [diff] [review] part 2, forward from nsWindow Review of attachment 8536813 [details] [diff] [review]: ----------------------------------------------------------------- Awesome, thanks!

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

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

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.

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