1111873 - Correctly handle in-process wheel events on Windows

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

aka "apz scroll about:config". Also non-e10s Firefox, though on Windows as we are still processing events on the main thread, it does not really buy us anything. It's also part proof-of-concept for Mac, since it can much more easily benefit from in-process event handling.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, layer to TabParent map

kats - this is your patch stolen from bug 920036. It looks fine to me, but I'm wondering whether it would be cleaner to use GeckoContentController instead and make it responsible for dispatching events?

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, widget confirmation

Comment 3

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

Comment on attachment 8536885 [details] [diff] [review]
part 2, widget confirmation

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

::: widget/windows/nsWindow.cpp
@@ +3759,5 @@
>    DispatchEvent(aEvent, status);
> +
> +  if (mAPZC && !TabParent::GetTabParentFromLayersId(guid.mLayersId)) {
> +    // The event was not routed through any child process, so we need to notify
> +    // APZ in the chrome process.

So there's a case we should handle here where the input event goes through the APZ, hit a dispatch-to-content region in the parent process, then goes through main-thread hit testing and actually ends up hitting the child process. In this case the TabChild code will send the SetTargetAPZC and ContentReceivedInputBlock notification back to the APZ correctly, but we need to avoid doing it here. Because the APZ will return the guid for the main-process layer that that had the dispatch-to-content region, we need some other way of knowing if the input event actually ended up going to the child process during main-thread dispatch. I think setting a flag on the InputAPZContext in TabParent::SendMouseWheelEvent might be a way to do this, but I'm open to other suggestions.

Comment 4

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

Comment on attachment 8536883 [details] [diff] [review]
part 1, layer to TabParent map

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

(In reply to David Anderson [:dvander] from comment #1)
> kats - this is your patch stolen from bug 920036. It looks fine to me, but
> I'm wondering whether it would be cleaner to use GeckoContentController
> instead and make it responsible for dispatching events?

I'm not sure what you had in mind - could you elaborate a bit on that? Also I shouldn't review this patch since I wrote it :) Generally I've been sending reviews like this to smaug but we can discuss what you had in mind first.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: bug1111873.patch

This was my other idea, which makes APZ responsible for dispatching events rather than each widget. It's also direct dispatch, not that that matters much for Desktop.

Comment 6

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

Comment on attachment 8537510 [details] [diff] [review]
bug1111873.patch

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

nsWindow is missing sone stuff but I get the idea. Looking at the code though I have some concerns. Until now the GeckoContentController has always been used to send info from the APZC to gecko while this patch uses it for something else. I think it would make more sense to define a new interface to hold the DispatchWheelEvent method (and eventually other ones too). There's also the problem with this patch that if the input event doesn't hut any APZC at all we still need to dispatch it somehow but here you'd get a null APZC and wouldn't be able to. Having some other mechanism to get this event dispatcher interface would probably be better.

Leaving the feedback flag for now as I will probably think about it more tomorrow morning.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Yeah, ideally we'd have some kind of event handling interface that TabParent and nsIWidget both can provide. I was assuming in this approach we would always have a top-level apzc as well, but I guess that's not necessarily always the case.

Would just controller || widget suffice for picking where to dispatch from?

Comment 8

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

So thinking about this more I don't see much advantage to having this sort of event handling interface versus say just having reusable methods in nsBaseWidget. With the interface you have a common abstraction that hides whether you're using a TabParent or nsIWidget underneath, but you still need to (a) make sure you get the right implementation of the interface and (b) provide the two implementations.

Your patch currently does (a) by pulling it from the APZ code as part of ReceiveInputEvent, but I don't really want to tie it to the GeckoContentController interface like that. If we don't use the GeckoContentController and instead define a PostAPZInputHandler (or whatever) interface, then we'll need a map somewhere to go from layers id to an implementation of the interface. This is basically equivalent to what I did with the TabParent map.

For (b) we need to implement DispatchWheelEvent (for example) in the TabParent version of PostAPZInputHandler and in the nsIWidget version of PostAPZInputHandler. The TabParent version is already done, and if we put the nsIWidget version in nsBaseWidget, or (better) some helper class hanging off nsBaseWidget, then we just need to write it once and reuse it in all the various platform-specific nsWindows, as well as from TabChild. Basically, we can still do code reuse without having to put it in ChromeProcessController.

So it seems to me that the interface won't really simplify any of the code at all, or abstract anything away. Thoughts?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8536883 [details] [diff] [review]
part 1, layer to TabParent map

Please don't use std::map. We want a hashtable here.
nsDataHashtable<nsUint64HashKey, TabParent*>


Does the static member variable add a static ctor?
If it does, maybe we need nsDataHashtable<nsUint64HashKey, TabParent*>* as the type.

>+TabParent*
>+TabParent::GetTabParentFromLayersId(uint64_t aLayersId)
>+{
>+  auto i = sTabParentMap.find(aLayersId);
>+  return (i == sTabParentMap.end() ? nullptr : i->second);
>+}
This is super hard to read. What i? what ->second. I guess std has just dummy
naming.
Oh well, with hashtable this would be just
return sTabParentMap.Get(aLayersId);

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Olli Pettay [:smaug] from comment #9)
> >+TabParent*
> >+TabParent::GetTabParentFromLayersId(uint64_t aLayersId)
> >+{
> >+  auto i = sTabParentMap.find(aLayersId);
> >+  return (i == sTabParentMap.end() ? nullptr : i->second);
> >+}
> This is super hard to read. What i? what ->second. I guess std has just dummy
> naming.
> Oh well, with hashtable this would be just
> return sTabParentMap.Get(aLayersId);

Not that I necessarily disagree, but this is idiomatic STL (storing iterators in "auto", and first = key, second = value).

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, layer to tabparent map

new version w/ hashtable

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8538108 [details] [diff] [review]
part 1, layer to tabparent map

Maybe *Table should be something like *LayerTable
'Table' as such is too generic.



(And we shouldn't use bad parts of stl ;))

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, widget confirmation

Comment 14

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

Comment on attachment 8538183 [details] [diff] [review]
part 2, widget confirmation

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

::: dom/ipc/TabParent.cpp
@@ +2150,5 @@
>      if (aOutInputBlockId) {
>        *aOutInputBlockId = InputAPZContext::GetInputBlockId();
>      }
> +
> +    // Let the widget know that the event will be handled asynchronously.

Not sure "asynchronously" is the right word to use here, as it's so overloaded in gecko. I'd say "... will be sent to the child process, which will send the necessary notifications directly to the APZ"

::: gfx/layers/apz/util/InputAPZContext.cpp
@@ +27,5 @@
>  InputAPZContext::InputAPZContext(const ScrollableLayerGuid& aGuid,
>                                   const uint64_t& aBlockId)
>    : mOldGuid(sGuid)
>    , mOldBlockId(sBlockId)
>  {

mOldRoutedToChildProcess should be initialized to sRoutedToChildProcess

@@ +40,5 @@
>    sBlockId = mOldBlockId;
> +  sRoutedToChildProcess = mOldRoutedToChildProcess;
> +}
> +
> +void

nit: /*static*/ void

Also I'd prefer if you moved this up in the file to be with its fellow static functions

::: widget/nsBaseWidget.cpp
@@ +938,5 @@
>  
> +nsEventStatus
> +nsBaseWidget::DispatchEventForAPZ(WidgetGUIEvent* aEvent,
> +                                 const ScrollableLayerGuid& aGuid,
> +                                 uint64_t aInputBlockId)

nit: fix indent on overhanging lines.

@@ +947,5 @@
> +  DispatchEvent(aEvent, status);
> +
> +  if (mAPZC &&
> +      !dom::TabParent::GetTabParentFromLayersId(aGuid.mLayersId) &&
> +      !context.WasRoutedToChildProcess())

hmm.. so this !GetTabParentFromLayersId check is actually redundant here if you're always sending the wheel event through the main process event dispatch, because context.WasRoutedToChildProcess() is a more accurate flag that holds the information we want.

I'm just going to braindump the whole process since it'll make it easier to refer to later.

When we call mAPZC->ReceiveInputEvent, aGuid.mLayersId is set to a TabParent's layer id if the APZ thinks that the event can be dispatched directly to the child without going through the main process (call this case 1). This happens if the point at which the wheel event occurred was unambiguously in the child process and had no wheel listeners in the main process. In this case we should be able to dispatch the event directly to the TabParent via GetTabParentFromLayersId(...)->SendMouseWheelEvent. I did the equivalent of this for input events in attachment 8536257 [details] [diff] [review] (nsWindow.cpp, line 245).

If aGuid.mLayersId is set to the root process' layer id, that means the main process *might* care about the event, and so we must dispatch it through the main process. Sometimes the event will be consumed in the main process (call this case 2a). However, it may be the case that during the dispatch, gecko discovers it doesn't really care about it and can send it through to the child process (call this case 2b).

Now, we need to make sure that the APZ code receives the SetTargetAPZC and ContentReceivedInputBlock notifications in each of these cases. Consider case 1. By definition GetTabParentFromLayersId returns non-null in this case. If we dispatch the event directly through to TabParent, then WasRoutedToChildProcess will get set to true, and the code in TabChild.cpp will send the notifications. However, we can also choose to dispatch the event through the main process like you're doing here. Ideally this means it will go into case 2b - WasRoutedToChildProcess will get set to true, and the code in TabChild.cpp will send the notifications.

However, if there are bugs lurking somewhere (quite likely), it might happen that the APZ was wrong, and we actually end up falling from case 1 into case 2a. In this scenario GetTabParentFromLayersId will be non-null, yet the child process will never see the event and WasRoutedToChildProcess will be set to false. The if condition will fail, and neither the widget nor TabChild will send the notifications.

This is kind of an edge case but given the way the code is written in this patch I think it should be easy to avoid, simply by removing the GetTabParentFromLayersId part of the condition and relying only on WasRoutedToChildProcess. In cases 1 and 2b WasRoutedToChildProcess will get set to true, and we don't want to send the notifications from here because they will get sent from TabChild in the child process. And in case 2a WasRoutedToChildProcess will get set to false, and we do want to send the notifications from here.

An alternative would be to actually do the case 1 optimization, and call GetTabParentFromLayersId(...)->SendMouseWheelEvent if GetTabParentFromLayersId is non-null. That's primarily what I was planning on using the TabParent map for. I don't mind leaving that out for now though. I'd still like to land the part 1 patch because I'll want to do this optimization for touch events and I'll need that code.

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

Okay - I'll do the non-optimization case for now and just rely on the "was routed to child" bit, but keep part 1.

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6964ae0da6fa
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/03090de75c45

w/ nits addressed

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6964ae0da6fa
https://hg.mozilla.org/mozilla-central/rev/03090de75c45