1085569 - Composition bounds calculation should use cumulative resolution in all places

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

Bug 935219 changed the calculation of the composition bounds to use the "parent resolution" instead of the cumulative resolution.

This turned out to be wrong, and bug 1000423 fixed it, but only in RecordFrameMetrics() (since renamed to ComputeFrameMetrics()).

There are two other places where the composition bounds are calculated: CalculateFrameMetricsForDisplayPort(), and CalculateRootCompositionSize(). These should be updated as well.

Comment 1

[Botond Ballo [:botond]]

Attachment: bug1085569.patch

Comment 2

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8508165 [details] [diff] [review]
bug1085569.patch

The composition size for a frame is either the frame size, or its the widget/content viewer bounds. The widget/content viewer bounds are the size of the container that holds the document. So if the local resolution changes the size of the the container doesn't change. So I don't understand this change in the case we derive the composition bounds from the widget/content viewer bounds.

Comment 3

[Botond Ballo [:botond]]

Attachment: bug1085569.patch

(In reply to Timothy Nikkel (:tn) from comment #2)
> The composition size for a frame is either the frame size, or its the
> widget/content viewer bounds. The widget/content viewer bounds are the size
> of the container that holds the document. So if the local resolution changes
> the size of the the container doesn't change. So I don't understand this
> change in the case we derive the composition bounds from the widget/content
> viewer bounds.

Oops, you're right! I didn't realize that ComputeFrameMetrics() also used the parent pres shell's cumulative resolution when using the content viewer bounds.

Comment 4

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8508275 [details] [diff] [review]
bug1085569.patch

>@@ -2758,17 +2758,19 @@ CalculateFrameMetricsForDisplayPort(nsIF
>   metrics.SetZoom(deviceScale * cumulativeResolution * LayerToScreenScale(1));
> 
>   // Only the size of the composition bounds is relevant to the
>   // displayport calculation, not its origin.
>   nsSize compositionSize = nsLayoutUtils::CalculateCompositionSizeForFrame(aScrollFrame);
>   metrics.mCompositionBounds
>       = LayoutDeviceRect::FromAppUnits(nsRect(nsPoint(0, 0), compositionSize),
>                                        presContext->AppUnitsPerDevPixel())
>-      * (cumulativeResolution / resolution);
>+      * cumulativeResolution
>+      * LayerToScreenScale(1.0f)
>+      * ScreenToParentLayerScale(1.0f);

compositionSize here could also be either the frame bounds or the widget/content viewer bounds. So shouldn't whether we include the local resolution in the cummulative resolution depend on whether we are using frame bounds or widget/content viewer bounds?

Comment 5

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #4)
> Comment on attachment 8508275 [details] [diff] [review]
> bug1085569.patch
> 
> >@@ -2758,17 +2758,19 @@ CalculateFrameMetricsForDisplayPort(nsIF
> >   metrics.SetZoom(deviceScale * cumulativeResolution * LayerToScreenScale(1));
> > 
> >   // Only the size of the composition bounds is relevant to the
> >   // displayport calculation, not its origin.
> >   nsSize compositionSize = nsLayoutUtils::CalculateCompositionSizeForFrame(aScrollFrame);
> >   metrics.mCompositionBounds
> >       = LayoutDeviceRect::FromAppUnits(nsRect(nsPoint(0, 0), compositionSize),
> >                                        presContext->AppUnitsPerDevPixel())
> >-      * (cumulativeResolution / resolution);
> >+      * cumulativeResolution
> >+      * LayerToScreenScale(1.0f)
> >+      * ScreenToParentLayerScale(1.0f);
> 
> compositionSize here could also be either the frame bounds or the
> widget/content viewer bounds. So shouldn't whether we include the local
> resolution in the cummulative resolution depend on whether we are using
> frame bounds or widget/content viewer bounds?

CalculateFrameMetricsForDisplayPort() assumes that the FM is being calculated for a subframe [1]. Therefore, we know the frame bounds will be used here.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?rev=11fa2110679d#2769

Comment 6

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #5)
> (In reply to Timothy Nikkel (:tn) from comment #4)
> > Comment on attachment 8508275 [details] [diff] [review]
> > bug1085569.patch
> > 
> > >@@ -2758,17 +2758,19 @@ CalculateFrameMetricsForDisplayPort(nsIF
> > >   metrics.SetZoom(deviceScale * cumulativeResolution * LayerToScreenScale(1));
> > > 
> > >   // Only the size of the composition bounds is relevant to the
> > >   // displayport calculation, not its origin.
> > >   nsSize compositionSize = nsLayoutUtils::CalculateCompositionSizeForFrame(aScrollFrame);
> > >   metrics.mCompositionBounds
> > >       = LayoutDeviceRect::FromAppUnits(nsRect(nsPoint(0, 0), compositionSize),
> > >                                        presContext->AppUnitsPerDevPixel())
> > >-      * (cumulativeResolution / resolution);
> > >+      * cumulativeResolution
> > >+      * LayerToScreenScale(1.0f)
> > >+      * ScreenToParentLayerScale(1.0f);
> > 
> > compositionSize here could also be either the frame bounds or the
> > widget/content viewer bounds. So shouldn't whether we include the local
> > resolution in the cummulative resolution depend on whether we are using
> > frame bounds or widget/content viewer bounds?
> 
> CalculateFrameMetricsForDisplayPort() assumes that the FM is being
> calculated for a subframe [1]. Therefore, we know the frame bounds will be
> used here.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.
> cpp?rev=11fa2110679d#2769

CalculateFrameMetricsForDisplayPort gets called from nsLayoutUtils::PaintFrame, so in that case the frame is not going to be a subframe. Also if we want to do subframe zooming we'll need to use the content viewer bounds for the composition size of the zoomed subframe.

Comment 7

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #6)
> CalculateFrameMetricsForDisplayPort gets called from
> nsLayoutUtils::PaintFrame, so in that case the frame is not going to be a
> subframe. 

It shouldn't - for the root frame, another component (TabChild on B2G) should set a displayport so that GerOrMaybeCreateDisplayport() takes the 'haveDisplayPort = true' path.

> Also if we want to do subframe zooming we'll need to use the
> content viewer bounds for the composition size of the zoomed subframe.

This brings up an interesting question: 

There is only a difference between the cumulative resolution and the "parent resolution" for things that are zoomed. If, for such things, we are using the content viewer bounds, which should be multiplied by the parent resolution, then why do we ever multiply by the cumulative resolution?

Comment 8

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #7)
> (In reply to Timothy Nikkel (:tn) from comment #6)
> > CalculateFrameMetricsForDisplayPort gets called from
> > nsLayoutUtils::PaintFrame, so in that case the frame is not going to be a
> > subframe. 
> 
> It shouldn't - for the root frame, another component (TabChild on B2G)
> should set a displayport so that GerOrMaybeCreateDisplayport() takes the
> 'haveDisplayPort = true' path.

That's a really delicate dependency, and un-obvious connection. I'd rather not have it and just write code that works for all situations.

> > Also if we want to do subframe zooming we'll need to use the
> > content viewer bounds for the composition size of the zoomed subframe.
> 
> This brings up an interesting question: 
> 
> There is only a difference between the cumulative resolution and the "parent
> resolution" for things that are zoomed. If, for such things, we are using
> the content viewer bounds, which should be multiplied by the parent
> resolution, then why do we ever multiply by the cumulative resolution?

I suppose that where we do multiply by cumulative resolution we are effectively multiplying by the parent resolution, it's just that they are the same thing in those cases. (Or at least were the same, before we added css transform resolution into the mix.)

Comment 9

[Botond Ballo [:botond]]

Attachment: bug1085569.patch

Discussed this with Timothy over IRC. Summary:

  - CalculateCompositionSizeForFrame (CCSFF) does something different
    for the RCD-RSF than for other frames.

  - Other callers of CCSFF already special-case the RCD-RSF.

  - The updated patch documents in CCSFF the need to special-case,
    and changes CalculateFrameMetricsForDisplayPort to do so
    as well.

In a subsequent patch, I'd like to clean up the code surrounding the composition bounds/size calculations, but I don't want to block the apz-css-transforms work on that cleanup.

Comment 10

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8509912 [details] [diff] [review]
bug1085569.patch

Thanks.

Comment 11

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bcc51b1a6c07

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bcc51b1a6c07