Botond Ballo [:botond] Assignee
	Description • 11 years ago

The way resolutions and transforms are set when zooming in on Metro is presenting a difficulty for coming up with a consistent definition for "composition bounds" (bug 935219).

Generally, when zooming in on a layer L, two things happen. Suppose we were initially at a zoom of 1 and we are zooming in by a factor of 2x:

  1) The pres shell associated with L gets a resolution equal to the zoom, i.e. 2.0.
     By "pres shell associated with L" I mean the pres shell of the scrollable
     frame from which L was built.

  2) The layer L gets a CSS transform which is the inverse of the zoom, i.e. 0.5.
     Specifically, L->GetTransform() should return a matrix with scale 0.5.
     (L->GetTransform() actually consists of a pre-scale, base transform, and post-scale.
     The APZ doesn't really care about the components, just the final result.)

The idea is that the CSS transform compensates for the resolution, effectively cancelling each other out. (The actual zooming is then due to another transform matrix managed by APZ, the async transform.)

The above is what happens on B2G.

On Metro, something slightly different happens: instead of L getting the CSS transform, it is L's parent layer - let's call it M - which gets the transform.

This would be fine for APZ as long as the resolution was also set, not on the pres shell associated with L, but its parent pres shell.

Unfortunately, this does not happen. It seems that on Metro the CSS transform is set on L's parent layer M, but the resolution is set on the pres shell associated with L.

Does anyone know why this inconsistency - between where the resolution is set and where the transform is set - exists, and whether it is necessary?

	Timothy Nikkel (:tnikkel)
	Comment 1 • 11 years ago

Is L the scrollable layer (layer created by a scroll layer item) for the document in both cases? Who creates the layer M?

	Botond Ballo [:botond] Assignee
	Comment 2 • 11 years ago

(In reply to Timothy Nikkel (:tn) from comment #1)
> Is L the scrollable layer (layer created by a scroll layer item) for the
> document in both cases?

aScrollFrame->GetContent() in the RecordFrameMetrics() call for the layer is the HTML element, so I guess so.

> Who creates the layer M?

I will look into this.

	Timothy Nikkel (:tnikkel)
	Comment 3 • 11 years ago

Which RecordFrameMetrics call is this? The one from PaintForFrame or from a scroll layer item? That might be the difference there. RecordFrameMetrics in PaintForFrame records it on the container layer for the document, RecordFrameMetrics from the scroll layer item records it on the container layer for the scroll frame.

Can we remove the RecordFrameMetrics call from PaintForFrame and make nsGfxScrollFrame always build a scroll info layer, even for root scroll frames root content documents? It seems like that check only makes sense if PaintForFrame is called on the root content document (like on b2g).

	Timothy Nikkel (:tnikkel)
	Comment 4 • 11 years ago

Alternatively I think kats had a patch in some bug that added a RecordFrameMetrics call for subdocuments.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 5 • 11 years ago

The patch (probably much bitrotted) is at https://github.com/staktrace/mozilla-central/commit/4aae77913a7bbeced7699c512e612de0ba04b33b

	Botond Ballo [:botond] Assignee
	Comment 6 • 11 years ago

Looking into this further, it seems that there is no problem with where the resolution and transform are being set, only with how RecordFrameMetrics() decides to ascribe a resolution to a given scrollable layer.

Since pres shells are per-document, the same pres shell can be associated with multiple layers. It seems that when a document is zoomed in, the corresponding CSS transforms are set on the root layer of the document. RecordFrameMetrics(), however, ascribes the pres shell resolution to the root _scrollable_ layer in the document. On B2G, the root layer of the content document is scrollable, (so the root layer and the root scrollable layer are the same), so there is no problem. On Metro, however, the root layer of the content document is not scrollable, so the root layer and root scrollable layer are different, and we get this mismatch.

The fix is therefore simple: RecordFrameMetrics() should only ascribe the pres shell resolution to a scrollable layer if that is the root layer of the document. Patch forthcoming.

	Timothy Nikkel (:tnikkel)
	Comment 8 • 11 years ago

Comment on attachment 8355249 [details] [diff] [review]
bug951936.patch

I think the first thing we need to do is be consistent about when we call RecordFrameMetrics. Right now we call it for scroll layer items and root layers in display root documents. So whether a tab is remote or not changes if we call RecordFrameMetrics on it's root layer or not (remote tabs are display roots, non remote ones are not). Without that consistency this patch is confusing.

	Botond Ballo [:botond] Assignee
	Comment 9 • 11 years ago

(In reply to Timothy Nikkel (:tn) from comment #8)
> Comment on attachment 8355249 [details] [diff] [review]
> bug951936.patch
> 
> I think the first thing we need to do is be consistent about when we call
> RecordFrameMetrics. Right now we call it for scroll layer items and root
> layers in display root documents. So whether a tab is remote or not changes
> if we call RecordFrameMetrics on it's root layer or not (remote tabs are
> display roots, non remote ones are not). Without that consistency this patch
> is confusing.

There is another consistency which currently exists, and which I'd like to maintain: that RecordFrameMetrics() gets called precisely for scrollable layers.

Specifically, I would not like to add a call to RecordFrameMetrics() on the root layer of a document if that layer is not scrollable.

On the other hand, it does not seem that we can remove the call to RecordFrameMetrics() for the root layer of a display root document, because that is the only scrollable layer.

Do you know what is the mechanism by which, on B2G, where tabs are remote, the root scroll layer item does _not_ create an _additional_ scrollable layer? Perhaps understanding that will help us reach an appropriate solution.

	Botond Ballo [:botond] Assignee
	Comment 10 • 11 years ago

(In reply to Botond Ballo [:botond] from comment #9)
> On the other hand, it does not seem that we can remove the call to
> RecordFrameMetrics() for the root layer of a display root document, because
> that is the only scrollable layer.

Sorry, I meant to say "that is the only scrollable layer on B2G (with no subframes in the content)".

	Timothy Nikkel (:tnikkel)
	Comment 11 • 11 years ago

(In reply to Botond Ballo [:botond] from comment #9)
> (In reply to Timothy Nikkel (:tn) from comment #8)
> > Comment on attachment 8355249 [details] [diff] [review]
> > bug951936.patch
> > 
> > I think the first thing we need to do is be consistent about when we call
> > RecordFrameMetrics. Right now we call it for scroll layer items and root
> > layers in display root documents. So whether a tab is remote or not changes
> > if we call RecordFrameMetrics on it's root layer or not (remote tabs are
> > display roots, non remote ones are not). Without that consistency this patch
> > is confusing.
> 
> There is another consistency which currently exists, and which I'd like to
> maintain: that RecordFrameMetrics() gets called precisely for scrollable
> layers.

What do you mean by scrollable layer here? Right now I don't see any consistency in how we are doing this.

> Specifically, I would not like to add a call to RecordFrameMetrics() on the
> root layer of a document if that layer is not scrollable.
> 
> On the other hand, it does not seem that we can remove the call to
> RecordFrameMetrics() for the root layer of a display root document, because
> that is the only scrollable layer.

I don't think we should remove it. I think we should add more RecordFrameMetrics calls.

> Do you know what is the mechanism by which, on B2G, where tabs are remote,
> the root scroll layer item does _not_ create an _additional_ scrollable
> layer? Perhaps understanding that will help us reach an appropriate solution.

Answering this hinges on what you are calling a scrollable layer. Anyways I'd like the see the layer dumps for when you are seeing this.

	Timothy Nikkel (:tnikkel)
	Comment 12 • 11 years ago

The patch in bug 942189 will probably fix this.

	Botond Ballo [:botond] Assignee
	Comment 13 • 11 years ago

(In reply to Timothy Nikkel (:tn) from comment #12)
> The patch in bug 942189 will probably fix this.

Assuming you're talking about the "unifying RecordFrameMetrics calls" patch - I think that by making B2G behave more like Metro, that patch actually breaks B2G in the same way that Metro is broken. That's not a bad thing, though, as the platforms are now consistent. It does mean that a variation of my patch here will be required as well.

	Botond Ballo [:botond] Assignee
	Comment 14 • 11 years ago

In fact, the patch in bug 942189 allows me to simplify this patch significantly: now the layer for which RecordFrameMetrics is called is never the root layer of a document, so the resolution should alwaysbe 1.0.

	Botond Ballo [:botond] Assignee
	Comment 18 • 11 years ago

After discussing the underlying issues with Timothy on IRC, we came up with a plan that combines:

 Part 1: a change that prevents ScrollFrameHelper::BuildDisplayList from exiting early
 Part 2: the "Unify RecordFrameMetrics" patch from bug 942189, with the change I suggested 
         in my feedback at that bug
 Part 3: my patch from comment #14

Kats, if you prefer that some else reviewed Part 2, please flag accordingly (not Timothy as he is the original author).

	Botond Ballo [:botond] Assignee
	Comment 19 • 11 years ago

Here is a combined try push for these patches and the ones from bug 935219: https://tbpl.mozilla.org/?tree=Try&rev=27c4af511cbc.

	Botond Ballo [:botond] Assignee
	Comment 20 • 11 years ago

(In reply to Botond Ballo [:botond] from comment #18)
> After discussing the underlying issues with Timothy on IRC, we came up with
> a plan

To summarize our findings in a little more detail:

  - Without these patches, Metro and B2G have different layer structures near
    the root scrollable layer. Metro has a root layer which is not scrollable,
    and a child of it, which is scrollable (and which corresponds to the
    root scroll frame). B2G just has a root layer which is scrollable.

      - The reason Metro's root layer is not scrollable is that the root
        layer being scrollable happens because nsDisplayList::PaintForFrame()
        calls RecordFrameMetrics(), but this function is only called for 
        display root documents. On Metro, the root content document is not
        a display root document (on B2G it is).

      - The reason B2G doesn't have a child scrollable layer corresponding
        to the root scroll frame is that for that scroll frame, the
        'ignore viewport scrolling' flag is set, and this causes
        ScrollFrameHelper::BuildDisplayList() to exit early [1].

  - We want the layer structures to be the same on B2G and Metro. To do this,
    we decided to:
      
      - Remove the call to RecordFrameMetrics from nsDisplayList::PaintForFrame().
        The only things that now create scrollable layers are scroll frames.
        Note that this means a document will always have a non-scrollable root layer.
        This is the Part 2 patch.

      - Ensure that the root scroll frame creates a scrollable layer by not
        setting the 'ignore viewport scrolling' flag on it which causes
        ScrollFrameHelper::BuildDisplayList() to exit early.
        This is the Part 1 patch.

  - Finally, since the root layer of a document is now never scrollable, the CSS 
    transform that is set on the root layer to compensate for the pres shell
    resolution is now always on an ancestor layer of a scrollable layer rather
    than the scrollable layer itself. Accordingly, RecordFrameMetrics needs to
    include the pres shell resolution in FrameMetrics::mCumulativeResolution
    but not in FrameMetrics::mResolution.
    This is the Part 3 patch.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2299

	Timothy Nikkel (:tnikkel)
	Comment 21 • 11 years ago

(In reply to Botond Ballo [:botond] from comment #20)
>       - The reason B2G doesn't have a child scrollable layer corresponding
>         to the root scroll frame is that for that scroll frame, the
>         'ignore viewport scrolling' flag is set, and this causes
>         ScrollFrameHelper::BuildDisplayList() to exit early [1].

To clarify this point: the 'ignore viewport scrolling' flag is set is always set when we set a display port. But the only place that looks at that flag is nsLayoutUtils::PaintFrame, so it only has an effect on display roots.

	Timothy Nikkel (:tnikkel)
	Comment 22 • 11 years ago

Comment on attachment 8355696 [details] [diff] [review]
Part 1 - Do not set the 'ignore viewport scrolling' flag for a root scroll frame

Just noting this here (we can do the cleanup later): this should allow us to remove some display port related code from nsLayoutUtils::PaintFrame.

	Timothy Nikkel (:tnikkel)
	Comment 23 • 11 years ago

Comment on attachment 8355697 [details] [diff] [review]
Part 2 - Unify RecordFrameMetrics calls

This was just a quick patch I wrote, there are probably more simplifications that can be made.

	Timothy Nikkel (:tnikkel)
	Comment 24 • 11 years ago

Comment on attachment 8355698 [details] [diff] [review]
Part 3 - Assign resolution correctly in RecordFrameMetrics

If mResolution is always 1 do we even need it?

	Botond Ballo [:botond] Assignee
	Comment 25 • 11 years ago

(In reply to Timothy Nikkel (:tn) from comment #24)
> Comment on attachment 8355698 [details] [diff] [review]
> Part 3 - Assign resolution correctly in RecordFrameMetrics
> 
> If mResolution is always 1 do we even need it?

I think that field can be assigned to elsewhere in APZ code. Also some of the APZ tests set a resolution that's not 1. We may want to look into removing the field but I think we can do that as a follow-up.

	Timothy Nikkel (:tnikkel)
	Comment 26 • 11 years ago

Comment on attachment 8355698 [details] [diff] [review]
Part 3 - Assign resolution correctly in RecordFrameMetrics

This is fine with me (simpler code!), but the AZPC is the place where we actually use this value.

	Timothy Nikkel (:tnikkel)
	Comment 27 • 11 years ago

Actually, I think we should fix the stuff in nsLayoutUtils::PaintFrame now because we don't want to have the visible region being incorrect.

We're moving from setting ignore viewport scrolling on the presshell, making the visible region the display port in nsLayoutUtils::PaintFrame, and taking the early exit for ignored scroll frames in ScrollFrameHelper::BuildDisplayList to handling all this in ScrollFrameHelper::BuildDisplayList like all other scroll frames (with scroll layer items etc).

	Timothy Nikkel (:tnikkel)
	Comment 28 • 11 years ago

Comment on attachment 8355696 [details] [diff] [review]
Part 1 - Do not set the 'ignore viewport scrolling' flag for a root scroll frame

It would be nice if we could get a little bit more explanation of this change in the commit msg, not on the first line of course.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 29 • 11 years ago

(In reply to Timothy Nikkel (:tn) from comment #23)
> Comment on attachment 8355697 [details] [diff] [review]
> Part 2 - Unify RecordFrameMetrics calls
> 
> This was just a quick patch I wrote, there are probably more simplifications
> that can be made.

Do you have any specific simplifications in mind? Feel free to upload a new patch with any additional changes you want to put in.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 30 • 11 years ago

Comment on attachment 8355698 [details] [diff] [review]
Part 3 - Assign resolution correctly in RecordFrameMetrics

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

::: layout/base/nsDisplayList.cpp
@@ +651,5 @@
>  
> +  // Only the root layer for a given presShell should pick up
> +  // the presShell's resolution. Since the root layer is never
> +  // scrollable, the resolution we assign here is always 1.0.
> +  metrics.mResolution = ParentLayerToLayerScale(1.0f);

It might be worth adding an assert here to verify that this is not the root layer. What do you think?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 31 • 11 years ago

Also we should test this on Fennec. Some of these changes might affect the layer tree there and since the PZC code is different it might have unexpected consequences. A basic panning/zooming smoketest on the following pages should be good enough, I think:

https://people.mozilla.org/~kgupta/grid.html
https://people.mozilla.org/~kgupta/gridframe.html
http://chrislord.net/files/mozilla/fixed.html

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 32 • 11 years ago

Comment on attachment 8355697 [details] [diff] [review]
Part 2 - Unify RecordFrameMetrics calls

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

This looks ok to me based on the discussion above and I'll give it an r+ but I'm probably not the best person to review this. I don't know who else understands this code well enough to review - maybe mattwoodrow? Will wait for any updated patch from tn before requesting a review from him.

	Timothy Nikkel (:tnikkel)
	Comment 33 • 11 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #29)
> (In reply to Timothy Nikkel (:tn) from comment #23)
> > Comment on attachment 8355697 [details] [diff] [review]
> > Part 2 - Unify RecordFrameMetrics calls
> > 
> > This was just a quick patch I wrote, there are probably more simplifications
> > that can be made.
> 
> Do you have any specific simplifications in mind? Feel free to upload a new
> patch with any additional changes you want to put in.

I just went with the 100% for sure changes, I think the remaining ones can be a followup, so we can deal with any regressions from those separately.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 34 • 11 years ago

Comment on attachment 8355697 [details] [diff] [review]
Part 2 - Unify RecordFrameMetrics calls

Sounds good. Requesting review from Matt as it would be good to get another pair of eyes on this.

	Botond Ballo [:botond] Assignee
	Comment 35 • 11 years ago

(In reply to Timothy Nikkel (:tn) from comment #28)
> Comment on attachment 8355696 [details] [diff] [review]
> Part 1 - Do not set the 'ignore viewport scrolling' flag for a root scroll
> frame
> 
> It would be nice if we could get a little bit more explanation of this
> change in the commit msg, not on the first line of course.

Uploaded patch with updated commit message. Carrying r+.

	Botond Ballo [:botond] Assignee
	Comment 36 • 11 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #30)
> Comment on attachment 8355698 [details] [diff] [review]
> Part 3 - Assign resolution correctly in RecordFrameMetrics
> 
> Review of attachment 8355698 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsDisplayList.cpp
> @@ +651,5 @@
> >  
> > +  // Only the root layer for a given presShell should pick up
> > +  // the presShell's resolution. Since the root layer is never
> > +  // scrollable, the resolution we assign here is always 1.0.
> > +  metrics.mResolution = ParentLayerToLayerScale(1.0f);
> 
> It might be worth adding an assert here to verify that this is not the root
> layer. What do you think?

Kats and I discussed this on IRC. Asserting that this is not the root layer is difficult, since at this stage the layer is just being set up and has not been hooked into the layer tree yet.

However, a useful assertion is to ensure that the frame is actually scrollable (previously RecordFrameMetrics would sometimes be called for non-scrollable frames as well). Since the root layer is now never scrollable, the frame being scrollable should ensure that this is not the root layer.

	Timothy Nikkel (:tnikkel)
	Comment 37 • 11 years ago

Comment on attachment 8356180 [details] [diff] [review]
Part 2.5 - RecordFrameMetrics can now assume aScrollFrame != NULL

I think we can also assume that both aScrollFrame and aScrolledFrame are non-null. Would also be nice if we could assume scrollableFrame was non-null too (maybe we can, I didn't check).

	Botond Ballo [:botond] Assignee
	Comment 38 • 11 years ago

(In reply to Timothy Nikkel (:tn) from comment #37)
> Comment on attachment 8356180 [details] [diff] [review]
> Part 2.5 - RecordFrameMetrics can now assume aScrollFrame != NULL
> 
> I think we can also assume that both aScrollFrame and aScrolledFrame are
> non-null.

We have already been assuming aScrolledFrame (formerly aForFrame) is not null. For example, on the first line of RecordFrameMetrics we call PresContext() on it without checking that it is not null.

	Botond Ballo [:botond] Assignee
	Comment 39 • 11 years ago

These patches seem to cause a regression where e.g. in http://people.mozilla.org/~kgupta/gridframe.html, pinching on the iframe zooms in just the iframe rather than the entire page. I will investigate.

	Timothy Nikkel (:tnikkel)
	Comment 40 • 11 years ago

Random guess, does the iframe get it's own layer? Try making this always true http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSubDocumentFrame.cpp#387 maybe?

	Botond Ballo [:botond] Assignee
	Comment 41 • 11 years ago

(In reply to Botond Ballo [:botond] from comment #39)
> These patches seem to cause a regression where e.g. in
> http://people.mozilla.org/~kgupta/gridframe.html, pinching on the iframe
> zooms in just the iframe rather than the entire page. I will investigate.

The problem was that we've gone from the root layer being a scrollable layer to it being a scroll info layer. Scroll info layers never have children, so there is no ancestor-descendant relationship between the root layer's APZC and the iframe's APZC. This causes problems (specifically, when routing the pinch to the "root" APZC the iframe is misidentified as the root because it doesn't have an ancestor).

The fix is to force the root scroll frame to build a scrollable layer rather than a scroll info layer. This corresponds to the behaviour before the patches, where PaintForFrame would build a scrollable layer.

While making this change and updating the comment here [1] accordingly, I realized that it was fairly out of date, so I rewrote it. Kats, Timothy, I'm asking for feedback specifically on the new comment.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2394

	Botond Ballo [:botond] Assignee
	Comment 42 • 11 years ago

These patches are also causing a regression on http://people.mozilla.org/~nhirata/html_tp/: the page is loaded at twice the expected zoom, and it's blurry, as if it were painted at the resolution required for the correct zoom, and then additionally zoomed by the compositor. Panning the page causes some strange jumps in the zoom, after which the blurriness goes away but the initial incorrect zoom remains.

I initially thought this was because TabChild was setting FrameMetrics::mResolution to something other than 1.0 here [1] and here [2], when it's now supposed to be 1.0, but changing those values to assign 1.0 (and adjusting the call to set the pres shell resolution accordingly) causes much worse problems, including breaking zooming on basic pages like http://people.mozilla.org/~bballo/grid.html. I will have to investigate this more deeply.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#359
[2] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#618

	Timothy Nikkel (:tnikkel)
	Comment 43 • 11 years ago

I filed bug 959847 to make non-display roots behave the same way as display roots (aka "make metro like b2g"). This goes in the opposite direction of the approach we have here by adding RecordFrameMetrics calls for subdocuments.

	Botond Ballo [:botond] Assignee
	Comment 44 • 11 years ago

Comment on attachment 8356798 [details] [diff] [review]
Part 2 - Unify RecordFrameMetrics calls

Unflagging for review as Timothy and I are now considering a different approach for solving this problem in the short term (see bug 959847).

	Botond Ballo [:botond] Assignee
	Comment 45 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #44)
> Unflagging for review as Timothy and I are now considering a different
> approach for solving this problem in the short term (see bug 959847).

Our alternative approach has landed and we are happy with it for now.

I think that if we later revisit our approach in bug 959847 and decide to do something different, it will be something different from this bug as well, so I don't think there is point in keeping this bug open.