886298 - Propagate new Pixel and Scale types into fixed-position layer code

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

TransformFixedLayers doesn't make much sense to me. While propagating the new pixel and scale types I ran into the problem where one caller for TransformFixedLayers was passing in a LayerToScreenScale for the "aScaleDiff" and another caller was passing in a LayoutDeviceToScreenScale. I'm not sure what's going on there.

Cwiiis, do you have time to take a look at this code on m-c and fix it up as needed? I'd really like to propagate the new pixel and scale types into that code so that it is clearer. This might also provide a cleaner fix for bug 876542 (I say that based on what roc's patch is doing but without really understanding the code).

Comment 1

[Chris Lord [:cwiiis]]

needinfo'ing myself so I don't forget to answer this (but I can't really answer this right now). ftr, I'd like to.

Comment 2

[Chris Lord [:cwiiis]]

I can take this :)

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Should I try to port 876542 forward to trunk, or let you do this first, or let you fix 876542 on central in this bug?

Comment 4

[Chris Lord [:cwiiis]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> Should I try to port 876542 forward to trunk, or let you do this first, or
> let you fix 876542 on central in this bug?

I'll forward-port on bug 876542 - I'd like to make sure that no behaviour unexpectedly changes in fennec.

Comment 5

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Convert layer fixed-position anchor to LayerPixels

A long time in coming due to all the rewriting from bug 876542, but here's part 1.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 774600 [details] [diff] [review]
Part 1 - Convert layer fixed-position anchor to LayerPixels

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +258,2 @@
>      gfxPoint locallyTransformedAnchor = layerTransform.Transform(anchor);
> +    gfxPoint locallyTransformedOffsetAnchor = layerTransform.Transform(anchor + offset);

Do you need to keep both "offset" and "anchor" as gfxPoints? I'd rather do the addition in the LayerPoint world and only convert back to gfxPoint for the transform. Eventually I want to make the Transform function also be type-aware although I'm not entirely sure how I'll do that.

::: layout/base/nsDisplayList.cpp
@@ +3013,5 @@
>    // This, in conjunction with the container scale, will correspond to the
>    // coordinate-space of the built layer.
>    float factor = presContext->AppUnitsPerDevPixel();
>    nsPoint origin = viewportFrame->GetOffsetToCrossDoc(ReferenceFrame());
> +  LayoutDeviceRect anchorRect(NSAppUnitsToFloatPixels(origin.x, factor) *

anchorRect here is in LayerPixels, not LayoutDevice. origin.x is in app units, NSAppUnitsToFloatPixels(origin.x, factor) returns something in LayoutDevicePixels, and then multiplying it by aContainerParameters.mXScale gives a LayerPixel value.

Ideally what I would like to see here is something like:
LayerRect anchorRect = LayerRect::FromAppUnits(nsRect(origin, containingBlockSize), factor) * LayoutDeviceToLayerScale(aContainerParameters.mXScale, aContainerParmaeters.mYScale);

You may need to add a function to LayoutDevicePixel in layout/base/Units.h to make this work. Also note the ScaleFactor class doesn't actually keep x- and y- scale factors internally so the above line will not actually work, but try to get as close to it as you can.

@@ +3022,5 @@
> +                                aContainerParameters.mXScale,
> +                              NSAppUnitsToFloatPixels(containingBlockSize.height, factor) *
> +                                aContainerParameters.mYScale);
> +
> +  LayerPoint anchor(anchorRect.x, anchorRect.y);

Doing this is bad because it defeats the entire point of the typing. You shouldn't just arbitrarily convert from LayoutDevicePixels to LayerPixels without a comment or explanation or using one of the FromUnknown*/ToUnknown* functions. The values flowing through here are actually correct because anchorRect is really in LayerPixels but even so I would r- based on this line alone.

Comment 7

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Convert layer fixed-position anchor to LayerPixels

And here's the revision.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 774708 [details] [diff] [review]
Part 1 - Convert layer fixed-position anchor to LayerPixels

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +264,2 @@
>      gfxPoint locallyTransformedAnchor = layerTransform.Transform(anchor);
> +    gfxPoint locallyTransformedOffsetAnchor = layerTransform.Transform(offsetAnchor);

I really would like more useful variable names here but I'm not totally sure what would be more appropriate.

Comment 9

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Convert layer fixed-position margins to LayerPixels

And here's the same thing for margins.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 775661 [details] [diff] [review]
Part 2 - Convert layer fixed-position margins to LayerPixels

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

This is missing changes to layout/base/Units.h and gfx/2d/Rect.h. Looks fine otherwise.

Comment 11

[Chris Lord [:cwiiis]]

Attachment: Part 1.5 - Add margin unit types

Er yeah, whoops, forgot to attach this one...

Comment 12

[Chris Lord [:cwiiis]]

Comment on attachment 775661 [details] [diff] [review]
Part 2 - Convert layer fixed-position margins to LayerPixels

Re-requesting in light of part 1.5.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 775700 [details] [diff] [review]
Part 1.5 - Add margin unit types

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

r=me with comment addressed

::: gfx/2d/Rect.h
@@ +22,3 @@
>  
> +    MarginTyped() : Super(0, 0, 0, 0) {}
> +    explicit MarginTyped(const MarginTyped<units>& aMargin) :

This doesn't need to be "explicit", it's just a regular copy constructor.

Comment 14

[Chris Lord [:cwiiis]]

Pushed to inbound (along with bug 876542):

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5b969d135bfa
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/964ad721c308
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c6f2fd048e62

Comment 15

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/5b969d135bfa
https://hg.mozilla.org/mozilla-central/rev/964ad721c308
https://hg.mozilla.org/mozilla-central/rev/c6f2fd048e62