Closed Bug 967844 (multi-layer-apz) Opened 10 years ago Closed 10 years ago

Allow content from one scrollable element to fragment into multiple layers in order to fully support async scroll of out of flow content

Categories

(Core :: Panning and Zooming, defect, P2)

Product:
Core
Component:
Panning and Zooming
Version:
29 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla35
Project Flags:
feature-b2g 2.1
Tracking Flags:
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: tnikkel, Assigned: roc)

References

Details

(Keywords: perf, Whiteboard: [c=handeye p= s= u=])

Attachments

(3 files, 25 obsolete files)

Part 1: Move mBackgroundColor from Layer to FrameMetrics
(deleted), patch
kats
: review+
Details | Diff | Splinter Review
Part 2: Move mContentDescription from Layer to FrameMetrics
(deleted), patch
kats
: review+
Details | Diff | Splinter Review
Part 3 v3
(deleted), patch
mattwoodrow
: review+
bajaj
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
From bug 962791
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #55)
> I think it's clear now that we have to fix ScrollLayer stuff so we can
> always smooth-scroll even if the scrolling content must fragment into
> multiple layers. We can do that by extending the APZC layers attributes to
> associate a set of layers with a scrollable element. It might not even be
> that hard.

We are not wrongly wrapping some abs pos content, this will allow correct wrapping, and to remove the workaround from bug 962791 for that problem.
Summary: although content from one scrollable element to fragment into multiple layers in order to fully support async scroll of out of flow content → Allow content from one scrollable element to fragment into multiple layers in order to fully support async scroll of out of flow content
I believe the plan here is to somehow allow each APZC instance to be associated with multiple layers. That way, if we have content from a single scrollable element that is split into multiple layers because of interleaved (wrt z-index) items, those layers will still scroll together.

For this to work we will probably need to have multiple ContainerLayer instances point to the same APZC instance and so use the same async transform. Since the APZ code was not designed with this in mind, there are likely some assumptions that might break. For instance, the SampleContentTransformForFrame function might need updating if it gets called multiple times per frame of animation (then again it might just work - who knows!).
Alias: multi-layer-apz
Keywords: perf
Whiteboard: [c=handeye p= s= u=]
So these three patches *should* do the job on the APZC side, and don't appear to cause regressions in current behaviour. We can't really exercise the code properly until the layout side is done. ni? to roc since the ball is in your court now :)

Also I'd like to avoid landing these patches until we have confidence that there's no issues we missed with this approach.
Flags: needinfo?(roc)
OS: Mac OS X → All
Hardware: x86 → All
Based on your recent blog post and what matt said on IRC I assume you're planning on working on this soonish. Please let me know if that's not the case, tn might be able to take it if so.
Assignee: nobody → roc
Flags: needinfo?(roc)
Priority: -- → P2
Blocks: 1016796
feature-b2g: --- → 2.1
Whiteboard: [c=handeye p= s= u=] → [c=handeye p= s= u=][ucid:Graphic20,ft:graphic]
Roc, are you still working on this? Is bug 1022612 a part of this work?
Flags: needinfo?(roc)
Yes and yes
Flags: needinfo?(roc)
Depends on: 1022612
Blocks: 1026271
No longer blocks: 1026271
Blocks: 1016939
STR (in a 3rd-party app):
1) Install https://marketplace.firefox.com/app/firetext.
2) Click the menu icon in the top left corner.
3) Click the settings icon in the bottom left corner of the sidebar.
4) Try to use overscroll on the settings page.

Expected result:
Overscroll works fine ^^

Actual result:
No overscroll

As gordonb pointed out, there may be certain combinations of css that could cause this, but all scrolling regions should have overscroll!
No longer blocks: 1016796
Whiteboard: [c=handeye p= s= u=][ucid:Graphic20,ft:graphic] → [c=handeye p= s= u=]
(In reply to Joshua Smith [:joshua-s] from comment #10)
> STR (in a 3rd-party app):
> 1) Install https://marketplace.firefox.com/app/firetext.
> 2) Click the menu icon in the top left corner.
> 3) Click the settings icon in the bottom left corner of the sidebar.
> 4) Try to use overscroll on the settings page.
> 
> Expected result:
> Overscroll works fine ^^
> 
> Actual result:
> No overscroll
> 
> As gordonb pointed out, there may be certain combinations of css that could
> cause this, but all scrolling regions should have overscroll!

I've looked at an APZC tree log for this, and while there are two APZCs on the page in question - the root one, and one for a <div> - neither of them have a scroll range (i.e. both have scrollable rect = composition bounds). This suggests that this bug could indeed be the cause of the problem, though we should confirm by testing once this is fixed.

