Closed
Bug 886298
Opened 11 years ago
Closed 11 years ago
Propagate new Pixel and Scale types into fixed-position layer code
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: kats, Assigned: cwiiis)
References
Details
Attachments
(3 files, 1 obsolete file)
9.54 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
24.47 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
3.65 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
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•11 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•11 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•11 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 | ||
Comment 5•11 years ago
|
||
A long time in coming due to all the rewriting from bug 876542, but here's part 1.
Attachment #774600 -
Flags: review?(bugmail.mozilla)
Reporter | ||
Comment 6•11 years ago
|
||
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•11 years ago
|
||
And here's the revision.
Attachment #774600 -
Attachment is obsolete: true
Attachment #774708 -
Flags: review?(bugmail.mozilla)
Reporter | ||
Comment 8•11 years ago
|
||
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•11 years ago
|
||
And here's the same thing for margins.
Attachment #775661 -
Flags: review?(bugmail.mozilla)
Reporter | ||
Comment 10•11 years ago
|
||
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•11 years ago
|
||
Er yeah, whoops, forgot to attach this one...
Attachment #775700 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 12•11 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)
Reporter | ||
Comment 13•11 years ago
|
||
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+
Reporter | ||
Updated•11 years ago
|
Attachment #775661 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 14•11 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
Comment 15•11 years ago
|
||
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
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in
before you can comment on or make changes to this bug.
Description
•