978248 - Displayport not tile-aligned on first paint

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

I'm filing this based on Vlad's description on IRC; haven't tested it myself.

On a first paint, the displayport is not tile-aligned because MaybeAlignAndClampDisplayPort does not get called.

I'm pretty sure I can see why this is by inspection:

 - ScrollFrameHelper::mOriginOfLastScroll is initialized to nsGkAtoms::other [1].
   Nothing changes this by the time TabChild's before-first-paint handler
   calls APZCCallbackHelper::UpdateRootFrame().
 - ScrollFrameTo() does not set aSuccessOut to true if the origin of the 
   last scroll is set and not nsGkAtoms::apz [2].
 - APZCCallbackHelper::UpdateRootFrame() does not call 
   MaybeAlignAndClampDisplayPort() if ScrollFrameTo() did not set
   aSuccessOut.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#1554
[2] http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/APZCCallbackHelper.cpp#111
[3] http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/APZCCallbackHelper.cpp#111

Comment 1

[Botond Ballo [:botond]]

Attachment: Make sure display port is tile-aligned on first paint

This patch should resolve the problem by calling MaybeAlignAndClampDisplayPort() even if we called RecenterDisplayPort().

Someone who has tiling enabled should test this.

Comment 2

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8383909 [details] [diff] [review]
Make sure display port is tile-aligned on first paint

ScrollFrameTo actually have three different possibilities:
1) no scroll frame, we can't ask it to scroll, we can't get it's scroll position, we turn (0,0)
2) we can't ask the scroll frame to scroll, but we return it's current scroll position
3) we scroll the scroll frame and return the resulting scroll position

I think this patch will make us handle 1) incorrectly. We will re-center the display port, and then call MaybeAlignAndClampDisplayPort with actualScrollOffset=(0,0), effectively moving the displayport by mScrollOffset on the frame metrics. I think this is the exact situation bug 966507, so it will unfix that bug. In this case perhaps we should call MaybeAlignAndClampDisplayPort with the mScrollOffset from the frame metrics.

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: use the AZPC scroll offset if the scroll frame is gone

If the scroll frame is just being recreated (so it will come back right away) it will most likely try to restore to the scroll position it had before it was recreated. This scroll position is much more likely to be closer to what the AZPC controller thinks is the scroll position then (0,0). This should fix the issue I described in the last comment.

Comment 4

[Botond Ballo [:botond]]

Requesting 1.4? due to blocking bug 963073.

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8383909 [details] [diff] [review]
Make sure display port is tile-aligned on first paint

Vlad told me that this patch fixed the problem he was having. Flagging for review.

Comment 6

[Timothy Nikkel (:tnikkel)]

landed my part
https://hg.mozilla.org/integration/b2g-inbound/rev/89282d84d99d

Comment 7

[Botond Ballo [:botond]]

Try push for the two patches together: https://tbpl.mozilla.org/?tree=Try&rev=e87813a56824

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/89282d84d99d

Comment 9

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #7)
> Try push for the two patches together:
> https://tbpl.mozilla.org/?tree=Try&rev=e87813a56824

Looks clean. Landed: https://hg.mozilla.org/integration/b2g-inbound/rev/3601c5bc51e1

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

This doesn't need to be 1.4+ in order to land, nor do we need to uplift it anywhere. Do you still want it blocking?

Comment 11

[Botond Ballo [:botond]]

(In reply to Milan Sreckovic [:milan] from comment #10)
> This doesn't need to be 1.4+ in order to land, nor do we need to uplift it
> anywhere. Do you still want it blocking?

Guess not, then? I just assumed that, as with 1.3, something blocking a 1.4 blocker needs to also be a 1.4 blocker.

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

No, I meant "if you're only asking for this to be a blocker because you want to land it, you don't need to". If this is required for tiling to be successful, we'll mark it a blocker. 
Can't tell if it should be :tn or :botond, as assignee, please reassign if I'm off.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3601c5bc51e1

Comment 14

[Timothy Nikkel (:tnikkel)]

Botond's is the main fix.

Comment 15

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

(In reply to Timothy Nikkel (:tn) from comment #2)
> ScrollFrameTo actually have three different possibilities:
> 1) no scroll frame, we can't ask it to scroll, we can't get it's scroll
> position, we turn (0,0)
> 2) we can't ask the scroll frame to scroll, but we return it's current
> scroll position
> 3) we scroll the scroll frame and return the resulting scroll position
> 
> I think this patch will make us handle 1) incorrectly. We will re-center the
> display port, and then call MaybeAlignAndClampDisplayPort with
> actualScrollOffset=(0,0), effectively moving the displayport by
> mScrollOffset on the frame metrics. I think this is the exact situation bug
> 966507, so it will unfix that bug. In this case perhaps we should call
> MaybeAlignAndClampDisplayPort with the mScrollOffset from the frame metrics.

This patch also made (2) be handled incorrectly. See https://bugzilla.mozilla.org/show_bug.cgi?id=1010119#c8