While investigating this, I discovered that on every page of the Firetext app, we are continuously repainting all the time. This may or may not be related to the sync scrolling issue. I filed bug 1036100 for this.
(In reply to Botond Ballo [:botond] from comment #11)
> I've looked at an APZC tree log for this, and while there are two APZCs on
> the page in question - the root one, and one for a <div> - neither of them
> have a scroll range (i.e. both have scrollable rect = composition bounds).
> This suggests that this bug could indeed be the cause of the problem, though
> we should confirm by testing once this is fixed.

The display list dump suggests the problem is unrelated to this bug; I split it out to bug 1036119.
Attached patch Part 1 - Move FrameMetrics from ContainerLayer to Layer (obsolete) (deleted) — — Splinter Review 
Unbitrotted
Attachment #8400374 - Attachment is obsolete: true
To unbitrot part 2 I would also need to move the mScrollHandoffParentId, mContentDescription, and mBackgroundColor fields up from ContainerLayer to Layer. This is starting to feel like it's going to use a lot more memory unnecessarily. I emailed roc to explore some alternative options.
Attached patch Part 1 - Move FrameMetrics from ContainerLayer to Layer (obsolete) (deleted) — — Splinter Review 

        Attachment #8469410 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 4 - Move the APZ from ContainerLayer to Layer (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #8400375 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 5 - Update APZ tree building to deal with multiple layers (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
This is probably missing some pieces. The hit-testing code in APZCTM with the unobscured rect definitely needs to be updated.

I'm also unsure if the different layers that share scrollIds can have different transforms on them. If they do then things get more complicated, as we probably won't be able to store them directly in the APZC instances.

        Attachment #8400376 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #19)
> I'm also unsure if the different layers that share scrollIds can have
> different transforms on them. If they do then things get more complicated,
> as we probably won't be able to store them directly in the APZC instances.

They may do. For example a video that's being scrolled would have a transform scaling the video ImageLayer combined with a translation for scrolling, while a ThebesLayer sharing the same scrolling state would only have the translation.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 6: Attach FrameMetrics to layers via FrameLayerBuilder (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
This is basically what I have in mind. Not ready for review yet.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 6: Attach FrameMetrics to layers via FrameLayerBuilder (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
This seems to work, in that for some tests in reftest-ipc layout/reftests/async-scrolling it generates good-looking FrameMetrics for non-container layers...

        Attachment #8470624 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8470074 [details] [diff] [review]
Part 1 - Move FrameMetrics from ContainerLayer to Layer

Actually it might be better to move these to a separate bug that we can land sooner.

        Attachment #8470074 -
        Flags: review?(matt.woodrow)

    
    

    
  
Comment on attachment 8470074 [details] [diff] [review]
Part 1 - Move FrameMetrics from ContainerLayer to Layer

Moved the first 4 parts to bug 1051985, so obsoleting them on this bug.

        Attachment #8470074 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 6: Attach FrameMetrics to layers via FrameLayerBuilder (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Sorry!

What's the best way to test this stuff?

        Attachment #8470755 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8470082 [details] [diff] [review]
Part 5 - Update APZ tree building to deal with multiple layers

Moved this patch to bug 1052063

        Attachment #8470082 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> Sorry!

No worries :)

> What's the best way to test this stuff?

I was thinking we should have some reftests that set up pages affected by this bug (e.g. the test case on bug 1011639) and use the reftest-async-scroll properties to scroll the element. That should be a decent sanity check. I can also write a bunch of gtests for specific layer trees to make sure the APZ side is behaving as expected.

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #19)
> > I'm also unsure if the different layers that share scrollIds can have
> > different transforms on them. If they do then things get more complicated,
> > as we probably won't be able to store them directly in the APZC instances.
> 
> They may do. For example a video that's being scrolled would have a
> transform scaling the video ImageLayer combined with a translation for
> scrolling, while a ThebesLayer sharing the same scrolling state would only
> have the translation.

So the thing that was boggling me for a while was the order in which transforms are applied. I want to try to explain it here, using the terminology and chain of layers I defined at [1]. I'm extending it to have two layers L1 and L2, both of which are children of M, and both of which have the same scrollid.

Given the order in which transforms are applied, and given that we use the same async transforms for layers L1 and L2, if L1C and L2C are different, then they will not move "together" as you scroll that APZC. If L1C is the identity matrix and L2C has a 2.0 scaling transform, then L2 will move twice as fast as L1 even though they are supposed to scroll together, because the scaling transform will amplify the async transform.

After some thinking I realized that it might make more sense to change the order in which transforms are applied, so that LA is applied *after* LC, rather than before. This would solve the above problem. I implemented this in part 4 of bug 1052063 (still needs more testing).

Then I realized that there might be other bad cases such as if L1 and L2 are not siblings in the tree. Say L1 has a parent M1 and L2 has a parent M2, and M1's transform is different from M2's transform. Then we have the same problem again. In general, if we have two layers that have the same scrollid, but the chain of transforms that are applied after the async transform is different for those two layers, then we're gonna have a problem. Do you know if that will ever be the case?

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=4b96541d8a0d&mark=1247-1267#1247

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #30)
> Then I realized that there might be other bad cases such as if L1 and L2 are
> not siblings in the tree. Say L1 has a parent M1 and L2 has a parent M2, and
> M1's transform is different from M2's transform.

I don't think we'll have the situation where M1 != M2 and the transforms are different. At least I can't think of one right now.

Trying to think about whether M1 != M2 is possible, there is one case I need to handle, like this:

<div id="S" style="overflow:scroll">
  <div id="O" style="opacity:0.5">
    <div id="L1">...</div>
  </div>
  <div id="L2">...</div>
</div>

With my current patch I think we'll generate FrameMetrics for L1 as well as O and L2, because O and L2 have the same animated geometry root S, but I guess we don't want to do that.

However, there's this interesting case:

<div id="S" style="overflow:scroll">
  <div id="O" style="opacity:0.5">
    <div id="L1">...</div>
    <div id="L2" style="position:absolute">...</div>
  </div>
</div>

In this obnoxious case, L1 and L2 should share a parent layer generated by O (so they get composited in the same group) but L1 is scrolled and L2 is not. O's animated geometry root is S but L2's is the viewport!

How do you want to handle that? :-)

    
    

    
  
To be honest, I don't mind if we simply say that for the forseeable future, that case doesn't scroll correctly during async scrolling. We have much bigger fish to fry here and the steady-state rendering will still be correct.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 6 v2 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
This adds a test for the split case but I haven't actually tested it yet. It also suppresses setting FrameMetrics on a layer with the same animated geometry root as its container layer.

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #31)
> In this obnoxious case, L1 and L2 should share a parent layer generated by O
> (so they get composited in the same group) but L1 is scrolled and L2 is not.
> O's animated geometry root is S but L2's is the viewport!

I think my latest patch will generate a situation where O has FrameMetrics for S but its child layer L2 will have FrameMetrics for the viewport. So there's a situation where two layers with the same FrameMetrics have different parents. However, those parents would normally have the same transform. We can't break that invariant by introducing a CSS transform since a CSS-transformed element is a container for all its positioned descendants, which would force them all to be scrolled by S. I still haven't though of a situatoin where the parents would have different transforms.

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #34)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #31)
> > In this obnoxious case, L1 and L2 should share a parent layer generated by O
> > (so they get composited in the same group) but L1 is scrolled and L2 is not.
> > O's animated geometry root is S but L2's is the viewport!
> 
> I think my latest patch will generate a situation where O has FrameMetrics
> for S but its child layer L2 will have FrameMetrics for the viewport. So
> there's a situation where two layers with the same FrameMetrics have
> different parents. However, those parents would normally have the same
> transform.

Just to make sure I understand, you mean we would have this:

<div id="S" style="overflow:scroll">                  | ContainerLayer S | scrollid=2
  <div id="O" style="opacity:0.5">                    | ContainerLayer O | scrollid=1
    <div id="L1">...</div>                            |   Layer L1       |
    <div id="L2" style="position:absolute">...</div>  |   Layer L2       | scrollid=2
  </div>
</div>

where scrollid=1 means it moves as the scrollTop of S changes, and scrollid=2 means it moves as the scrollTop of document.documentElement changes. In this case S and L2 share the scrollid but have different parents, and their parents transforms are the same. However, L2 will still be affected by the async transform set on O which is undesirable.

Can we detect this case and push the scrollid=1 framemetrics from ContainerLayer O down to Layer L1?

    
    

    
  
Whoops, that text got wrapped badly. The "scrollid=n" stuff is supposed to be on the end of the previous line.

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #35)
> In this case S and L2 share the scrollid but have different
> parents, and their parents transforms are the same. However, L2 will still
> be affected by the async transform set on O which is undesirable.

Right.

> Can we detect this case and push the scrollid=1 framemetrics from
> ContainerLayer O down to Layer L1?

We could do that, but keep in mind that there could be other child content of S, say L0, that's outside O. So you still have the situation that L1 and L0 both have scrollid 1 but have different parents.

I suggest we don't address this for now but leave it aside until we've landed the basic code. Then we can see where the best place is to fit it in. This case is super rare and it doesn't even really matter if it ships with poor behavior.

    
    

    
  
Sounds good.

I think there might be a few other cases that we won't properly handle with the current APZC tree architecture. Putting it here so I don't forget:

ContainerLayer R
  ContainerLayer Q1, has transform T1, scrollid 1
    Layer L, scrollid 2
  ContainerLayer Q2, has transform T2, scrollid 1
    Layer M, scrollid 3

When the APZC tree for this is built it will look like:

APZC Q, scrollid 1 (for Q1 and Q2)
  APZC L, scrollid 2 (for L)
  APZC M, scrollid 3 (for M)

however, Q currently can only store one of T2 or T1 as the "layer transform". With the current implementation it will store T2 as that's the one it encounters first. This means when we do a hit test for layer L, it will use the T2 transform instead of T1. The fundamental problem is that the APZC tree structure loses information that was present in the original layer tree; to fix this we would have to augment the information stored there somehow. One option might be to store T1 and T2 as part of the "ancestor transform" in L and M, rather than storing it on Q. I can try this tomorrow.

    
    

    
  
One problem I've noticed is that we might have more than one scrollframe actively scrolling the same content. E.g.
<div style="overflow:scroll" id="S1">
  <div style="overflow:scroll" id="S2">
    <div id="L1">...</div>

In this case, what should we do? Attach multiple FrameMetrics? I think not... Force the introduction of an intermediate layer? I don't know how to make that work. This can actually be quite common when you have a scrolling element, because its parent is always actively scrolled. 

