969378 - Flush repaints on the entire handoff chain on touch up

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

Attachment: Patch

Spun off from https://bugzilla.mozilla.org/show_bug.cgi?id=965593#c7 - see that comment for details. Comment 15 has my original patch, and comment 25 has Botond's review. Updated patch attached.

(In reply to Botond Ballo [:botond] from bug 965593 comment #25)
> General question: do we have a mechanism for making sure that two
> RequestContentRepaints, one on an inner frame and one on an outer, don't
> actually cause two paints by Gecko?

Usually the dirty rect takes care of this, but that doesn't apply when a displayport is set. Once we enable tiles and tile caching though, this problem should be handled by that code.

> ::: gfx/layers/ipc/AsyncPanZoomController.cpp
> This change introduces a reliance here on the overscroll handoff chain being
> populated whenever we are panning. This _should_ be the case, but if subtle
> bugs in event handling (perhaps ones we introduce in the future) break this
> property, we will fail worse by relying on it here.
> 
> Perhaps we can add a boolean return value to
> FlushRepaintsForOverscrollHandoffChain() which is false if the chain was
> empty, and explicitly RequestContentRepaint here in that case?

Done.

Comment 1

[Botond Ballo [:botond]]

Comment on attachment 8372269 [details] [diff] [review]
Patch

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

Thanks!

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +683,5 @@
>  bool
> +APZCTreeManager::FlushRepaintsForOverscrollHandoffChain()
> +{
> +  if (mOverscrollHandoffChain.length() == 0) {
> +    NS_WARNING("Overscroll handoff chain was empty! This should not be the case.");

Can we say "overscroll handoff chain was empty during panning"? (And if you then think the warning makes more sense at the call site, feel free to move it.)

Comment 2

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

Yeah, now that you mention it, it probably does make more sense at the call site. I'll move it and reword it.

Also I realized that the patch above was based on 1.3. On master there's also a call to UpdateSharedCompositorFrameMetrics at the call site, which I'm going to move into FlushRepaintForOverscrollHandoff. That way it also gets called for the entire handoff chain, and if there is no chain, it will get called for the local APZC.

Comment 3

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bf0b04301ca

Comment 4

[Timothy Nikkel (:tnikkel)]

wrong bug number in commit so this got put in the wrong bug
https://hg.mozilla.org/mozilla-central/rev/1bf0b04301ca

Comment 5

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

Comment on attachment 8372269 [details] [diff] [review]
Patch

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): APZ
User impact if declined: In cases where we fall back to the slower scrolling mechanism (see bug 975033 comment 29), this patch helps prevent a noticeable "jump" when you start scrolling, because some state is out of sync.
Testing completed: locally, on m-c for a while now
Risk to taking this patch (and alternatives if risky): pretty low risk i think. 
String or UUID changes made by this patch: none

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/23e7a68fb491

Comment 7

[Ryan VanderMeulen [:RyanVM]]

I had to back this out for bustage.
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/cee34f28af0e

https://tbpl.mozilla.org/php/getParsedLog.php?id=35387876&tree=Mozilla-B2g28-v1.3

Comment 8

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

Attachment: Branch patch for b2g28

I'll land this tomorrow since I can't watch the tree now. If anybody else wants to land it before then, please go right ahead.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8383452 [details] [diff] [review]
Branch patch for b2g28

Thanks for the rebase :)

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/782253bdd6c5