STR: 1. Load http://people.mozilla.org/~bballo/iframe.html 2. Observe the initial layer tree on page load Actual results: - On B2G, the iframe starts layerized. - On desktop (I tested on Linux), the iframe does not start layerized. It only becomes layerized after you start panning it. Expected results: Layerization behaviour is the same on both platforms. It's not immediately clear to me which behaviour is the intended one.

This layerization difference does not exist for http://people.mozilla.org/~bballo/div.html: there, the subframe starts off not layerized on both platforms.

(In reply to Botond Ballo [:botond] from comment #1) > This layerization difference does not exist for > http://people.mozilla.org/~bballo/div.html: there, the subframe starts off > not layerized on both platforms. I lied - the difference exists there too! (I was judging by the handoff behaviour, and failed to take the scrollgrabbing container into account on B2G.)

Is e10s enabled in the desktop build you're testing on?

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3) > Is e10s enabled in the desktop build you're testing on? Good question! ni myself to check this when I'm back at my office desktop (can't get ssh with X forwarding to work, and I don't know how to check a pref value from the command line).

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3) > Is e10s enabled in the desktop build you're testing on? I have e10s enabled, yes. Should I also be testing things with e10s disabled?

No, e10s enabled is the case we care about for now.

(In reply to Botond Ballo [:botond] from comment #0) > It's not immediately clear to me which behaviour is the intended one. The layerization of the subframe on page load is unexpected. It's done by the optimization added in bug 982141 to layerize the primary scrollable frame; it's firing wrongly, because the subframe isn't the primary scrollable frame here (the root scroll frame is).

This was regressed by bug 1013780. The patch in that bug added a builder.IsPaintingToWindow() flag to the condition required for a scrollable frame to be considered the primary scrollable frame. However, nsLayoutUtils::PaintFrame() sets this flag on the builder further down than the call to GetOrMaybeCreateDisplayPort() which checks it for the root scroll frame.

The reason the same issue doesn't affect desktop is that on desktop, we have containerless scrolling for root scroll frames, and so the call to GetOrMaybeCreateDisplayPort() for the root scroll frame happens in ScrollFrameHelper::BuildDisplayList() (by which time the painting-to-window flag has been set) rather than in nsLayoutUtils::PaintFrame(). (Actually, the call to GetOrMaybeCreateDisplayPort() in nsLayoutUtils::PaintFrame() is not conditioned on containerless scrolling for root frames being off, so if it's on, as on desktop, GetOrMaybeCreateDisplayPort() gets called twice for the root scroll frame (with only the second call, in ScrollFrameHelper::BuildDisplayList(), picking up the painting-to-window flag). We should probably omit the first call in this case.)

/r/7085 - Bug 1151663 - Initialize display list builder flags earlier in nsLayoutUtils::PaintFrame(). r=tn Pull down this commit: hg pull -r c021ed7e178aa6b681df435f2fce7863cf7f344c https://reviewboard-hg.mozilla.org/gecko/

I'm also working on a test for this.

Comment on attachment 8593036 [details] MozReview Request: bz://1151663/botond /r/7085 - Bug 1151663 - Only call GetOrMaybeCreateDisplayPort() in nsLayoutUtils::PaintFrame() if we are using containers for root scroll frames. r=tn Pull down this commit: hg pull -r 302c07bbdbc81ac73bdcfb0d5564840d72612953 https://reviewboard-hg.mozilla.org/gecko/

MozReview, you are failing me!

Comment on attachment 8593047 [details] [diff] [review] Part 2 - Fix for the issue mentioned in comment 9 Note that this will also change the dirty rect we are using. This should be fine though. We made the same change for subdocuments. It will get the displayport when we enter the scroll frame. And fixed pos items will also get their proper dirty rect.

There was a bug in the code for communicating the APZ tree structure to APZ mochitests, introduced by parent-process-apz: the parent of the root APZC in the content process would incorrectly be picked up as the root APZC in the content process, causing the mochitest to misinterpret the APZC tree structure. It so happened that the misinterpreted tree structure still satisfied the constraints of the bug 982141 mochitest, so it continued passing. The problem came to light during my attempt to write a mochitest for this bug, though. This patch fixes it.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cd29aae9fae

