Closed Bug 969378 Opened 6 years ago Closed 6 years ago

Flush repaints on the entire handoff chain on touch up


(Core :: Panning and Zooming, defect)

Gonk (Firefox OS)
Not set



Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed


(Reporter: kats, Assigned: kats)



(2 files)

Attached patch PatchSplinter Review
Spun off from - 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?

Attachment #8372269 - Flags: review?(botond)
Comment on attachment 8372269 [details] [diff] [review]

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


::: 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.)
Attachment #8372269 - Flags: review?(botond) → review+
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.
Assignee: nobody → bugmail.mozilla
wrong bug number in commit so this got put in the wrong bug
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment on attachment 8372269 [details] [diff] [review]

NOTE: This flag is now for security issues only. Please see 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
Attachment #8372269 - Flags: approval-mozilla-b2g28?
Attachment #8372269 - Flags: approval-mozilla-b2g28? → approval-mozilla-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.
Attachment #8383452 - Flags: checkin?
Comment on attachment 8383452 [details] [diff] [review]
Branch patch for b2g28

Thanks for the rebase :)
Attachment #8383452 - Flags: checkin? → checkin+
You need to log in before you can comment on or make changes to this bug.