959847 - respect ignore viewport scrolling flag on subdocuments and RecordFrameMetrics on root layers of all documents exactly when we are ignoring viewport scrolling

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Timothy Nikkel (:tnikkel)]

We don't create scroll layers for root scroll frames of of presshell's that are ignoring viewport scrolling. So we should call RecordFrameMetrics on the root layer of the document if we are ignoring viewport scrolling. We have always also ignored the "ignore viewport scrolling" flag for all subdocuments.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: Part 1. Retrieve the displayport the same as everywhere else.

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: Part 2. Respect ignore viewport scrolling flag on subdocuments.

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: Part 3. Contain zoom items inside resolution items.

In the rare case where we have both I think this is the correct order.

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: Part 4. Call RecordFrameMetrics for root layers of subdocuments.

This is kats' patch from https://github.com/staktrace/mozilla-central/commit/4aae77913a7bbeced7699c512e612de0ba04b33b unbitrotted and then I made some changes to it as well.

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: Part 5. Only call RecordFrameMetrics on document root layers if we are ignoring viewport scrolling and hence our root scroll frame won't.

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: Part 6. Don't RecordFrameMetrics in PaintForFrame if we aren't ignoring viewport scrolling (our root scroll frame will do it).

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: Part 7. Move ignoreViewportScrolling declaration down to where we actually use it.

Comment 8

[Botond Ballo [:botond]]

I tested these patches (in isolation, without my bug 935219 patches) on B2G and Fennec.

Things look fine on B2G, but on Fennec I see a lot heavier checkerboarding with these patches than without. It's almost as if we don't have a displayport at all. I will have to look into this.

I haven't tested on Metro yet (still building, Metro builds take forever).

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #8)
> Things look fine on B2G, but on Fennec I see a lot heavier checkerboarding
> with these patches than without. It's almost as if we don't have a
> displayport at all. I will have to look into this.

On Fennec the layer structure has always been the "other" way, so I would not be surprised if the Fennec APZC (what do we call it?) does not know how to deal with this.

Comment 10

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #8)
> I haven't tested on Metro yet (still building, Metro builds take forever).

Intent on proving me wrong, my Metro build finished a couple of minutes after I wrote this. Unfortunately, Metro seems to crash on startup with these patches, after showing the UI for 1-2 seconds. I will have to look into this as well.

