1110038 - Don't reuse input blocks with dead APZCs

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

Attachment: fix

If we have an active input block that's ready for handling, then open a new tab - the confirmed apzc for that input block will be destroyed, but we'll always override the target apzc with the input block's target.

I'm not sure what the intended behavior is. This patch just starts a new input block if the old target is dead.

Comment 1

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

Actually, this would never matter for ReceiveTouchInput since it gets explicit "start" messages. So I don't think there's any prior behavior here.

Comment 2

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

Comment on attachment 8534853 [details] [diff] [review]
fix

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

(In reply to David Anderson [:dvander] from comment #0)
> If we have an active input block that's ready for handling, then open a new
> tab - the confirmed apzc for that input block will be destroyed, but we'll
> always override the target apzc with the input block's target.
> 
> I'm not sure what the intended behavior is. This patch just starts a new
> input block if the old target is dead.

If I understand correctly the existing code will have the behavior that switching tabs while in the middle of a wheel-scroll will abort the scroll, and your patch will change it so that the scroll will carry over to the new tab. Is that correct? Currently desktop FF does the latter (even though I personally never liked that behavior) so conceptually then your patch is fine. However there are a couple of issues with the code, see below.

::: gfx/layers/apz/src/InputQueue.cpp
@@ +136,5 @@
>                                      const ScrollWheelInput& aEvent,
>                                      uint64_t* aOutInputBlockId) {
>    WheelBlockState* block = nullptr;
>    if (!mInputBlockQueue.IsEmpty()) {
>      block = mInputBlockQueue.LastElement().get()->AsWheelBlock();

You should be able to get rid of the .get() in this line.

@@ +140,5 @@
>      block = mInputBlockQueue.LastElement().get()->AsWheelBlock();
> +
> +    // If the block's APZC has been destroyed, request a new block.
> +    if (block->GetTargetApzc()->IsDestroyed())
> +      block = nullptr;

AsWheelBlock() could return nullptr above, so you should change this if condition to be "if (block && block->....)". Also, braces please.

Comment 3

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

Yeah, if you initiate a new wheel event while a new tab is opening, it will carry over to the new tab. If that ends up being problematic we might have to do what the non-APZ code does and base it off a timeout/mouse position change, but for now it seems fine.

Comment 4

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

Attachment: bug1110038-dead-blocks.patch

Comment 5

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> If I understand correctly the existing code will have the behavior that
> switching tabs while in the middle of a wheel-scroll will abort the scroll,
> and your patch will change it so that the scroll will carry over to the new
> tab. Is that correct? Currently desktop FF does the latter (even though I
> personally never liked that behavior)

Interesting, I can't reproduce this behaviour with desktop Firefox.

Comment 6

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/9a630c125d39

Comment 7

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

(In reply to Botond Ballo [:botond] from comment #5)
> Interesting, I can't reproduce this behaviour with desktop Firefox.

Hm, maybe I confused it with the trackpad-based momentum scrolling. That definitely carries across tab switches.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a630c125d39