Closed Bug 1085569 Opened 6 years ago Closed 6 years ago

Composition bounds calculation should use cumulative resolution in all places

Categories

(Core :: Panning and Zooming, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: botond, Assigned: botond)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
See Also: → 1000423
Attached patch bug1085569.patch (obsolete) — Splinter Review
Assignee: nobody → botond
Attachment #8508165 - Flags: review?(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.
Attached patch bug1085569.patch (obsolete) — Splinter Review
(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.
Attachment #8508165 - Attachment is obsolete: true
Attachment #8508165 - Flags: review?(tnikkel)
Attachment #8508275 - Flags: review?(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?
(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
(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.
(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?
(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.)
Attached patch bug1085569.patchSplinter Review
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.
Attachment #8508275 - Attachment is obsolete: true
Attachment #8508275 - Flags: review?(tnikkel)
Attachment #8509912 - Flags: review?(tnikkel)
Comment on attachment 8509912 [details] [diff] [review]
bug1085569.patch

Thanks.
Attachment #8509912 - Flags: review?(tnikkel) → review+
https://hg.mozilla.org/mozilla-central/rev/bcc51b1a6c07
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.