Comment 11

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #10)
> (In reply to Botond Ballo [:botond] from comment #8)
> > I haven't tested on Metro yet (still building, Metro builds take forever).
> 
> Intent on proving me wrong, my Metro build finished a couple of minutes
> after I wrote this. Unfortunately, Metro seems to crash on startup with
> these patches, after showing the UI for 1-2 seconds. I will have to look
> into this as well.

Good news: the crash happens without the patches too, so it's not caused by them. It's in the layers dumping code, so maybe related to bug 946879.

Comment 12

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #11)
> Good news: the crash happens without the patches too, so it's not caused by
> them. It's in the layers dumping code, so maybe related to bug 946879.

It seems the Metro crash was indeed related to bug 946879, as the same workaround that fixed that fixed this crash as well.

I tested these patches with the workaround on Metro. I didn't see any blatant regression with checkerboarding the way I did on Fennec, nor any correctness issues.

Comment 13

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #9)
> (In reply to Botond Ballo [:botond] from comment #8)
> > Things look fine on B2G, but on Fennec I see a lot heavier checkerboarding
> > with these patches than without. It's almost as if we don't have a
> > displayport at all. I will have to look into this.
> 
> On Fennec the layer structure has always been the "other" way, so I would
> not be surprised if the Fennec APZC (what do we call it?) does not know how
> to deal with this.

Kats suggested on IRC that the issue might be related to what LayerManager::GetPrimaryScrollableLayer() [1] returns.

Here is the layer structure without the patches:

A1 ContainerLayer [chrome document]
A2   ContainerLayer [root content document]
A3     ContainerLayer  <scrollable>
A4       ThebesLayer

and with:

B1 ContainerLayer [chrome document]
B2   ContainerLayer [root content document] <scrollable>
B3     ThebesLayer

GetPrimaryScrollableLayer() returns A3 without the patches, and B2 with them. I believe that's in order.

I also saw in the layer dump that B2 has a displayport set on it that looks reasonable.

So nothing seems wrong so far. I guess we'll need to investigate further.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.cpp#55

Comment 14

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #13)
> I guess we'll need to investigate further.

Further investigation revealed a relevant difference between the the before- and after- layer trees. With the patches in this bug, the valid region of the thebes layer representing the scrollable content is smaller than expected (the size of the scroll port rather than the size of the display port).

Comment 15

[Botond Ballo [:botond]]

Attachment: Part 8. Don't let ProcessDisplayItems set the clip on subdocument items when they are being used to draw the display port content.

Comment 16

[Botond Ballo [:botond]]

Attachment: Part 9 - Subdocument items for subdocuments that have displayports need to expand the visibility region.

I posted a couple of more patches that Timothy sent me over email. With these patches I saw no checkerboarding regression on Fennec (or anywhere else) and the bug 935219 patch works correctly on top of these.

Comment 17

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

Without this we don't hit APZ performance target for 1.4.

Comment 18

[Timothy Nikkel (:tnikkel)]

Attachment: Part 1. Retrieve the displayport the same as everywhere else.

Comment 19

[Timothy Nikkel (:tnikkel)]

Attachment: Part 2. Respect ignore viewport scrolling flag on subdocuments.

Comment 20

[Timothy Nikkel (:tnikkel)]

Attachment: Part 3. Contain zoom items inside resolution items.

Comment 21

[Timothy Nikkel (:tnikkel)]

Attachment: Part 4. Call RecordFrameMetrics for root layers of subdocuments.

Comment 22

[Timothy Nikkel (:tnikkel)]

Attachment: Part 5. Only call RecordFrameMetrics on document root layers if we are ignoring viewport scrolling and hence our root scroll frame won't.

Comment 23

[Timothy Nikkel (:tnikkel)]

Attachment: Part 6. Don't RecordFrameMetrics in PaintForFrame if we aren't ignoring viewport scrolling (our root scroll frame will do it).

Comment 24

[Timothy Nikkel (:tnikkel)]

Attachment: Part 7. Move ignoreViewportScrolling declaration down to where we actually use it.

Comment 25

[Timothy Nikkel (:tnikkel)]

Attachment: Part 8. Don't let ProcessDisplayItems set the clip on subdocument items when they are being used to draw the display port content.

Comment 26

[Timothy Nikkel (:tnikkel)]

Attachment: Part 9. Subdocument items for subdocuments that have displayports need to expand the visibility region.

Comment 27

[Timothy Nikkel (:tnikkel)]

Attachment: Part 10. Implement GetBounds, GetOpaqueRegion, and ShouldBuildLayerEvenIfInvisible for nsDisplaySubDocument like nsDisplayScrollLayer.

Comment 28

[Timothy Nikkel (:tnikkel)]

Attachment: Part 11. Use nsLayoutUtils::ViewportHasDisplayPort instead of repeating boilerplate code.

Comment 29

[Timothy Nikkel (:tnikkel)]

This makes nsDisplaySubDocument and nsDisplayScrollLayer duplicate a lot of code. It would be nice if we could refactor out this duplication, but I haven't done it yet.

Comment 30

[Timothy Nikkel (:tnikkel)]

The basic idea here is to make the root layer for a subdocument be the scrollable layer, instead of having a child layer created by the root scroll frame by the scrollable layer. This is what we do for display root documents currently. We want to do the same for subdocuments. We want to do this so that the resolution is on the same layer that we async pan because that is what AZPC can handle.

Comment 31

[Timothy Nikkel (:tnikkel)]

This looks big, but a lot of them are simple patches or cleanup.

Comment 32

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

Try push of these patches on top of latest m-c code:

https://tbpl.mozilla.org/?tree=Try&rev=4c0873272efb

(Trying to figure out if the failures we saw in the try push for bug 935219 are real or not)

Comment 33

[Timothy Nikkel (:tnikkel)]

Attachment: Part 2. Respect ignore viewport scrolling flag on subdocuments.

Comment 34

[Timothy Nikkel (:tnikkel)]

Attachment: Part 11. Use nsLayoutUtils::ViewportHasDisplayPort instead of repeating boilerplate code.

Comment 35

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

Aborted the try push in comment 32, new try push with the updated patches (to fix minor compile error):

https://tbpl.mozilla.org/?tree=Try&rev=5b6d53559be2

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8382045 [details] [diff] [review]
Part 3. Contain zoom items inside resolution items.

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

Please explain somewhere why this is correct (and why it matters!)

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8382046 [details] [diff] [review]
Part 4. Call RecordFrameMetrics for root layers of subdocuments.

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

This is kind of yucky, but it will go away when we convert scrolling to be driven from FrameLayerBuilder instead of scroll display items.

Comment 38

[Timothy Nikkel (:tnikkel)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36)
> Comment on attachment 8382045 [details] [diff] [review]
> Part 3. Contain zoom items inside resolution items.
> 
> Review of attachment 8382045 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please explain somewhere why this is correct (and why it matters!)

I don't have any evidence that it matters, but I feel that conceptually this is the right order. App units per dev pixel changes cause the document to be laid out differently, whereas resolution is just a scale transform applied after all other layout. So conceptually the scale transform is happening last. Do you feel the other way? I don't think this is needed, I just noticed it while working on this code.

Comment 39

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tn) from comment #38)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36)
> > Comment on attachment 8382045 [details] [diff] [review]
> > Part 3. Contain zoom items inside resolution items.
> > 
> > Review of attachment 8382045 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Please explain somewhere why this is correct (and why it matters!)
> 
> I don't have any evidence that it matters, but I feel that conceptually this
> is the right order. App units per dev pixel changes cause the document to be
> laid out differently, whereas resolution is just a scale transform applied
> after all other layout. So conceptually the scale transform is happening
> last. Do you feel the other way? I don't think this is needed, I just
> noticed it while working on this code.

Looking at the original bug that gave us resolution items and gave us the current ordering (bug 732971) it looks my intention was always to have zoom items be contained inside resolution items, but somehow it didn't make it into the final code that landed.

Comment 40

[Timothy Nikkel (:tnikkel)]

comment 38 and comment 39

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Find, just add a comment in the patch explaining why.

Comment 42

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

Try push for these patches is green. Feel free to land whenever ready.

Comment 43

[Timothy Nikkel (:tnikkel)]

Attachment: Part 3. Contain zoom items inside resolution items.

Comment 44

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/b2g-inbound/rev/a11ecf90de1a
https://hg.mozilla.org/integration/b2g-inbound/rev/ffb2d1363821
https://hg.mozilla.org/integration/b2g-inbound/rev/522ba2d5437b
https://hg.mozilla.org/integration/b2g-inbound/rev/95590431aebb
https://hg.mozilla.org/integration/b2g-inbound/rev/c984b7ac2a84
https://hg.mozilla.org/integration/b2g-inbound/rev/ed8cade5616f
https://hg.mozilla.org/integration/b2g-inbound/rev/dc9252c6de8d
https://hg.mozilla.org/integration/b2g-inbound/rev/3bf7def1b7f4
https://hg.mozilla.org/integration/b2g-inbound/rev/33a413a575e6
https://hg.mozilla.org/integration/b2g-inbound/rev/4432a2475f49
https://hg.mozilla.org/integration/b2g-inbound/rev/d5595e7a7a3c

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a11ecf90de1a
https://hg.mozilla.org/mozilla-central/rev/ffb2d1363821
https://hg.mozilla.org/mozilla-central/rev/522ba2d5437b
https://hg.mozilla.org/mozilla-central/rev/95590431aebb
https://hg.mozilla.org/mozilla-central/rev/c984b7ac2a84
https://hg.mozilla.org/mozilla-central/rev/ed8cade5616f
https://hg.mozilla.org/mozilla-central/rev/dc9252c6de8d
https://hg.mozilla.org/mozilla-central/rev/3bf7def1b7f4
https://hg.mozilla.org/mozilla-central/rev/33a413a575e6
https://hg.mozilla.org/mozilla-central/rev/4432a2475f49
https://hg.mozilla.org/mozilla-central/rev/d5595e7a7a3c