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

RESOLVED FIXED in mozilla25

Status

()

Core
Graphics: Layers
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kats, Assigned: cwiiis)

Tracking

24 Branch
mozilla25
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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).
(Assignee)

Comment 1

4 years ago
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.
Flags: needinfo?(chrislord.net)
(Assignee)

Comment 2

4 years ago
I can take this :)
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
Flags: needinfo?(chrislord.net)
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?
(Assignee)

Comment 4

4 years ago
(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.
(Assignee)

Updated

4 years ago
Blocks: 876542
(Assignee)

Comment 5

4 years ago
Created attachment 774600 [details] [diff] [review]
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.
Attachment #774600 - Flags: review?(bugmail.mozilla)
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.
Attachment #774600 - Flags: review?(bugmail.mozilla) → review-
(Assignee)

Comment 7

4 years ago
Created attachment 774708 [details] [diff] [review]
Part 1 - Convert layer fixed-position anchor to LayerPixels

And here's the revision.
Attachment #774600 - Attachment is obsolete: true
Attachment #774708 - Flags: review?(bugmail.mozilla)
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.
Attachment #774708 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 9

4 years ago
Created attachment 775661 [details] [diff] [review]
Part 2 - Convert layer fixed-position margins to LayerPixels

And here's the same thing for margins.
Attachment #775661 - Flags: review?(bugmail.mozilla)
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.
Attachment #775661 - Flags: review?(bugmail.mozilla) → review-
(Assignee)

Comment 11

4 years ago
Created attachment 775700 [details] [diff] [review]
Part 1.5 - Add margin unit types

Er yeah, whoops, forgot to attach this one...
Attachment #775700 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 12

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

Re-requesting in light of part 1.5.
Attachment #775661 - Flags: review- → review?(bugmail.mozilla)
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.
Attachment #775700 - Flags: review?(bugmail.mozilla) → review+
Attachment #775661 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Updated

4 years ago
No longer blocks: 876542
Depends on: 876542
(Assignee)

Comment 14

4 years ago
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
https://hg.mozilla.org/mozilla-central/rev/5b969d135bfa
https://hg.mozilla.org/mozilla-central/rev/964ad721c308
https://hg.mozilla.org/mozilla-central/rev/c6f2fd048e62
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 903473
You need to log in before you can comment on or make changes to this bug.