We have code that does some stuff to the composition bounds, but it exists in triplicate [1][2][3]. We've wanted to refactor it for a while but it's tricky. [1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?rev=18d118c05f8a#7598 [2] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?rev=18d118c05f8a#7673 [3] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?rev=18d118c05f8a#8239

I originally wrote this for bug 1157579 but it makes more sense over here.

I'm hoping the fact that the last two parameters on the helper are different is just a mistake in the pre-existing code, and we can get rid of them by always using the Right Thing inside the helper.

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef8c3c7047aa has these patches (although part 2 is un-squashed).

If there's more missing stuff let me know.

Comment on attachment 8600289 [details] [diff] [review] Part 1 - Refactor scrollbar subtraction decision-making code Review of attachment 8600289 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.cpp @@ +7589,5 @@ > + return false; > + } > + bool isRootScrollFrame = aScrollFrame == presShell->GetRootScrollFrame(); > + bool isRootContentDocRootScrollFrame = isRootScrollFrame > + && presContext->IsRootContentDocument(); I'm not sure how I feel about recomputing isRCDRSF here when we have it available at all call sites. Perhaps we can pass it in instead? That said, I don't feel strongly about it.

Comment on attachment 8600290 [details] [diff] [review] Part 2 - Extract a helper function Review of attachment 8600290 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.cpp @@ +7579,5 @@ > > static bool > +UpdateCompositionBoundsForRCDRSF(ParentLayerRect& aCompBounds, > + nsPresContext* aPresContext, > + nsRect aFrameBounds, Take the rect by reference-to-const. @@ +7623,5 @@ > + > + LayoutDeviceIntSize contentSize; > + if (nsLayoutUtils::GetContentViewerSize(aPresContext, contentSize)) { > + LayoutDeviceToParentLayerScale scale; > + if (aScaleContentViewerSize && aPresContext->GetParentPresContext()) { Timothy, do you know why we were scaling the content-viewer size in ComputeFrameMetrics but not in CalculateCompositionSizeForFrame?

Comment on attachment 8600291 [details] [diff] [review] Part 3 - Use the helper function more Review of attachment 8600291 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.cpp @@ +7581,5 @@ > UpdateCompositionBoundsForRCDRSF(ParentLayerRect& aCompBounds, > nsPresContext* aPresContext, > nsRect aFrameBounds, > + bool aScaleContentViewerSize, > + gfxSize aTransformToAncestor = gfxSize(1,1)) Please name it 'aTransformToAncestorScale' (it's the scale portion of the transform to the ancestor frame, not the entire transform). @@ +7613,5 @@ > #ifdef MOZ_WIDGET_ANDROID > ParentLayerRect frameBounds = > LayoutDeviceRect::FromAppUnits(aFrameBounds, aPresContext->AppUnitsPerDevPixel()) > * LayoutDeviceToLayerScale(aPresContext->PresShell()->GetCumulativeResolution()) > + * LayerToParentLayerScale2D(aTransformToAncestor); The transform-to-ancestor scale is part of the LayoutDevice -> Layer scale, not the Layer -> ParentLayer scale. Suggested formulation: LayoutDeviceToLayerScale2D cumulativeResolution( rootPresShell->GetCumulativeResolution() * aTransformToAncestorScale); ParentLayerRect frameBounds = LayoutDeviceRect::FromAppUnits(aFrameBounds, aPresContext->AppUnitsPerDevPixel()) * cumulativeResolution * LayerToParentLayerScale(1.0); @@ +7716,2 @@ > if (nsIFrame* rootFrame = rootPresShell->GetRootFrame()) { > + gfxSize transformToAncestor = nsLayoutUtils::GetTransformToAncestorScale(rootFrame); transformToAncestorScale @@ +7717,5 @@ > + gfxSize transformToAncestor = nsLayoutUtils::GetTransformToAncestorScale(rootFrame); > + ParentLayerRect compBounds; > + if (UpdateCompositionBoundsForRCDRSF(compBounds, rootPresContext, > + rootFrame->GetRect(), true, transformToAncestor)) { > + rootCompositionSize = ScreenSize(compBounds.width, compBounds.height); Would prefer 'ViewAs<ScreenSize>(compBounds.Size(), ScreenIsParentLayerForRoot)'. @@ +7726,5 @@ > + int32_t rootAUPerDevPixel = rootPresContext->AppUnitsPerDevPixel(); > + LayerSize frameSize = > + (LayoutDeviceRect::FromAppUnits(rootFrame->GetRect(), rootAUPerDevPixel) > + * cumulativeResolution).Size(); > + rootCompositionSize = frameSize * LayerToScreenScale(1.0f); Should we have this as a general fallback in UpdateCompositionBoundsForRCFRSF, rather than just here?

Comment on attachment 8600292 [details] [diff] [review] Part 4 - Put in a missing scale factor Review of attachment 8600292 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.cpp @@ +7671,5 @@ > > bool isRootContentDocRootScrollFrame = presContext->IsRootContentDocument() > && aFrame == presShell->GetRootScrollFrame(); > if (isRootContentDocRootScrollFrame) { > + gfxSize transformToAncestor = nsLayoutUtils::GetTransformToAncestorScale(aFrame); transformToAncestorScale @@ +7676,3 @@ > ParentLayerRect compBounds; > + if (UpdateCompositionBoundsForRCDRSF(compBounds, presContext, aFrame->GetRect(), > + false, transformToAncestor)) { Please remove the default argument for aTransformToAncestorScale in UpdateCompositionBoundsForRCDRSF now that everyone passes it.

(In reply to Botond Ballo [:botond] from comment #12) > Would prefer 'ViewAs<ScreenSize>(compBounds.Size(), > ScreenIsParentLayerForRoot)'. Er, ViewAs<ScreenPixel>(...).

(In reply to Botond Ballo [:botond] from comment #10) > Comment on attachment 8600289 [details] [diff] [review] > Part 1 - Refactor scrollbar subtraction decision-making code > > I'm not sure how I feel about recomputing isRCDRSF here when we have it > available at all call sites. Perhaps we can pass it in instead? > > That said, I don't feel strongly about it. I guess it's left over from when I wanted the function to be as standalone as possible so I could call it from HTMLScrollFrame::TryLayout over in bug 1157579, but honestly I still like that it's standalone. It should be pretty cheap to compute isRCDRSF and doing the computation reduces the chances that we'll pass in conflicting information (i.e. a frame that is RCDRSF with a false bool, or something). (In reply to Botond Ballo [:botond] from comment #11) > Comment on attachment 8600290 [details] [diff] [review] > Part 2 - Extract a helper function > > Take the rect by reference-to-const. Done. (In reply to Botond Ballo [:botond] from comment #12) > Comment on attachment 8600291 [details] [diff] [review] > Part 3 - Use the helper function more > > Please name it 'aTransformToAncestorScale' (it's the scale portion of the > transform to the ancestor frame, not the entire transform). Done. > The transform-to-ancestor scale is part of the LayoutDevice -> Layer scale, > not the Layer -> ParentLayer scale. > > Suggested formulation: > > LayoutDeviceToLayerScale2D cumulativeResolution( > rootPresShell->GetCumulativeResolution() > * aTransformToAncestorScale); > ParentLayerRect frameBounds = > LayoutDeviceRect::FromAppUnits(aFrameBounds, > aPresContext->AppUnitsPerDevPixel()) > * cumulativeResolution > * LayerToParentLayerScale(1.0); Updated the code to look like this. > > + gfxSize transformToAncestor = nsLayoutUtils::GetTransformToAncestorScale(rootFrame); > > transformToAncestorScale Done > > + rootCompositionSize = ScreenSize(compBounds.width, compBounds.height); > > Would prefer 'ViewAs<ScreenSize>(compBounds.Size(), > ScreenIsParentLayerForRoot)'. Done > @@ +7726,5 @@ > > + int32_t rootAUPerDevPixel = rootPresContext->AppUnitsPerDevPixel(); > > + LayerSize frameSize = > > + (LayoutDeviceRect::FromAppUnits(rootFrame->GetRect(), rootAUPerDevPixel) > > + * cumulativeResolution).Size(); > > + rootCompositionSize = frameSize * LayerToScreenScale(1.0f); > > Should we have this as a general fallback in > UpdateCompositionBoundsForRCFRSF, rather than just here? The structure of the other two callsites of UpdateCompositionBoundsForRCDRSF is different. Those two look like this: compositionbounds = ... if (UpdateCompBounds(...)) { compBounds = ... } whereas this one is like: ScreenSize compositionBounds; if (...) { if (UpdateCompBounds(...)) { compBounds = ... } else { compBounds = ... } } else { compBounds = ... } So even if I tried to restructure this one to look like the other two it came out messy because of the nested if condition. That said I'm not opposed to cleaning this up further but perhaps as a follow-up bug. (In reply to Botond Ballo [:botond] from comment #13) > > if (isRootContentDocRootScrollFrame) { > > + gfxSize transformToAncestor = nsLayoutUtils::GetTransformToAncestorScale(aFrame); > > transformToAncestorScale Done > > Please remove the default argument for aTransformToAncestorScale in > UpdateCompositionBoundsForRCDRSF now that everyone passes it. As discussed on IRC, I had to make some changes in part 2 to fix an error (my change caused the ComputeFrameMetrics function to use presShell->GetCumulativeResolution() instead of metrics.GetCumulativeResolution(), which are different). So I moved the introduction of the aTransformToAncestorScale parameter into part 2 and was able to remove the default argument in part 4.

Here are the updated patches.

Comment on attachment 8600445 [details] [diff] [review] Part 2 - Extract a helper function (v2) > static bool >+UpdateCompositionBoundsForRCDRSF(ParentLayerRect& aCompBounds, >+ nsPresContext* aPresContext, >+ const nsRect& aFrameBounds, >+ bool aScaleContentViewerSize, >+ gfxSize aTransformToAncestorScale = gfxSize(1, 1)) Instead of just passing aTransformToAncestorScale and then multiplying it by the cumulative resolution of the presshell why not just pass a LayoutDeviceToLayerScale2D that has the exact scale we want. The main reason I suggest is that in nsLayoutUtils::ComputeFrameMetrics we use metrics.GetCumulativeResolution() which contains the presshell cumulative resolution, the transform to ancestor scale, but also the "extra resolution" from the layer aContainerParameters scale. That way we include the extra resolution (and match the calculation as it was before).

Comment on attachment 8600445 [details] [diff] [review] Part 2 - Extract a helper function (v2) >@@ -7605,60 +7667,25 @@ nsLayoutUtils::CalculateCompositionSizeForFrame(nsIFrame* aFrame) > // scroll port. The scroll port excludes the frame borders and the scroll > // bars, which we don't want to be part of the composition bounds. > nsIScrollableFrame* scrollableFrame = aFrame->GetScrollTargetFrame(); > nsSize size = scrollableFrame ? scrollableFrame->GetScrollPortRect().Size() : aFrame->GetSize(); > > nsPresContext* presContext = aFrame->PresContext(); > nsIPresShell* presShell = presContext->PresShell(); > >- // See the comments in the code that calculates the root >- // composition bounds in ComputeFrameMetrics. >- // TODO: Reuse that code here. > bool isRootContentDocRootScrollFrame = presContext->IsRootContentDocument() > && aFrame == presShell->GetRootScrollFrame(); > if (isRootContentDocRootScrollFrame) { >- if (nsIFrame* rootFrame = presShell->GetRootFrame()) { >-#ifdef MOZ_WIDGET_ANDROID >- nsIWidget* widget = rootFrame->GetNearestWidget(); >-#else >- nsView* view = rootFrame->GetView(); >- nsIWidget* widget = view ? view->GetWidget() : nullptr; >-#endif >+ ParentLayerRect compBounds; >+ // TODO: The UpdateCompositionBoundsForRCDRSF below doesn't take into >+ // account the mTransformScale as part of the LayerToParentLayerScale. >+ if (UpdateCompositionBoundsForRCDRSF(compBounds, presContext, aFrame->GetRect(), false)) { Nit, we should pass a rect based on the variable |size| from above so that it is the scrollport size, and not the rect size.

Comment on attachment 8600446 [details] [diff] [review] Part 3 - Use the helper function more (v2) Looks good, with the changes from part 2 propagated (the transform to ancestor scale change).

(In reply to Timothy Nikkel (:tn) from comment #20) > Instead of just passing aTransformToAncestorScale and then multiplying it by > the cumulative resolution of the presshell why not just pass a > LayoutDeviceToLayerScale2D that has the exact scale we want. +1 I meant to suggest this in my review but forgot.

(In reply to Timothy Nikkel (:tn) from comment #20) > >+ gfxSize aTransformToAncestorScale = gfxSize(1, 1)) > > Instead of just passing aTransformToAncestorScale and then multiplying it by > the cumulative resolution of the presshell why not just pass a > LayoutDeviceToLayerScale2D that has the exact scale we want. The main reason > I suggest is that in nsLayoutUtils::ComputeFrameMetrics we use > metrics.GetCumulativeResolution() which contains the presshell cumulative > resolution, the transform to ancestor scale, but also the "extra resolution" > from the layer aContainerParameters scale. That way we include the extra > resolution (and match the calculation as it was before). Good point, I made this change in the new part 2/3/4. (In reply to Timothy Nikkel (:tn) from comment #21) > Nit, we should pass a rect based on the variable |size| from above so that > it is the scrollport size, and not the rect size. Since this is a functional change from the way it was before, I broke it out into part 5. If it causes regressions it will be easier to bisect.

Rebased, carrying r+

Rebased, carrying r+

This addresses comment 21

Comment on attachment 8602733 [details] [diff] [review] Part 2 - Extract a helper function (v3) Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffeb7ddc393f https://hg.mozilla.org/integration/mozilla-inbound/rev/5ff0763a09c4 https://hg.mozilla.org/integration/mozilla-inbound/rev/9dfb28c44400

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c9790f3db34 https://hg.mozilla.org/integration/mozilla-inbound/rev/03152183293a

For the record I landed the first three parts separately from the last two for easier bisection in case of problems but I foiled my own plan by not making sure the first three parts actually built independently. The original versions did, but after addressing the review comments I introduced a busted LayoutDeviceToLayerScale2D(float) call in part 2 which gets removed in part 4. Ah well.

https://hg.mozilla.org/mozilla-central/rev/ffeb7ddc393f https://hg.mozilla.org/mozilla-central/rev/5ff0763a09c4 https://hg.mozilla.org/mozilla-central/rev/9dfb28c44400 https://hg.mozilla.org/mozilla-central/rev/0c9790f3db34 https://hg.mozilla.org/mozilla-central/rev/03152183293a