(In reply to Botond Ballo [:botond] from comment #20) > Try push: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cd29aae9fae Looks green except for Linux build failures, which https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f95fd0ea644 demonstrates are unrelated.

Comment on attachment 8595085 [details] [diff] [review] Part 3 - Extract some helper functions for writing APZ mochitests into a separate file Review of attachment 8595085 [details] [diff] [review]: ----------------------------------------------------------------- r+ assuming this was a straight move of code, no changes.

Comment on attachment 8595087 [details] [diff] [review] Part 4 - Fix reconstruction of APZC tree structure in APZ mochitests Review of attachment 8595087 [details] [diff] [review]: ----------------------------------------------------------------- Let me know if I misunderstood something... ::: gfx/layers/apz/test/apz_test_utils.js @@ +94,1 @@ > addLink(root, scrollId, paint[scrollId]["parentScrollId"]); I don't think this works as intended. For one thing, you can have the same scrollId in multiple layers subtrees, so this addLink call may attach it to the wrong parent. And secondly calling addRoot for the "isRootForLayersId" means that the layers subtrees will get reparented to the root rather than corresponding to the position in the C++ tree. It might just be better to export the "layersId|scrollId" combo from the C++ code and use that as a unique identifier here.

Comment on attachment 8595089 [details] [diff] [review] Part 5 - New mochitest for the optimization-misfiring issue Review of attachment 8595089 [details] [diff] [review]: ----------------------------------------------------------------- Nice test, thanks! ::: gfx/layers/apz/test/mochitest.ini @@ +5,4 @@ > support-files = > apz_test_utils.js > helper_bug982141.html > + helper_bug1151663.html Better to do this: [DEFAULT] support-files = apz_test_utils.js [test_bug982141.html] skip-if... support-files = helper_bug982141.html [test_bug1151663.html] skip-if... support-files = helper_bug1151663.html

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #23) > Comment on attachment 8595087 [details] [diff] [review] > Part 4 - Fix reconstruction of APZC tree structure in APZ mochitests > > Review of attachment 8595087 [details] [diff] [review]: > ----------------------------------------------------------------- > > Let me know if I misunderstood something... The C++ code that logs the isRootForLayersId and parentScrollId messages, only does so for the layer subtree of the content process that originated the layer transaction [1]. Therefore, the APZC tree structure that the JS code reconstructs, is just the subtree corresponding to that layer subtree. (I'll update the comments in the JS code to make this more clear.) Let me know if this addresses your concerns. [1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=6ee1d3e056b3#453

Comment on attachment 8595087 [details] [diff] [review] Part 4 - Fix reconstruction of APZC tree structure in APZ mochitests Review of attachment 8595087 [details] [diff] [review]: ----------------------------------------------------------------- Doh! Yup that works then. Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2fab0c1ed863 https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e788ba5506 https://hg.mozilla.org/integration/mozilla-inbound/rev/1f578c4228ef https://hg.mozilla.org/integration/mozilla-inbound/rev/fbbca69d29e8 https://hg.mozilla.org/integration/mozilla-inbound/rev/4cd578366a91

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #24) > ::: gfx/layers/apz/test/mochitest.ini > @@ +5,4 @@ > > support-files = > > apz_test_utils.js > > helper_bug982141.html > > + helper_bug1151663.html > > Better to do this: > > [DEFAULT] > support-files = > apz_test_utils.js > [test_bug982141.html] > skip-if... > support-files = > helper_bug982141.html > [test_bug1151663.html] > skip-if... > support-files = > helper_bug1151663.html This doesn't seem to work - the tests are busted with an error that suggests that apz_test_utils.js is not being found. That suggests the later support-files clauses are overwriting the [DEFAULT] one, not adding to it. The push in comment 28 moves all support files into a single support-files clause under [DEFAULT].

Oh interesting. Well that's fine too.

https://hg.mozilla.org/mozilla-central/rev/2fab0c1ed863 https://hg.mozilla.org/mozilla-central/rev/b6e788ba5506 https://hg.mozilla.org/mozilla-central/rev/1f578c4228ef https://hg.mozilla.org/mozilla-central/rev/fbbca69d29e8 https://hg.mozilla.org/mozilla-central/rev/4cd578366a91 https://hg.mozilla.org/mozilla-central/rev/726a442167d9

The issue fixed by the Part 1 patch is a regression dating back to B2G 2.0. The regression is that we layerize more than we intend to on initial page load (on a page where the page's root scroll frame is scrollable, we still layerize the first scrollable subframe we find). It shouldn't have correctness implications, but it could mean using more memory than we need to. Kats, do you think it's worth pursuing uplift for this fix?

We could uplift to 2.2 but even for that I'm not really sure it's worth it. In cases where the root frame is scrollable I would think it unlikely that the first scrollable subframe is very large. Also fixing this may expose other issues that were previously masked which we would then have to uplift as well.