I think in this case you get the inner FrameMetrics and the outer one is ignored. I don't know if that'll cause problems.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 6 v3 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
I had to update the reftest async-scroll infrastructure a bit to handle the new setup.

All tests pass, which makes me deeply suspicious.

        Attachment #8471299 -
        Attachment is obsolete: true

        Attachment #8472274 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #40)
> All tests pass, which makes me deeply suspicious.

But it may be worth trying the patch out for real, anyway :-)

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #39)
> One problem I've noticed is that we might have more than one scrollframe
> actively scrolling the same content. E.g.
> <div style="overflow:scroll" id="S1">
>   <div style="overflow:scroll" id="S2">
>     <div id="L1">...</div>

So previously we would have had:

ContainerLayer S1 with scrollid=1
  ContainerLayer S2 with scrollid=2
    ThebesLayer for L1

I imagine that now we should have

ContainerLayer S2 with scrollid=1
  ThebesLayer L1 with scrollid=2

i.e. everything just moves down a level. Is that not the case?

> 
> In this case, what should we do? Attach multiple FrameMetrics? I think
> not... Force the introduction of an intermediate layer? I don't know how to
> make that work. This can actually be quite common when you have a scrolling
> element, because its parent is always actively scrolled. 
> 
> I think in this case you get the inner FrameMetrics and the outer one is
> ignored. I don't know if that'll cause problems.

Yeah if we drop the metrics for S1 then S1 won't scroll...

    
    

    
  
I discussed this a bit with tn in the context a page like this:

<div style="overflow:scroll" id="S1">
  <div id="L1">...</div>
  <div style="overflow:scroll" id="S2">
    <div id="L2">...</div>
  </div>
</div>

So in this case you could flatten down to ThebesLayers and have something like this:

ThebesLayer L1 scrollid 1
ThebesLayer L2 scrollid 2

as long as you have a pointer from L2 to L1 that indicates that the metrics on L1 also affects L2. Currently we have the mScrollHandoffParentId on Layer which is used for something similar but not quite the same thing. tn suggested that we should align that with the scroll parent relationship. So in this case L2 would have a scroll handoff parent point to L1. That might work; I'm not sure if there are cases that would give us trouble with that setup.

I'm also not sure how that would map to the scenario you had in comment 39, where there's just one ThebesLayer and so there's no place to put the "parent" FrameMetrics. Or would there still be a layer generated for S1 in that scenario? I guess there must be something in S1 that's outside S2, maybe just a ColorLayer, and we can stick it on that.

If we do turn the mScrollHandoffParentId into a more general "scroll parent" mapping to establish a parent-child scrolling relationship between sibling layers, then the code in AsyncCompositionManager (or maybe somewhere else in the compositor?) would have to be responsible for walking that parent chain and accumulating the necessary async transforms, since that doesn't happen explicitly now.

    
    

    
  
Comment on attachment 8473516 [details] [diff] [review]
part 6 v3

>+    nsIScrollableFrame* scrollFrame =
>+        nsLayoutUtils::GetScrollableFrameFor(e->mAnimatedGeometryRoot);
>+    if (scrollFrame && e->mAnimatedGeometryRoot != mContainerAnimatedGeometryRoot) {
>+      nsRect clipRect(0, 0, -1, -1);
>+      scrollFrame->SetupFrameMetrics(e->mLayer, mContainerReferenceFrame,
>+                                     mParameters, &clipRect);
>+      if (clipRect.width >= 0) {
>+        nsIntRect pixClip = ScaleToNearestPixels(clipRect);
>+        const nsIntRect* layerClip = e->mLayer->GetClipRect();
>+        if (layerClip) {
>+          pixClip.IntersectRect(pixClip, *layerClip);
>+        }
>+        // XXX this could cause IPC churn due to cliprects being updated
>+        // twice during layer building --- for non-ThebesLayers that have
>+        // both CSS and scroll clipping.
>+        // XXX what about content in a layer that is clipped by two active
>+        // scrollframes? Only the inner one will get to apply its clipping here.
>+        e->mLayer->SetClipRect(&pixClip);
>+      }
>+    } else {
>+      e->mLayer->SetFrameMetrics(FrameMetrics());
>+    }

What if the anitmated geometry root is some element that is having it's margin animated, which is a child of a scroll frame that we want async scrolling on, we won't attach frame metrics to that layer, but shouldn't we?

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #43)
> I'm also not sure how that would map to the scenario you had in comment 39,
> where there's just one ThebesLayer and so there's no place to put the
> "parent" FrameMetrics.

Right. That's the problem.

One way to fix this, perhaps the simplest way, would be to allow a layer to have zero or more FrameMetrics attached, in order. How do you feel about that?

    
    

    
  
