Closed Bug 949132 Opened 11 years ago Closed 11 years ago

Replace UpdateScrollOffset with a flag on the FrameMetrics during layer updates

Categories

(Core :: Panning and Zooming, defect)

Product:
Core
Component:
Panning and Zooming
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla29
Project Flags:
blocking-b2g 1.3+
Tracking Flags:
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files, 2 obsolete files)

Part 1 - Add a flag on FrameMetrics (v2)
(deleted), patch
tnikkel
: review+
Details | Diff | Splinter Review
Part 2 - Use the flag and delete a mountain of code
(deleted), patch
cwiiis
: review+
Details | Diff | Splinter Review 
As one of the possible solutions to bug 947337 we discussed getting rid of the UpdateScrollOffset method on APZCTreeManager/AsyncPanZoomController. Instead, we could have a flag on the FrameMetrics that is set when content does a scrollTo, and then during the layers update the APZC could pick up the scroll offset from the frame metrics. Markus provided a patch on that bug to accomplish this.

Although that patch didn't fix that bug I'd still like to land it because it results in cleaner code so I'm forking it over to this bug.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #0)
> As one of the possible solutions to bug 947337 we discussed getting rid of
> the UpdateScrollOffset method on APZCTreeManager/AsyncPanZoomController.
> Instead, we could have a flag on the FrameMetrics that is set when content
> does a scrollTo, and then during the layers update the APZC could pick up
> the scroll offset from the frame metrics. Markus provided a patch on that
> bug to accomplish this.

This will be in addition to, not instead of, getting rid of UpdateScrollOffset, right?
Yeah. We can get rid of UpdateScrollOffset if we have the flag.
Attached patch Part 1 - Add a flag on FrameMetrics (obsolete) (deleted) — — Splinter Review 
This is Markus' patch from bug 947337, with two changes:
1) Updated UUID for nsIDOMWindowUtils.idl
2) Renamed mWasScrolledByContent to be mWasScrolledByApz (and inverted the meaning). I did this because there other things that can trigger scrolls, like mouse wheel movements, and so the thing we really want to distinguish is APZ-driven scrolling vs everything else.
Attachment #8346171 - Flags: review?(tnikkel)
Attached patch Part 2 - Use the flag and delete a mountain of code (obsolete) (deleted) — — Splinter Review 
I've tested this on B2G but haven't built Fennec/Metro with it yet. Will push to try.
Attachment #8346172 - Flags: review?(jmathies)
Attachment #8346172 - Flags: review?(botond)
Comment on attachment 8346171 [details] [diff] [review]
Part 1 - Add a flag on FrameMetrics

>@@ -2045,16 +2047,18 @@ ScrollFrameHelper::ScrollToImpl(nsPoint aPt, const nsRect& aRange)
>   ScheduleSyntheticMouseMove();
>   nsWeakFrame weakFrame(mOuter);
>   UpdateScrollbarPosition();
>   if (!weakFrame.IsAlive()) {
>     return;
>   }
>   PostScrollEvent();
> 
>+  mOriginOfLastScroll = aOrigin;
>+
>   // notify the listeners.
>   for (uint32_t i = 0; i < mListeners.Length(); i++) {
>     mListeners[i]->ScrollPositionDidChange(pt.x, pt.y);
>   }
> }

Minor point: I would update mOriginOfLastScroll a little earlier, since the scroll has happened, and the weakframe check shows that we will enter other code here, if it ever checks the origin of last scroll it will be out of date. I would say after we set the position of the scrolled frame.


General question: what happens if we get an apz scroll, then a scroll from somewhere else, and then we trigger a layers update? Would that cause a problem?
Attachment #8346171 - Flags: review?(tnikkel) → review+
(In reply to Timothy Nikkel (:tn) from comment #6)
> General question: what happens if we get an apz scroll, then a scroll from
> somewhere else, and then we trigger a layers update? Would that cause a
> problem?

That scenario is actually ok because then mWasScrolledByApz will be false, and so in the APZC code we'll take the new scroll offset as desired. The problem is actually the opposite scenario, where we get a content scrollTo, then an inflight apz scroll is processed, and then we trigger a layers update, because then mWasScrolledByApz will be true and will mask the fact that there was a content scrollTo. (Also we'll end up losing the scroll position of the content scrollTo because the apz scroll will clobber it).

Previously this was handled because the UpdateScrollOffset would set the content scrollTo value in the APZC so even if it got clobbered by an inflight apz scroll, the next apz scroll would have the right value. The only way I see to not regress this behaviour with the new patches is to add some logic in ScrollToImpl that throws away apz scrolls between a non-apz scroll and the corresponding layers update. This would involve adding a "non-apz" atom and using that for all scrolls other than apz-driven scrolls, and being able to reset the mOriginOfLastScroll as part of RecordFrameMetrics. Doable but kinda ugly because it's putting logic into nsGfxScrollFrame that maybe doesn't belong there.
Comment on attachment 8346172 [details] [diff] [review]
Part 2 - Use the flag and delete a mountain of code

no identifiable side effects as far as I can tell.
Attachment #8346172 - Flags: review?(jmathies) → review+
cc'ing matt so he knows this code is coming out.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7)
> Previously this was handled because the UpdateScrollOffset would set the
> content scrollTo value in the APZC so even if it got clobbered by an
> inflight apz scroll, the next apz scroll would have the right value. The
> only way I see to not regress this behaviour with the new patches is to add
> some logic in ScrollToImpl that throws away apz scrolls between a non-apz
> scroll and the corresponding layers update. This would involve adding a
> "non-apz" atom and using that for all scrolls other than apz-driven scrolls,
> and being able to reset the mOriginOfLastScroll as part of
> RecordFrameMetrics. Doable but kinda ugly because it's putting logic into
> nsGfxScrollFrame that maybe doesn't belong there.

Would the apzc callback that updates the scroll position not have enough information to determine if it should update the scroll position? Could we provide accessors so it does?
Attached patch Part 1 - Add a flag on FrameMetrics (v2) (deleted) — — Splinter Review 
You were right, I can move that logic into APZCCallbackHelper and it ends up being cleaner. Here is an updated patch. I renamed the flag on FrameMetrics again because it turned out I need to invert the boolean again to catch some other cases.
Attachment #8346171 - Attachment is obsolete: true
Attachment #8346892 - Flags: review?(tnikkel)
This is pretty much the same as before but rebased. Carrying jim's r+ and r? for botond.
Attachment #8346172 - Attachment is obsolete: true
Attachment #8346172 - Flags: review?(botond)
Attachment #8346894 - Flags: review?(botond)
Comment on attachment 8346892 [details] [diff] [review]
Part 1 - Add a flag on FrameMetrics (v2)

>+ScrollFrameHelper::ScrollToImpl(nsPoint aPt, const nsRect& aRange, nsIAtom* aOrigin)
> {
>+  if (aOrigin == nullptr) {
>+    // If no origin was specified, we still want to set it to something that's
>+    // non-null, so that we can use nullness to distinguish if the frame was scrolled
>+    // at all. Default it to some generic placeholder.
>+    aOrigin = nsGkAtoms::generic_;
>+  }

There are a few places already in the scroll frame code where we have the same pattern but we use the "other" atom. Can that work for this too?
Attachment #8346892 - Flags: review?(tnikkel) → review+
Yeah I can change that. I just glanced through the list of atoms and picked the first one that sounded appropriate.
Btw the try run for these patches is also clean

https://tbpl.mozilla.org/?tree=Try&rev=24891d16cc00
We deliberately do not want this on train 28 (b2g 1.3), right?
Blocks: gaia-apzc-2
No longer blocks: 947337
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=947337
That depends. For me the patches on this bug solve bug 947337 on both the Peak and hamachi devices. Vivien said he could still reproduce bug 947337 with (an earlier version of) these patches. I would like him to re-test with the latest version of these patches; if they work for him AND we want to fix bug 947337 in 1.3, then this would need to go on that train.
Comment on attachment 8346894 [details] [diff] [review]
Part 2 - Use the flag and delete a mountain of code

Will steal this, but leaving r?botond for now in case he gets to it before I do (because I can't do it right now)
Attachment #8346894 - Flags: review?(chrislord.net)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17)
> That depends. For me the patches on this bug solve bug 947337 on both the
> Peak and hamachi devices. Vivien said he could still reproduce bug 947337
> with (an earlier version of) these patches. I would like him to re-test with
> the latest version of these patches; if they work for him AND we want to fix
> bug 947337 in 1.3, then this would need to go on that train.

OK, so the Comment 0 "one of the possible solutions to bug 947337 is to fix this" is really "we must fix this to fix bug 947337"?
(In reply to Milan Sreckovic [:milan] from comment #19)
> OK, so the Comment 0 "one of the possible solutions to bug 947337 is to fix
> this" is really "we must fix this to fix bug 947337"?

Given my current understanding of what's happening in bug 947337, yes. It could be that my understanding is wrong, though. I'm putting together some additional logging patches that might help shed some more light on the matter.
Thanks.  If you verify the dependency, rather than option, can you make it 1.3?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17)
> That depends. For me the patches on this bug solve bug 947337 on both the
> Peak and hamachi devices. Vivien said he could still reproduce bug 947337
> with (an earlier version of) these patches. I would like him to re-test with
> the latest version of these patches; if they work for him AND we want to fix
> bug 947337 in 1.3, then this would need to go on that train.

My feeling is that we want to fix bug 947337 in 1.3. I don't mind to land a stop gap as the patch I proposed in bug 947337 even if I agree this is not the right fix and it may have side issues (which seems hard to trigger imho). Then you will have plenty of time to fix the real issue in 1.4 :)

In the meantime I will retry with this patch as soon as I can.
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #22)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17)
> > That depends. For me the patches on this bug solve bug 947337 on both the
> > Peak and hamachi devices. Vivien said he could still reproduce bug 947337
> > with (an earlier version of) these patches. I would like him to re-test with
> > the latest version of these patches; if they work for him AND we want to fix
> > bug 947337 in 1.3, then this would need to go on that train.
> 
> My feeling is that we want to fix bug 947337 in 1.3. I don't mind to land a
> stop gap as the patch I proposed in bug 947337 even if I agree this is not
> the right fix and it may have side issues (which seems hard to trigger
> imho). Then you will have plenty of time to fix the real issue in 1.4 :)
> 
> In the meantime I will retry with this patch as soon as I can.

With this patch I can reproduce bug 947337 and bug 942752.

Here is an example of the wrong values:
I/GeckoDump(11066): Event: pageY: 372
I/GeckoDump(11066): Event: pageY: 211
I/GeckoDump(11066): Event: pageY: 211
I/GeckoDump(11066): Event: pageY: -34092
I/GeckoDump(11066): Event: pageY: -34092
I/GeckoDump(11066): Event: pageY: -34092
I/GeckoDump(11066): Event: pageY: 349
I/GeckoDump(11066): Event: pageY: 349
I/GeckoDump(11066): Event: pageY: 276
I/GeckoDump(11066): Event: pageY: 276
I/GeckoDump(11066): Event: pageY: 31955
I/GeckoDump(11066): Event: pageY: 241
I/GeckoDump(11066): Event: pageY: -10365
I/GeckoDump(11066): Event: pageY: 201
Can you also provide the exact patch you are using to get the above output?
Comment on attachment 8346894 [details] [diff] [review]
Part 2 - Use the flag and delete a mountain of code

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

Stealing review. This is glorious, land it.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1490,5 @@
>      mFrameMetrics.mResolution = aLayerMetrics.mResolution;
>      mFrameMetrics.mCumulativeResolution = aLayerMetrics.mCumulativeResolution;
> +
> +    // If the layers update was not triggered by a repaint of our own repaint
> +    // request, then we want to take the new scroll offset.

This comment reads a bit oddly - how about just 'If the layers update was not triggered by our own repaint request'?
Attachment #8346894 - Flags: review?(chrislord.net)
Attachment #8346894 - Flags: review?(botond)
Attachment #8346894 - Flags: review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #24)
> Can you also provide the exact patch you are using to get the above output?

I'm just adding a |dump(currentY);| at https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/contacts_shortcuts.js#L128
https://hg.mozilla.org/mozilla-central/rev/d49be6bd6378
https://hg.mozilla.org/mozilla-central/rev/5047906b359f
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Do we want this in b2g1.3/Aurora? I would suggest that we do, given the code it replaces had possible race conditions.
Yeah, I agree. It will also make uplifting future patches easier.
blocking-b2g: --- → 1.3?
status-firefox27: --- → wontfix
status-firefox28: --- → affected
status-firefox29: --- → fixed
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #30)
> Yeah, I agree. It will also make uplifting future patches easier.

I think this is a strong reason to ask for aurora approval, but not a reason to block the release. Can you nominate this for aurora approval?
blocking-b2g: 1.3? → -
Comment on attachment 8346892 [details] [diff] [review]
Part 1 - Add a flag on FrameMetrics (v2)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): original APZ code
User impact if declined: Possible race conditions when scrolling while content is doing scrollTo calls can result in unexpected jumps or bad touch events. Also uplifting to aurora will make future APZ code uplifts easier
Testing completed (on m-c, etc.): on m-c for a while now
Risk to taking this patch (and alternatives if risky): pretty low risk, it's baked a while and it's well understood
String or IDL/UUID changes made by this patch: UUID change on nsIDOMWindowUtils.idl to remove a function that is no longer needed.
Attachment #8346892 - Flags: approval-mozilla-aurora?
Comment on attachment 8346894 [details] [diff] [review]
Part 2 - Use the flag and delete a mountain of code

Same as above
Attachment #8346894 - Flags: approval-mozilla-aurora?
This is duplicating bug 947337 which is 1.3+, so we need to 1.3+ this one and make sure uplift happens.
blocking-b2g: - → 1.3+
Attachment #8346892 - Flags: approval-mozilla-aurora?
Attachment #8346894 - Flags: approval-mozilla-aurora?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: