Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Description • 11 years ago

I've been slowly converting code to use the new templated gfx types introduced in bug 865735. While doing so it has become apparent that we could benefit from typing the conversion scale factors too. As Anthony pointed out at https://bugzilla.mozilla.org/show_bug.cgi?id=879004#c20, the resolution parameters passed to the conversion functions in LayerPixel et al would benefit from being incompatible with arbitrary floats or doubles. I'm not entirely sure of the best way to make this happen but I'm filing this bug to track this change and to think about it.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 1 • 11 years ago

I think I made a mistake when I added type info to mCompositionBounds. It's actually in ScreenPixel units, not LayerPixel units.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 2 • 11 years ago

This is my WIP for adding stronger type conversion between pixel units. I think using this ends up being pretty clunky, ideally we should be able to overload operators on some of these structs so that we can use * and / as one would expect to get back the right things.

Also note that this patch doesn't currently compile; still working on it but there's enough there to get an idea for feedback.

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)
	Comment 3 • 11 years ago

Comment on attachment 761104 [details] [diff] [review]
Part 2 (WIP) - Add a ScaleFactor class to convert between pixel units

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

::: gfx/2d/ScaleFactor.h
@@ +48,5 @@
> +  }
> +
> +  bool operator==(const ScaleFactor& aScale) const {
> +    return x == aScale.x && y == aScale.y;
> +  }

Seems odd to have equality but no copy constructor.

@@ +51,5 @@
> +    return x == aScale.x && y == aScale.y;
> +  }
> +
> +  bool operator!=(const ScaleFactor& aScale) const {
> +    return x != aScale.x || y != aScale.y;

return !(*this == aScale);

::: gfx/layers/client/ClientLayerManager.cpp
@@ +395,5 @@
>      // This is derived from the code in
>      // gfx/layers/ipc/CompositorParent.cpp::TransformShadowTree.
>      const gfx3DMatrix& rootTransform = GetRoot()->GetTransform();
> +    LayerToCSSScale paintScale = LayerToCSSScale(rootTransform.GetXScale(),
> +                                                 rootTransform.GetYScale());

LayerToCSSScale paintScale(rootTransform.GetXScale(), rootTransform.GetYScale());

::: layout/base/Units.h
@@ +44,5 @@
> +typedef gfx::ScaleFactor<LayerPixel, CSSPixel> LayerToCSSScale;
> +typedef gfx::ScaleFactor<CSSPixel, ScreenPixel> CSSToScreenScale;
> +typedef gfx::ScaleFactor<ScreenPixel, CSSPixel> ScreenToCSSScale;
> +typedef gfx::ScaleFactor<LayerPixel, ScreenPixel> LayerToScreenScale;
> +typedef gfx::ScaleFactor<ScreenPixel, LayerPixel> ScreenToLayerScale;

I'm not completely sold on these typedefs. For example if we just did s/Typed/T/ and s/Point/P/ then we could do away with them. ScaleFactor could just be called Scale.

	Benoit Girard (:BenWa)
	Comment 4 • 11 years ago

Comment on attachment 761104 [details] [diff] [review]
Part 2 (WIP) - Add a ScaleFactor class to convert between pixel units

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

::: gfx/layers/client/ClientLayerManager.cpp
@@ +394,5 @@
>  
>      // This is derived from the code in
>      // gfx/layers/ipc/CompositorParent.cpp::TransformShadowTree.
>      const gfx3DMatrix& rootTransform = GetRoot()->GetTransform();
> +    LayerToCSSScale paintScale = LayerToCSSScale(rootTransform.GetXScale(),

Is this really typesafe? Here paintScale is LayerToCSSScale and FromCSSRect takes a CSSToLayerScale.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 9 • 11 years ago

This is just to make the next few patches easier to read. And to keep Units.h a bit cleaner.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 10 • 11 years ago

Commit message needs updating on this, I forgot to do that.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 15 • 11 years ago

(In reply to Benoit Girard (:BenWa) from comment #4)
> 
> Is this really typesafe? Here paintScale is LayerToCSSScale and FromCSSRect
> takes a CSSToLayerScale.

I overloaded the () operator so that the compiler could call Invert() on it make it fit the requirements. I had hoped it would make the code simpler but I think it actually adds confusion. The new patches don't use this sneaky stuff (although I think I left in the overloaded operator in part 7, I can take that out).

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 16 • 11 years ago

Missed a couple of files in this patch.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 17 • 11 years ago

Try build at https://tbpl.mozilla.org/?tree=Try&rev=0e5914369c31 - hopefully I didn't screw up the merge with Ms2ger's changes to Units.h.

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)
	Comment 18 • 11 years ago

Comment on attachment 761723 [details] [diff] [review]
Part 10 - Remove ToCSSIntRectRoundIn and convert APZC::CalculateResolution

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

::: gfx/2d/Rect.h
@@ +109,5 @@
>                               NS_lround(aRect.height));
>  }
>  
> +template<class units>
> +IntRectTyped<units> RoundIn(const RectTyped<units>& aRect)

If we name the function RoundedIn then it gives a hint that it's not the dreaded state mutation variant.

::: gfx/2d/ScaleFactor.h
@@ +25,5 @@
>    ScaleFactor<dst, src> Invert() {
>      return ScaleFactor<dst, src>(1 / x, 1 / y);
>    }
>  
> +  ScaleFactor<src, dst> XScale() const {

Suggestion: XScale isn't an entirely obvious name but I can't think of a better one. It may be better not to have such a function and to explicitly use ScaleFactor(other.x, other.y). Having said that I don't know if this operation is actually necessary.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +749,5 @@
>      MonitorAutoLock monitor(mMonitor);
>  
>      // We want to inversely scale it because when you're zoomed further in, a
>      // larger swipe should move you a shorter distance.
> +    ScreenToCSSScale inverseResolution = CalculateResolution(mFrameMetrics).Invert();

This function would be better named Inverse() because it is returning the inverse.

	Benoit Girard (:BenWa)
	Comment 19 • 11 years ago

Comment on attachment 761686 [details] [diff] [review]
Part 2 - Convert another gfx::Point to a ScreenPoint

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

This being a ScreenPoint isn't going to work with sub-FrameMetrics and sub-APZC. It's really not meant to be a LayerPoint? I guess we will need to update this code.

	Benoit Girard (:BenWa)
	Comment 20 • 11 years ago

Comment on attachment 761694 [details] [diff] [review]
Part 5 - Units.h cleanup (cosmetic)

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

::: layout/base/Units.h
@@ +12,5 @@
>  
>  namespace mozilla {
>  
> +// Forward declarations so we can declare the typedefs
> +// and use them in the actual implementations for readbility.

Opinion: I'm not sure this comment adds value.

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)
	Comment 21 • 11 years ago

Comment on attachment 761697 [details] [diff] [review]
Part 7 - Introduce ScaleFactor

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

::: gfx/2d/ScaleFactor.h
@@ +21,5 @@
> +  explicit ScaleFactor(float aScale) : x(aScale), y(aScale) {}
> +  explicit ScaleFactor(float aX, float aY) : x(aX), y(aY) {}
> +  explicit ScaleFactor(gfxSize aScale) : x(aScale.width), y(aScale.height) {}
> +
> +  ScaleFactor<dst, src> Invert() {

Prefer Inverse().

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1191,2 @@
>    *aNewTransform = ViewTransform(-translation, localScale);
> +  aScrollOffset = (scrollOffset * localScale);

aScrollOffset = scrollOffset * localScale;

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 22 • 11 years ago

Comment on attachment 761697 [details] [diff] [review]
Part 7 - Introduce ScaleFactor

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

In a lot of places --- probably most places, overall --- x and y scales are always the same and we only want to store one of them. So I think I would rather have ScaleFactor be a single value and have ScaleFactorXY (or some better name) for the two-axis version.

::: gfx/2d/ScaleFactor.h
@@ +11,5 @@
> +namespace mozilla {
> +namespace gfx {
> +
> +template<class src, class dst>
> +struct ScaleFactor {

This needs documentation.

@@ +26,5 @@
> +    return ScaleFactor<dst, src>(1 / x, 1 / y);
> +  }
> +
> +  operator ScaleFactor<dst, src>() const {
> +    return Invert();

This is freaky. Why have it?

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 23 • 11 years ago

Comment on attachment 761687 [details] [diff] [review]
Part 3 - Convert some things in APZC::SampleContentTransformForFrame

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1154,5 @@
>      localScale = CalculateResolution(mFrameMetrics);
>  
>      if (frame.IsScrollable()) {
> +      metricsScrollOffset = LayerPoint::FromUnknownPoint(
> +        frame.GetScrollOffsetInLayerPixels());

Why not make it return a LayerPoint now you're changing the return value anyway?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 24 • 11 years ago

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #18)
> > +template<class units>
> > +IntRectTyped<units> RoundIn(const RectTyped<units>& aRect)
> 
> If we name the function RoundedIn then it gives a hint that it's not the
> dreaded state mutation variant.

Good call. I'll do that.

> > +  ScaleFactor<src, dst> XScale() const {
> 
> Suggestion: XScale isn't an entirely obvious name but I can't think of a
> better one. It may be better not to have such a function and to explicitly
> use ScaleFactor(other.x, other.y). Having said that I don't know if this
> operation is actually necessary.

Yeah I'm not happy with XScale either. I'll try doing what roc suggested and having just one scale factor, which will get rid of XScale entirely. Right now the code is a mess with respect to using x and y or just x scale factors (and I don't know if any of the x-and-y cases are actually important) so it might break some stuff.

> ::: gfx/layers/ipc/AsyncPanZoomController.cpp
> > +    ScreenToCSSScale inverseResolution = CalculateResolution(mFrameMetrics).Invert();
> 
> This function would be better named Inverse() because it is returning the
> inverse.

Will do.

(In reply to Benoit Girard (:BenWa) from comment #19)
> This being a ScreenPoint isn't going to work with sub-FrameMetrics and
> sub-APZC. It's really not meant to be a LayerPoint? I guess we will need to
> update this code.

Do you know what specifically conflicts? I'll double-check to see if it's actually a LayerPoint and I screwed up somewhere. It might also be in some other coordinate space entirely. I'm not sure if there's another coordinate space that needs to be introduced because of the cross-process architecture on B2G.

(In reply to Benoit Girard (:BenWa) from comment #20)
> > +// Forward declarations so we can declare the typedefs
> > +// and use them in the actual implementations for readbility.
> 
> Opinion: I'm not sure this comment adds value.

I'll remove it.

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #21)
> > +  ScaleFactor<dst, src> Invert() {
> 
> Prefer Inverse().

Yup, will do.

> ::: gfx/layers/ipc/AsyncPanZoomController.cpp
> > +  aScrollOffset = (scrollOffset * localScale);
> 
> aScrollOffset = scrollOffset * localScale;

Will do.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> 
> In a lot of places --- probably most places, overall --- x and y scales are
> always the same and we only want to store one of them. So I think I would
> rather have ScaleFactor be a single value and have ScaleFactorXY (or some
> better name) for the two-axis version.

Ok, I'll give that a shot.

> ::: gfx/2d/ScaleFactor.h
> > +template<class src, class dst>
> > +struct ScaleFactor {
> 
> This needs documentation.

I'll add some.

> @@ +26,5 @@
> > +  operator ScaleFactor<dst, src>() const {
> > +    return Invert();
> 
> This is freaky. Why have it?

Left over from my original WIP actually. I'll remove it.

(In reply to :Ms2ger from comment #23)
> > +      metricsScrollOffset = LayerPoint::FromUnknownPoint(
> > +        frame.GetScrollOffsetInLayerPixels());
> 
> Why not make it return a LayerPoint now you're changing the return value
> anyway?

I assume you're referring to GetScrollOffsetInLayerPixels return value? I'm not actually sure that it is a LayerPoint. The calculations it does internally looks wrong to me, and I need audit those pieces more carefully before I'm convinced that the "LayerPixels" in the name is the same as the "LayerPixels" we're using elsewhere.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 25 • 11 years ago

Rebased, carrying r+

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 26 • 11 years ago

Updated part 6 so that the function is called RoundedToInt rather than RoundToInt. This makes it consistent with the change Anthony requested. Carrying r+

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 27 • 11 years ago

Updated so that ScaleFactor has only one float instead of two. After discussing with a few people no use cases came up where the scale factor would actually differ in the two axes, so I decided to remove that aspect entirely. It can be put back if it's needed later. Also on a suggestion from cpeterson I added MOZ_ASSERTs to ScaleFactor so that if it is constructed with differing scales on the two axes it will tell us.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 28 • 11 years ago

Rebased, carrying r?BenWa

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 29 • 11 years ago

Rebased, carrying r+

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 30 • 11 years ago

This one is rebased, but because of the accumulated changes from the previous patches it looks quite different. Re-requesting review just to be safe.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 31 • 11 years ago

New try build at https://tbpl.mozilla.org/?tree=Try&rev=0e42ee89b840

	Benoit Girard (:BenWa)
	Comment 32 • 11 years ago

Comment on attachment 762208 [details] [diff] [review]
Part 7 - Introduce ScaleFactor (v2)

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

::: gfx/2d/ScaleFactor.h
@@ +19,5 @@
> + * Note that some parts of the code that pre-date this class used separate
> + * scaling factors for the x and y axes. However, at runtime these values
> + * were always expected to be the same, so this class uses only one scale
> + * factor for both axes. The two constructors that take two-axis scaling
> + * factors check to ensure that this assertion holds.

Good idea taking both param and asserting.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +549,5 @@
>  
>    {
>      MonitorAutoLock monitor(mMonitor);
>  
> +    CSSToScreenScale resolution(CalculateResolution(mFrameMetrics).width);

This would be an excellent place to assert that width/height scale factor is the same by calling the right constructor.

@@ +554,4 @@
>      gfxFloat userZoom = mFrameMetrics.mZoom.width;
>      ScreenPoint focusPoint = aEvent.mFocusPoint;
>  
> +    CSSPoint focusChange = (mLastZoomFocus - focusPoint) / resolution;

Nice use of operator overload

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 33 • 11 years ago

(In reply to Benoit Girard (:BenWa) from comment #32)
> > +    CSSToScreenScale resolution(CalculateResolution(mFrameMetrics).width);
> 
> This would be an excellent place to assert that width/height scale factor is
> the same by calling the right constructor.

Done and landed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/494c2b42bd94
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1cffa2a5298c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f9e66f6ca377
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/89ddf463d04f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a26c17d194c1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6e4751b152a1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6e2cfc657c74
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1e0c4309ec6a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8050296faa27
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c61b755de9

	Ryan VanderMeulen [:RyanVM]
	Comment 34 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/494c2b42bd94
https://hg.mozilla.org/mozilla-central/rev/1cffa2a5298c
https://hg.mozilla.org/mozilla-central/rev/f9e66f6ca377
https://hg.mozilla.org/mozilla-central/rev/89ddf463d04f
https://hg.mozilla.org/mozilla-central/rev/a26c17d194c1
https://hg.mozilla.org/mozilla-central/rev/6e4751b152a1
https://hg.mozilla.org/mozilla-central/rev/6e2cfc657c74
https://hg.mozilla.org/mozilla-central/rev/1e0c4309ec6a
https://hg.mozilla.org/mozilla-central/rev/8050296faa27
https://hg.mozilla.org/mozilla-central/rev/f0c61b755de9