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.

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

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

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).

(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.

(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.

(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.

(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.

(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

(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).

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.

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

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.

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.

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

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)

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 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 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.

(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.

(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 38 and comment 39

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

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

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

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