(In reply to Timothy Nikkel (:tn) from comment #44)
> What if the anitmated geometry root is some element that is having it's
> margin animated, which is a child of a scroll frame that we want async
> scrolling on, we won't attach frame metrics to that layer, but shouldn't we?

You're right.

What we really need to do here is walk up through the animated geometry root ancestors until we reach mContainerReferenceFrame, adding a FrameMetrics for each animated geometry root ancestor that's a scrolled frame. If we find more than one, that's the scenario we're talking about above.

So, let me know if multiple FrameMetrics per layer is OK.
Flags: needinfo?(bugmail.mozilla)

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #45)
> Right. That's the problem.
> 
> One way to fix this, perhaps the simplest way, would be to allow a layer to
> have zero or more FrameMetrics attached, in order. How do you feel about
> that?

From the APZ side of things, that sounds functionally equivalent to introducing dummy ContainerLayers to hold the FrameMetrics, but more work because now a bunch of code needs to change. i.e. it's doable but I would rather just have the dummy ContainerLayers. I don't know how much work it is on the layout side to introduce these dummy ContainerLayers.

If we do have multiple FrameMetrics per layer we'll need to have corresponding multiple APZCs per layer and probably some other properties as well. Bits of code that currently walk up the layer tree looking for APZCs or FrameMetrics will get more complicated as they will need a nested loop. The amount of work is probably on the same order of magnitude as bug 1051985, but it makes the design more complicated I think.
Flags: needinfo?(bugmail.mozilla)

    
    

    
  
Also just to be clear the dummy ContainerLayers don't need to have more than one child. i.e. instead of doing this:

ThebesLayer with FrameMetrics F0, F1
ThebesLayer with FrameMetrics F0, F2

I'm proposing this is just fine:

ContainerLayer with FrameMetrics F0
  ThebesLayer with FrameMetrics F1
ContainerLayer with FrameMetrics F0
  ThebesLayer with FrameMetrics F2

as opposed to the "old" way:

ContainerLayer with FrameMetrics F0
  ThebesLayer with FrameMetrics F1
  ThebesLayer with FrameMetrics F2

    
    

    
  
It's a pain to do it in FrameLayerBuilder mainly because that code is already really complicated. There's a few other issues:
-- layer recycling. these dummy layers would have to be carefully recycled.
-- structure. currently when we build the children of a ContainerLayer we're only building direct children. With this change we'd have to create and manage some layers which aren't direct children of the ContainerLayer.
-- Configuring those ContainerLayers is not trivial. we'd have to set the opaque and component alpha flags correctly, and move the clip and mask up from the real layer to the outermost dummy ContainerLayer (but not transform I think). This is difficult to do since to minimize IPC traffic we currently need to make sure we don't set these attributes on a layer more than once. I.e. if we just set flags and clip and mask on the real layer and later take them off that layer and set them on the outermost dummy ContainerLayer, we generate IPC churn.
-- Every time we make FrameLayerBuilder more complicated we add overhead to all painting.

mattwoodrow, what do you think?
Flags: needinfo?(matt.woodrow)

    
    

    
  
I agree.

I think the only way to make it work would be to build layers normally, and then do a post-processing step where we wrap layers in an extra ContainerLayer. That would then hit the problem roc mentioned where we end up calling Mutated() on layers unnecessarily. It's not appealing, especially given how complex FLB is already.

I forget the exact details of how APZ is setup currently, but couldn't we make the FrameMetrics an actual shared object and have multiple layers reference the same instance? That way we keep one APZ per framemetrics, and we can store any per-layer data on the Layer.
Flags: needinfo?(matt.woodrow)

    
    

    
  
*sigh* fine.. :p

I can do the gfx side changes needed, but what should the API look like?

void Layer::AddFrameMetrics(const FrameMetrics& aMetrics);
uint32_t Layer::GetFrameMetricsCount();
FrameMetrics Layer::GetFrameMetrics(uint32_t aIndex);

What order will the metrics be added in? Do you need the ability to rearrange them? How exposed does that need to be?

    
    

    
  
(In reply to Matt Woodrow (:mattwoodrow) from comment #50)
> I forget the exact details of how APZ is setup currently, but couldn't we
> make the FrameMetrics an actual shared object and have multiple layers
> reference the same instance? That way we keep one APZ per framemetrics, and
> we can store any per-layer data on the Layer.

We could do this, but that's a bit orthogonal because we'll still need a way to associate multiple FM objects with a single layer, and the Layer API will need to support that. Having the actual FM object be shared would be a memory optimization but wouldn't affect the code changes needed to get it working.

    
    

    
  
I filed bug 1055760 for having multiple FrameMetrics per layer.

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #51)
> *sigh* fine.. :p
> 
> I can do the gfx side changes needed, but what should the API look like?
> 
> void Layer::AddFrameMetrics(const FrameMetrics& aMetrics);
> uint32_t Layer::GetFrameMetricsCount();
> FrameMetrics Layer::GetFrameMetrics(uint32_t aIndex);

I think just
  void SetFrameMetrics(const nsTArray<FrameMetrics>& aMetrics);
  void GetFrameMetrics(nsTArray<FrameMetrics>& aMetricsOut);

We'll be resetting the entire list for each layer anyway.

    
    

    
  
Instead of GetFrameMetrics(nsTArray<>..) I'll probably do GetFrameMetricsCount() and GetFrameMetrics(uint32_t aIndex) and then also have GetAsyncPanZoomController(uint32_t aIndex), where both indices are bound by the GetFrameMetricsCount(). I would like to ensure that we never have more APZCs than FrameMetrics.

Also, I'm going to assume index 0 in the array will be the innermost metrics for the layer and indices 1..n will be for the "squashed" container layers as you ascend up the tree. That is, the innermost APZC will be at the smallest index in the array and so its transform will get applied on the layer first.

If any of the FrameMetrics indicate they are not scrollable then there will be no APZC at that index and the corresponding async transform will be empty.

Let me know if any of this sounds bad/wrong.

    
    

    
  
I started to make these changes (see WIPs on bug 1055760) but it's turning out to be more complicated (i.e. less of a mechanical change) than I anticipated. There's a bunch of places where the FrameMetrics is used that I don't really understand (with RenderFrameParent.cpp topping that list) and I'm not sure how to change to accomodate the new architecture. It also seems like there's just a lot of code in general that's going to get more complicated.

We discussed this during the gfx daily today and it was suggested that since FrameLayerBuilder is already pretty complicated it might make more sense to "contain" the complexity within that class. i.e. have one place super-complicated rather than have a lot of places moderately complicated.

Also, with respect to the post-processing approach mentioned above, will the extra IPC churn be in the form of many small messages, or just the existing messages getting larger? BenWa said that we do a lot worse for many small messages, but if it's just the existing messages getting a little larger it might not be so bad. (CC'ing him on this bug as well since I'm not familiar with the perf characteristics myself).

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #56)
> Also, with respect to the post-processing approach mentioned above, will the
> extra IPC churn be in the form of many small messages, or just the existing
> messages getting larger? BenWa said that we do a lot worse for many small
> messages, but if it's just the existing messages getting a little larger it
> might not be so bad. (CC'ing him on this bug as well since I'm not familiar
> with the perf characteristics myself).

I'm not sure, but I got some noticeable performance wins in B2G by reducing layer Mutated() churn a couple of years ago, and I'm loathe to give them up.

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #56)
> We discussed this during the gfx daily today and it was suggested that since
> FrameLayerBuilder is already pretty complicated it might make more sense to
> "contain" the complexity within that class. i.e. have one place
> super-complicated rather than have a lot of places moderately complicated.

I strongly disagree with that thought. In my experience, cutting something complicated into multiple parts reduces the pain rather than increases it --- if there is a good interface between the parts. And I think there is in this case; I think it's pretty clear what it means to have multiple FrameMetrics on a layer.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 1: Move mBackgroundColor from Layer to FrameMetrics (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #8473516 -
        Attachment is obsolete: true

        Attachment #8479862 -
        Flags: review?(bugmail.mozilla)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 2: Move mContentDescription from Layer to FrameMetrics (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
These patches are on top of the patches in bug 1055760

        Attachment #8479863 -
        Flags: review?(bugmail.mozilla)

    
    

    
  
Comment on attachment 8479862 [details] [diff] [review]
Part 1: Move mBackgroundColor from Layer to FrameMetrics

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

::: gfx/layers/composite/TiledContentHost.cpp
@@ +361,5 @@
>      // Background colors are only stored on scrollable layers. Grab
>      // the one from the nearest scrollable ancestor layer.
>      for (Layer* ancestor = GetLayer(); ancestor; ancestor = ancestor->GetParent()) {
>        if (LayerMetricsWrapper::HasScrollableMetrics(ancestor)) {
> +        backgroundColor = LayerMetricsWrapper::BottommostMetrics(ancestor).GetBackgroundColor();

This should probably be BottommostScrollableMetrics rather than BottommostMetrics. Or you could rewrite the loop to be like so:

for (LayerMetricsWrapper ancestor(GetLayer(), LayerMetricsWrapper::StartAt::BOTTOM); ancestor; ancestor = ancestor.GetParent()) {
  if (ancestor.Metrics().IsScrollable()) {
    backgroundColor = ancestor.Metrics().GetBackgroundColor();

        Attachment #8479862 -
        Flags: review?(bugmail.mozilla) → review+

    
    

    
  
Comment on attachment 8479863 [details] [diff] [review]
Part 2: Move mContentDescription from Layer to FrameMetrics

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

This is gonna be a problem; FrameMetrics is shared across processes and so can't contain pointers to things on the heap. See bug 1015278. If we absolutely have to put it back in FrameMetrics we can make it fixed-length like I did there (although maybe longer than 20 chars) and then maybe figure out a different solution in a follow-up bug.

        Attachment #8479863 -
        Flags: review?(bugmail.mozilla) → review-

    
    

    
  
Let's make it fixed-length then. (Why is it being put in shmem though???)

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #63)
> Let's make it fixed-length then.

Unfortunately, that's not so great for debugging (see bug 1023557). Perhaps we can have an array of strings in Layer instead?

> (Why is it being put in shmem though???)

I'm not familiar with the details, but the storing of FrameMetrics in shmem was introduced in bug 895358 as part of the implementation of progressive tile rendering.

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #63)
> Let's make it fixed-length then. (Why is it being put in shmem though???)

It gets shared to the content process on B2G to implement the draw aborting part of progressive tile rendering. That is, between drawing batches of tiles the content process checks to see where the APZC is at so that it can stop drawing tiles that will never be used.

I think a better long-term fix is to create a new data structure to house the things that the draw-aborting code needs (which is very small subset of all the things in FrameMetrics) and just copy that into the shmem.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 2: Move mContentDescription from Layer to FrameMetrics (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
I've updated this with a char array so we won't crash. Not sure what to do with it next. Use more chars? reformat the strings to fit into a smaller number of chars?

        Attachment #8479863 -
        Attachment is obsolete: true

    
    

    
  


  
This probably isn't quite ready for review, but it is ready for inspection and testing.

It passes reftest-ipc layout/reftests/async-scrolling/reftest.list for me (including some new tests that test split layers). I've fixed all the issues I know about right now.

I haven't yet written tests that exercise multiple FrameMetrics on a single layer. That's actually going to be rarer than I previously thought --- which is both good news and bad news, I guess! I think maybe the only condition which will trigger multiple FrameMetrics on a single layer are when the user activates scrolling on nested scrollable elements (not including the viewport, because viewport FrameMetrics get put on the ContainerLayer for that viewport, not its child layers ... is that an anomaly we should fix?). Maybe there are some interactive scenarios, e.g. scrollgrab, that could trigger that. It's also possible for scripts to trigger nested active scrolling but maybe we'd never see that in the wild.

    
    

    
  
My next thing to do here is to test the entire patch set on device to see if the scrolling actually works interactively.

    
    

    
  
Comment on attachment 8480350 [details] [diff] [review]
Part 2: Move mContentDescription from Layer to FrameMetrics

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

This is fine. I looked at what fields are used in the draw-aborting code and there's more than I thought originally. So I wrote another way to allow using nsCString in FrameMetrics, patch coming.

        Attachment #8480350 -
        Flags: review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Allow using dynamic strings in FrameMetrics (untested) (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
If we do this and make sure the string is cleared in MakePODObject() then we should be fine. I haven't tested this though and we can worry about it after getting the main stuff here landed. I don't want to waste time on this issue right now.

    
    

    
  


  
Rebased to master and includes the change from comment 61, since I had to rebase that code anyway.

        Attachment #8479862 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 1: Move mContentDescription from Layer to FrameMetrics (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Rebased part 2 and also fixed a bug where mContentDescription was left uninitialized in FrameMetrics. This was causing FrameMetrics::IsDefault() on a brand-new FrameMetrics to return false when it should have returned true. This led problems and not being able to scroll stuff.

        Attachment #8480350 -
        Attachment is obsolete: true

        Attachment #8480594 -
        Flags: review+

    
    

    
  


  
Whoops, wrong patch

        Attachment #8480594 -
        Attachment is obsolete: true

        Attachment #8480595 -
        Flags: review+

    
    

    
  


  
Rebased on latest master. Trivial rebase except the bits to LayersLogging.cpp are no longer needed.

        Attachment #8480353 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8480596 [details] [diff] [review]
Part 3: Setup FrameMetrics from FrameLayerBuilder based on animated geometry roots

Basic sanity testing on a B2G device with the pref enabled shows no obvious regressions.

        Attachment #8480596 -
        Flags: feedback+

    
    

    
  
Minor issue with class/struct messing up OS X and some other builds.  Here's another try (for those platforms) and I'll attach the modified part 3:
https://tbpl.mozilla.org/?tree=Try&rev=06b713a22394

    
    

    
  


  
:roc's slightly modified patch to build on OS X.

        Attachment #8480596 -
        Attachment is obsolete: true

        Attachment #8480596 -
        Flags: review?(matt.woodrow)

        Attachment #8481411 -
        Flags: review?(matt.woodrow)

    
    

    
  
Jet, we're looking at options, but since Matt is gone for the day, can you think of somebody else that can review FrameMetrics/FrameLayerBuilder changes so that we can land this today?
Flags: needinfo?(bugs)

    
    

    
  
Hmm, this mochitest failure looks real:
17950 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_leaf_layers_partition_browser_window.xul | Leaf layers should form a non-overlapping partition of the browser window (non-maximized). [Set MOZ_DUMP_PAINT_LIST=1 env var to investigate.]

    
    

    
  
Will update the patch for the emulator build error, here's try with it:
https://tbpl.mozilla.org/?tree=Try&rev=28099eb66a54

    
    

    
  


  
This should fix the emulator build error.

        Attachment #8481411 -
        Attachment is obsolete: true

        Attachment #8481411 -
        Flags: review?(matt.woodrow)

        Attachment #8481483 -
        Flags: review?(matt.woodrow)

    
    

    
  
(In reply to Milan Sreckovic [:milan] from comment #80)
> Hmm, this mochitest failure looks real:
> 17950 INFO TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/chrome/layout/base/tests/chrome/
> test_leaf_layers_partition_browser_window.xul | Leaf layers should form a
> non-overlapping partition of the browser window (non-maximized). [Set
> MOZ_DUMP_PAINT_LIST=1 env var to investigate.]

I retriggered this and it does persist. It's also not present on the m-c cset the try push was based off of, so it does look like a regression from these patches. Only Linux debug builds seems like.

    
    

    
  
(In reply to (away|aug30-sep3) Kartikaya Gupta (email:kats@mozilla.com) from comment #83)
> 
> I retriggered this and it does persist. It's also not present on the m-c
> cset the try push was based off of, so it does look like a regression from
> these patches. Only Linux debug builds seems like.

Perhaps, though that test fails by timing out on (all three) Windows debug runs and it's skipped on OS X :)

    
    

    
  

      
      
      
      

        Attached file
          
        Output of failing test with MOZ_DUMP_PAINT_LIST=1 (obsolete) (deleted)
        — 
      

    


  
Attached is the output of:

MOZ_DUMP_PAINT_LIST=1 ./mach mochitest-chrome layout/base/tests/chrome/test_leaf_layers_partition_browser_window.xul

        Attachment #8481632 -
        Flags: feedback?(tnikkel)

    
    

    
  


  

        Attachment #8481658 -
        Flags: feedback?(tnikkel)

    
    

    
  
The first thebes layer has h=71 before roc's patches, h=571 after. Also the second container layer loses it's opaque-ness with roc's patches. So something is up with visible regions I'm guessing.

    
    

    
  
Comment on attachment 8481483 [details] [diff] [review]
Part 3: Setup FrameMetrics from FrameLayerBuilder based on animated geometry roots

>+ScrollFrameHelper::ComputeFrameMetrics(Layer* aLayer,
>+                                       nsIFrame* aContainerReferenceFrame,
>+                                       const ContainerLayerParameters& aParameters,
>+                                       nsRect* aClipRect,
>+                                       nsTArray<FrameMetrics>* aOutput) const
>+{
>+  if (!mShouldBuildScrollableLayer || BuildScrollContainerLayers()) {
>+    return;
>+  }

For root scroll frames with display ports I don't think we are going to be setting mShouldBuildScrollableLayer or mScrollParentID because we have the IgnoreScrollFrame set and we take the early exit path in ScrollFrameHelper::BuildDisplayList. So either make sure they are set in the early exit or in nsSubDocumentFrame::BuildDisplayList I think.

    
    

    
  
Comment on attachment 8481483 [details] [diff] [review]
Part 3: Setup FrameMetrics from FrameLayerBuilder based on animated geometry roots

>       if (itemType == nsDisplayItem::TYPE_SCROLL_LAYER) {
>         nsDisplayScrollLayer* scrollItem = static_cast<nsDisplayScrollLayer*>(item);
>         newLayerEntry->mOpaqueForAnimatedGeometryRootParent =
>             scrollItem->IsDisplayPortOpaque();
>+        newLayerEntry->mBaseFrameMetrics =
>+            scrollItem->ComputeFrameMetrics(ownLayer, mParameters);
>+      }
>+      if (itemType == nsDisplayItem::TYPE_SUBDOCUMENT) {
>+        newLayerEntry->mBaseFrameMetrics =
>+          static_cast<nsDisplaySubDocument*>(item)->ComputeFrameMetrics(ownLayer, mParameters);
>       }

Aren't we going to need to do the same for scroll info layers here? Otherwise they wouldn't get any frame metrics attached to their layers?

    
    

    
  
(In reply to Timothy Nikkel (:tn) from comment #87)
> The first thebes layer has h=71 before roc's patches, h=571 after. Also the
> second container layer loses it's opaque-ness with roc's patches. So
> something is up with visible regions I'm guessing.

I've got a fix for this locally.

    
    

    
  
(In reply to Timothy Nikkel (:tn) from comment #88)
> >+ScrollFrameHelper::ComputeFrameMetrics(Layer* aLayer,
> >+                                       nsIFrame* aContainerReferenceFrame,
> >+                                       const ContainerLayerParameters& aParameters,
> >+                                       nsRect* aClipRect,
> >+                                       nsTArray<FrameMetrics>* aOutput) const
> >+{
> >+  if (!mShouldBuildScrollableLayer || BuildScrollContainerLayers()) {
> >+    return;
> >+  }
> 
> For root scroll frames with display ports I don't think we are going to be
> setting mShouldBuildScrollableLayer or mScrollParentID because we have the
> IgnoreScrollFrame set and we take the early exit path in
> ScrollFrameHelper::BuildDisplayList. So either make sure they are set in the
> early exit or in nsSubDocumentFrame::BuildDisplayList I think.

We don't want to return anything here. nsDisplaySubDocument will set FrameMetrics on its ContainerLayer and we don't want to set it again on the child layers.

    
    

    
  
(In reply to Timothy Nikkel (:tn) from comment #89)
> Aren't we going to need to do the same for scroll info layers here?
> Otherwise they wouldn't get any frame metrics attached to their layers?

Yeah I guess so.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 3 v3 (deleted)
        — 
          — Splinter Review
      

    


  
Fixes leaf_layers test locally. The problem was that when a scrolled frame's contents fills the scrollport opaquely and there is no displayport, we need to allow that to be added to the opaque region for the container layer even though they have different animated geometry roots. We handled that OK in ComputeOpaqueRect for the case where is a displayport, but not when there isn't. I've fixed that here in ComputeOpaqueRect.

Also fixed the issue with scrollinfo layers that tnikkel pointed out. That should also fix the regression botond found where we couldn't async-scroll an element when the containerless-scrolling pref is false.

I've also modified the patch to turn the containerless-scroll pref on.

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

        Attachment #8481483 -
        Attachment is obsolete: true

        Attachment #8481632 -
        Attachment is obsolete: true

        Attachment #8481658 -
        Attachment is obsolete: true

        Attachment #8481483 -
        Flags: review?(matt.woodrow)

        Attachment #8481632 -
        Flags: feedback?(tnikkel)

        Attachment #8481658 -
        Flags: feedback?(tnikkel)

        Attachment #8481782 -
        Flags: review?(matt.woodrow)

    
    

    
  
Comment on attachment 8481782 [details] [diff] [review]
Part 3 v3

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

This ended up being simpler than I expected!

::: layout/base/FrameLayerBuilder.cpp
@@ +3016,5 @@
>              scrollItem->IsDisplayPortOpaque();
> +        newLayerEntry->mBaseFrameMetrics =
> +            scrollItem->ComputeFrameMetrics(ownLayer, mParameters);
> +      }
> +      if (itemType == nsDisplayItem::TYPE_SUBDOCUMENT) {

else if?

@@ +3529,5 @@
> +      // both CSS and scroll clipping.
> +    }
> +  }
> +  aEntry->mLayer->SetClipRect(layerClip);
> +  aEntry->mLayer->SetFrameMetrics(metricsArray);

We do a lot of copying of FrameMetrics objects, and this function call will do a full equality check of all the FM fields for every item in the array (to see if it needs to call Mutated).

It might be worth retaining FrameMetrics objects at some point so we can do pointer comparison. Maybe add a note that we should look out for this in profiles.

::: layout/base/nsDisplayList.cpp
@@ +3694,5 @@
> +  nsRect viewport = mFrame->GetRect() -
> +                    mFrame->GetPosition() +
> +                    mFrame->GetOffsetToCrossDoc(ReferenceFrame());
> +
> +  return UniquePtr<FrameMetrics>(new FrameMetrics(

Isn't MakeUnique<FrameMetrics>(...) the recommended way of doing this?

::: layout/generic/nsIScrollableFrame.h
@@ +356,5 @@
> +   */
> +  virtual void ComputeFrameMetrics(mozilla::layers::Layer* aLayer,
> +                                   nsIFrame* aContainerReferenceFrame,
> +                                   const ContainerLayerParameters& aParameters,
> +                                   nsRect* aClipRect,

Mention that this is an out param.

        Attachment #8481782 -
        Flags: review?(matt.woodrow) → review+
Depends on: 1061327

    
    

    
  
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b03db711b80f for what will no doubt prove an annoying failure in robopan thanks to its opaque and generic message (permared, but we didn't notice for hours since it only has the one failure message for every sort of failure that's happened over the past two years), https://tbpl.mozilla.org/php/getParsedLog.php?id=47182454&tree=Mozilla-Inbound

    
    

    
  
Try push with lots of logging in testPan:
https://tbpl.mozilla.org/?tree=Try&rev=e32aca06c4de
Same, but with containerless scrolling pref disabled:
https://tbpl.mozilla.org/?tree=Try&rev=02b366e8f07a

    
    

    
  
17:22:41     INFO -  09-01 17:21:24.968 I/Robocop ( 2198): testPan: called mActions.drag; mDriver.getScrollHeight()=144 mDriver.getHeight()=472 mDriver.getPageHeight()=14083
17:22:41     INFO -  09-01 17:21:25.937 I/Robocop ( 2198): testPan: called mActions.drag; mDriver.getScrollHeight()=144 mDriver.getHeight()=472 mDriver.getPageHeight()=14083
17:22:41     INFO -  09-01 17:21:26.921 I/Robocop ( 2198): testPan: called mActions.drag; mDriver.getScrollHeight()=144 mDriver.getHeight()=472 mDriver.getPageHeight()=14083
17:22:41     INFO -  09-01 17:21:27.882 I/Robocop ( 2198): testPan: called mActions.drag; mDriver.getScrollHeight()=144 mDriver.getHeight()=472 mDriver.getPageHeight()=14083
17:22:41     INFO -  09-01 17:21:28.867 I/Robocop ( 2198): testPan: called mActions.drag; mDriver.getScrollHeight()=144 mDriver.getHeight()=472 mDriver.getPageHeight()=14083
17:22:41     INFO -  09-01 17:21:29.867 I/Robocop ( 2198): testPan: called mActions.drag; mDriver.getScrollHeight()=144 mDriver.getHeight()=472 mDriver.getPageHeight()=14083
17:22:41     INFO -  09-01 17:21:30.820 I/Robocop ( 2198): testPan: called mActions.drag; mDriver.getScrollHeight()=144 mDriver.getHeight()=472 mDriver.getPageHeight()=14083
17:22:41     INFO -  09-01 17:21:31.804 I/Robocop ( 2198): testPan: called mActions.drag; mDriver.getScrollHeight()=144 mDriver.getHeight()=472 mDriver.getPageHeight()=14083

The drag() calls are not working. This is on a Wikipedia page which should not be complex, so I guess the next thing to do is to build Fennec and make sure touch-panning actually works at all.

    
    

    
  
I tried running the tests locally (by enabling testPan in robocop.ini and running mochitest-robocop) and I don't see the problem; mDriver.getScrollHeight() increases as expected.

    
    

    
  
Try push with layers.dump enabled:
https://tbpl.mozilla.org/?tree=Try&rev=f6a16aba409c
This should tell us whether scrolling is happening or not.

    
    

    
  
It should also tell us whether the right scrollinfo is available. Though we should be scrolling the viewport here so scrollinfo should not be an issue.

    
    

    
  
Botond, it might be worth you trying to reproduce this bug running trobopan locally.

I copied the wikipeda.html from talos into mobile/android/base/tests, edited it to remove a favicon and background-image that refer to http: resources (which causes tests to fail), and then edited testPan.java with
-        String url = getAbsoluteUrl("/startup_test/fennecmark/wikipedia.html");
+        String url = getAbsoluteUrl("/robocop/wikipedia.html");
and added testPan to mobile/android/base/tests/robocop.ini:
+[testPan]

Then I could run the test with TEST_PATH=testPan make mochitest-robocop. A bit hacky, so I hope something I hacked there isn't responsible for making the test work (or not).
Flags: needinfo?(botond)

    
    

    
  
22:41:13     INFO -  09-01 22:39:05.898 I/Robocop ( 2199): testPan: called mActions.drag; mDriver.getScrollHeight()=144 mDriver.getHeight()=472 mDriver.getPageHeight()=14083
22:41:13     INFO -  09-01 22:39:05.914 I/Gecko   ( 2199): LayerManager (0x6c9d8200)
22:41:13     INFO -  09-01 22:39:05.914 I/Gecko   ( 2199):   ContainerLayerComposite (0x6d941800) [shadow-transform=[ 1.30612 0; 0 1.30612; 0 -188.571; ]] [shadow-visible=< (x=0, y=0, w=980, h=472); >] [visible=< (x=0, y=0, w=980, h=472); >] [metrics0={ cb=(x=0.000000, y=0.000000, w=1280.000000, h=616.000000) sr=(x=0.000000, y=0.000000, w=1280.000000, h=616.000000) s=(0,0) dp=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) cdp=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) color=rgba(0, 0, 0, 0) scrollId=0 z=1.000 }]
22:41:13     INFO -  09-01 22:39:05.914 I/Gecko   ( 2199):     ContainerLayerComposite (0x6ed50400) [shadow-clip=(x=0, y=0, w=1280, h=616)] [shadow-transform=[ 1.30612 0; 0 1.30612; 0 0; ]] [shadow-visible=< (x=0, y=0, w=1280, h=617); >] [clip=(x=0, y=0, w=1280, h=616)] [postScale=0.765625, 0.765625] [transform=[ 1.30612 0; 0 1.30612; 0 0; ]] [visible=< (x=0, y=0, w=1280, h=617); >] [preScale=0.765625, 0.765625]
22:41:13     INFO -  09-01 22:39:05.914 I/Gecko   ( 2199):       ThebesLayerComposite (0x6f2a1800) [shadow-transform=[ 1 0; 0 1; 0 -188; ]] [shadow-visible=< (x=1000, y=-1, w=61, h=1); (x=1099, y=-1, w=153, h=1); (x=0, y=0, w=1280, h=2304); >] [transform=[ 1 0; 0 1; 0 -188; ]] [visible=< (x=1000, y=-1, w=61, h=1); (x=1099, y=-1, w=153, h=1); (x=0, y=0, w=1280, h=2304); >] [valid=< (x=1000, y=-1, w=61, h=1); (x=1099, y=-1, w=153, h=1); (x=0, y=0, w=1280, h=2305); (x=0, y=2305, w=1069, h=9); (x=1195, y=2305, w=85, h=9); (x=0, y=2314, w=599, h=4); (x=722, y=2314, w=347, h=4); (x=1195, y=2314, w=85, h=4); (x=0, y=2318, w=30, h=1); (x=98, y=2318, w=501, h=1); (x=722, y=2318, w=347, h=1); (x=1195, y=2318, w=85, h=1); (x=0, y=2319, w=30, h=7); (x=98, y=2319, w=120, h=7); (x=722, y=2319, w=347, h=7); (x=1195, y=2319, w=85, h=7); (x=0, y=2326, w=30, h=3); (x=98, y=2326, w=120, h=3); (x=722, y=2326, w=308, h=3); (x=1234, y=2326, w=46, h=3); (x=0, y=2329, w=14, h=10); (x=98, y=2329, w=120, h=10); (x=722, y=2329, w=308, h=10); (x=1234, y=2329, w=46, h=10); (x=0, y=2339, w=14, h
22:41:13     INFO -  09-01 22:39:05.914 I/Gecko   ( 2199):         TiledContentHost (0x6f309e00)



22:41:13     INFO -  09-01 22:39:06.796 I/Robocop ( 2199): testPan: called mActions.drag; mDriver.getScrollHeight()=144 mDriver.getHeight()=472 mDriver.getPageHeight()=14083
22:41:13     INFO -  09-01 22:39:06.812 I/Gecko   ( 2199): LayerManager (0x6c9d8200)
22:41:13     INFO -  09-01 22:39:06.812 I/Gecko   ( 2199):   ContainerLayerComposite (0x6d941800) [shadow-transform=[ 1.30612 0; 0 1.30612; 0 -188.571; ]] [shadow-visible=< (x=0, y=0, w=980, h=472); >] [visible=< (x=0, y=0, w=980, h=472); >] [metrics0={ cb=(x=0.000000, y=0.000000, w=1280.000000, h=616.000000) sr=(x=0.000000, y=0.000000, w=1280.000000, h=616.000000) s=(0,0) dp=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) cdp=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) color=rgba(0, 0, 0, 0) scrollId=0 z=1.000 }]
22:41:13     INFO -  09-01 22:39:06.812 I/Gecko   ( 2199):     ContainerLayerComposite (0x6ed50400) [shadow-clip=(x=0, y=0, w=1280, h=616)] [shadow-transform=[ 1.30612 0; 0 1.30612; 0 0; ]] [shadow-visible=< (x=0, y=0, w=1280, h=617); >] [clip=(x=0, y=0, w=1280, h=616)] [postScale=0.765625, 0.765625] [transform=[ 1.30612 0; 0 1.30612; 0 0; ]] [visible=< (x=0, y=0, w=1280, h=617); >] [preScale=0.765625, 0.765625]
22:41:13     INFO -  09-01 22:39:06.812 I/Gecko   ( 2199):       ThebesLayerComposite (0x6f2a1800) [shadow-transform=[ 1 0; 0 1; 0 -188; ]] [shadow-visible=< (x=1000, y=-1, w=61, h=1); (x=1099, y=-1, w=153, h=1); (x=0, y=0, w=1280, h=2304); >] [transform=[ 1 0; 0 1; 0 -188; ]] [visible=< (x=1000, y=-1, w=61, h=1); (x=1099, y=-1, w=153, h=1); (x=0, y=0, w=1280, h=2304); >] [valid=< (x=1000, y=-1, w=61, h=1); (x=1099, y=-1, w=153, h=1); (x=0, y=0, w=1280, h=2305); (x=0, y=2305, w=1069, h=9); (x=1195, y=2305, w=85, h=9); (x=0, y=2314, w=599, h=4); (x=722, y=2314, w=347, h=4); (x=1195, y=2314, w=85, h=4); (x=0, y=2318, w=30, h=1); (x=98, y=2318, w=501, h=1); (x=722, y=2318, w=347, h=1); (x=1195, y=2318, w=85, h=1); (x=0, y=2319, w=30, h=7); (x=98, y=2319, w=120, h=7); (x=722, y=2319, w=347, h=7); (x=1195, y=2319, w=85, h=7); (x=0, y=2326, w=30, h=3); (x=98, y=2326, w=120, h=3); (x=722, y=2326, w=308, h=3); (x=1234, y=2326, w=46, h=3); (x=0, y=2329, w=14, h=10); (x=98, y=2329, w=120, h=10); (x=722, y=2329, w=308, h=10); (x=1234, y=2329, w=46, h=10); (x=0, y=2339, w=14, h
22:41:13     INFO -  09-01 22:39:06.812 I/Gecko   ( 2199):         TiledContentHost (0x6f309e00)

    
    

    
  
Here are similar logs from before my patches:

23:51:21     INFO -  09-01 23:50:10.390 I/Gecko   ( 2677): LayerManager (0x6cc57200)
23:51:21     INFO -  09-01 23:50:10.390 I/Gecko   ( 2677):   ContainerLayerComposite (0x6d941800) [shadow-visible=< (x=0, y=0, w=980, h=472); >] [visible=< (x=0, y=0, w=980, h=472); >] [metrics0={ cb=(x=0.000000, y=0.000000, w=1280.000000, h=616.000000) sr=(x=0.000000, y=0.000000, w=1280.000000, h=616.000000) s=(0,0) dp=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) cdp=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) scrollId=0 z=1.000 }]
23:51:21     INFO -  09-01 23:50:10.390 I/Gecko   ( 2677):     ContainerLayerComposite (0x6ecbbc00) [shadow-clip=(x=0, y=0, w=1280, h=616)] [shadow-transform=[ 1.70596 0; 0 1.70596; 0 -3870.14; ]] [shadow-visible=< (x=0, y=0, w=1280, h=617); >] [clip=(x=0, y=0, w=1280, h=616)] [postScale=0.765625, 0.765625] [transform=[ 1.30612 0; 0 1.30612; 0 0; ]] [visible=< (x=0, y=0, w=1280, h=617); >] [metrics0={ cb=(x=0.000000, y=0.000000, w=1280.000000, h=616.000000) sr=(x=0.000000, y=0.000000, w=980.000000, h=14083.183594) s=(0,9352.12) dp=(x=0.000000, y=-2394.116699, w=980.000000, h=5488.000000) cdp=(x=0.000000, y=-336.116669, w=980.000000, h=1372.000000) scrollId=3 z=1.306 }] [preScale=0.765625, 0.765625]
23:51:21     INFO -  09-01 23:50:10.390 I/Gecko   ( 2677):       ThebesLayerComposite (0x6ecc3400) [shadow-transform=[ 1 0; 0 1; 0 -12215; ]] [shadow-visible=< (x=0, y=9087, w=1280, h=7169); >] [transform=[ 1 0; 0 1; 0 -12215; ]] [visible=< (x=0, y=9087, w=1280, h=7169); >] [opaqueContent] [valid=< (x=0, y=11775, w=1280, h=257); (x=0, y=12032, w=256, h=256); (x=0, y=12544, w=1280, h=512); >]
23:51:21     INFO -  09-01 23:50:10.390 I/Gecko   ( 2677):         TiledContentHost (0x6f19be00)



23:51:21     INFO -  09-01 23:50:10.421 I/Gecko   ( 2677): LayerManager (0x6cc57200)
23:51:21     INFO -  09-01 23:50:10.421 I/Gecko   ( 2677):   ContainerLayerComposite (0x6d941800) [shadow-visible=< (x=0, y=0, w=980, h=472); >] [visible=< (x=0, y=0, w=980, h=472); >] [metrics0={ cb=(x=0.000000, y=0.000000, w=1280.000000, h=616.000000) sr=(x=0.000000, y=0.000000, w=1280.000000, h=616.000000) s=(0,0) dp=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) cdp=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) scrollId=0 z=1.000 }]
23:51:21     INFO -  09-01 23:50:10.421 I/Gecko   ( 2677):     ContainerLayerComposite (0x6ecbbc00) [shadow-clip=(x=0, y=0, w=1280, h=616)] [shadow-transform=[ 1.70596 0; 0 1.70596; 0 -1380.76; ]] [shadow-visible=< (x=0, y=0, w=1280, h=617); >] [clip=(x=0, y=0, w=1280, h=616)] [postScale=0.765625, 0.765625] [transform=[ 1.30612 0; 0 1.30612; 0 0; ]] [visible=< (x=0, y=0, w=1280, h=617); >] [metrics0={ cb=(x=0.000000, y=0.000000, w=1280.000000, h=616.000000) sr=(x=0.000000, y=0.000000, w=980.000000, h=14083.183594) s=(0,10936.2) dp=(x=0.000000, y=-2410.183350, w=980.000000, h=5488.000000) cdp=(x=0.000000, y=-352.183319, w=980.000000, h=1372.000000) scrollId=3 z=1.306 }] [preScale=0.765625, 0.765625]
23:51:21     INFO -  09-01 23:50:10.421 I/Gecko   ( 2677):       ThebesLayerComposite (0x6ecc3400) [shadow-transform=[ 1 0; 0 1; 0 -14284; ]] [shadow-visible=< (x=0, y=11136, w=1280, h=7169); >] [transform=[ 1 0; 0 1; 0 -14284; ]] [visible=< (x=0, y=11136, w=1280, h=7169); >] [opaqueContent]
23:51:21     INFO -  09-01 23:50:10.421 I/Gecko   ( 2677):         TiledContentHost (0x6f19be00)

    
    

    
  
The key difference is that before my patches, the child ContainerLayerComposite has FrameMetrics and after my patches it doesn't...

    
    

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

I fixed the bug by extending the check for nsDisplayItem::TYPE_SUBDOCUMENT to include the other types that use nsDisplaySubdocument --- TYPE_ZOOM and TYPE_RESOLUTION.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/01df38521c54
https://hg.mozilla.org/mozilla-central/rev/e2dddf6f950b
https://hg.mozilla.org/mozilla-central/rev/ea4cfd84e417
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35

    
    

    
  
Clearing needinfo as it looks from comment 110 like the issue in question has been fixed.
Flags: needinfo?(botond)

    
    

    
  
Comment on attachment 8481782 [details] [diff] [review]
Part 3 v3

Approval Request Comment
[Feature/regressing bug #]: none
[User impact if declined]: slow jerky scrolling in some sites and apps
[Describe test coverage new/current, TBPL]: many automated tests cover this.
[Risks and why]: this is a feature that we promised for FxOS 2.1 and missed the Aurora uplift by several hours. The risk is relatively low since the majority of the changes can be preffed off if needed.
[String/UUID change made/needed]: none

        Attachment #8481782 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Comment on attachment 8481782 [details] [diff] [review]
Part 3 v3

approving given this missed the merged by a few hours and was a feature on-track for 2.1. if we see major fallouts that are risky to fix in 2.1 timefrmae we'll consider the pref-off option as necessary.

        Attachment #8481782 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1062411
Depends on: 1062545
No longer blocks: 1062411
Depends on: 1062411

    
    

    
  
Comment on attachment 8480537 [details] [diff] [review]
Allow using dynamic strings in FrameMetrics (untested)

Obsoleted by bug 1062411

        Attachment #8480537 -
        Attachment is obsolete: true
Depends on: 1063101
Depends on: 1062516

    
  
Depends on: 1063494
Depends on: 1063046

    
  
Depends on: 1064918
Depends on: 1062792
Depends on: 1068961
Depends on: 1073290
Depends on: 1089941

    
  
Blocks: 939239

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: