865735 - Disambiguate between Point instances in different coordinate systems

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

There are many gfxPoint, gfx::Point, nsIntPoint etc. classes floating around, often containing points in hard-to-figure-out coordinate systems. Anthony suggested templating the Point classes so that we would have type-safety and defined conversions between these instances, and use the right points in the right places, e.g. Point<Screen> can't be used instead of Point<Widget> or Point<Layer> or Point<Document> or Point<DisplayPort> or whatever. There's a lot of coordinate systems to deal with.

Hopefully we can do this incrementally so that new code which is typed can coexist with old code that isn't and then gradually convert all the code to be typed. I think the first step would be to identify all the different coordinate systems being used and come up with appropriate names for all of them, so that going forward we can use a consistent naming scheme for the new code we add.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Indeed.  We should have that first step published outside of the code as well (wiki?) as a starting point when someone is first starting in this area.

Comment 2

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

In general I find wiki documentation gets out of date pretty quickly. I'd rather document it in the .h files that define these templates.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

I support this initiative.

Comment 4

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

Here is an incomplete list of coordinate systems I have found in my Fennec/B2G work. Some of these are described at https://wiki.mozilla.org/Fennec/NativeUI/Viewport which is the result of my previous battles with coordinate systems, and is probably out of date now.

Pick any of:
* CSS pixels
* "Zoomed" CSS pixels (this is desktop zoom, not OMTC zoom)
* "Layers" pixels (this is "zoomed" pixels multiplied by the "resolution")
* "Device" or "screen" pixels (this changes asynchronously in OMTC but in steady-state should be the same as the layers pixels)

And create a coordinate system with the origin at any of:
* Gecko scroll position
* OMTC scroll position (should be the same as gecko scroll position in steady state)
* Current document
* Displayport (the area that gecko paints)
* Widget
* Screen
* Child process? (Still figuring out what the deal with B2G is)

Also there is the CSS viewport, which already has some sort of definition because authors can control it by meta tags but is also affected by the display-dpi scale ratio adjustment.

And don't forget the scrollPositionClampingScrollPortSize, in both gecko and OMTC flavours. I *think* the OMTC flavour of that is what we use for positioning fixed-position content but I'm not sure.

Comment 5

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

Bug 865804 will add at least one more level of zoom, I think.

Comment 6

[Timothy Nikkel (:tnikkel)]

There is at least one more type of pixel now (thanks to retina MacBooks): we have both display pixels and device pixels.

Comment 7

[Matt Brubeck (:mbrubeck)]

Would it be worth having two template parameters, one for coordinate system and one for units?  Then we could differentiate between Point<Screen,DevicePixel> and Point<Screen,DisplayPixel> or Point<CssViewport,Px> and Point<CssViewport,Appunit>.

Comment 8

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

Yeah I think that makes sense.

Comment 9

[Benoit Girard (:BenWa)]

(In reply to Matt Brubeck (:mbrubeck) from comment #7)
> Would it be worth having two template parameters, one for coordinate system
> and one for units?  Then we could differentiate between
> Point<Screen,DevicePixel> and Point<Screen,DisplayPixel> or
> Point<CssViewport,Px> and Point<CssViewport,Appunit>.

I don't think this is really useful since AppUnits are typically only in the CssViewport and display pixel are always for the screen. I don't want to see an explosion of template generate code so let's keep the permutations as low as possible.

Comment 10

[Benoit Girard (:BenWa)]

Attachment: Type Gfx Points/Rects/Sizes

Want to do a first pass review Kats before I flag others to make sure it captures what you had in mind?

Comment 11

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

Comment on attachment 743292 [details] [diff] [review]
Type Gfx Points/Rects/Sizes

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

Overall I like this approach. Some comments below.

::: gfx/2d/Point.h
@@ +13,5 @@
>  namespace mozilla {
>  namespace gfx {
>  
> +struct UnknownUnits {};
> +struct DefaultCoordinate {};

I assume you're going to #include Units.h instead of declaring these here.

@@ +18,2 @@
>  
> +template<typename units = UnknownUnits, typename coordinateSpace = DefaultCoordinate>

I think I'd rather refer to "origins" rather than "coordinate spaces". So this would be | typename origin = UnknownOrigin |, and we'd have things like | struct ScreenOrigin {} | and so on. I don't care enough about this to fight for it though.

::: layout/base/Units.h
@@ +17,5 @@
> +struct CSSPixel {};
> +
> +struct LayerPixel {};
> +
> +struct DevicePixel {};

For consistency make these all plural (CSSPixels, etc.)

Comment 12

[Benoit Girard (:BenWa)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> Comment on attachment 743292 [details] [diff] [review]
> Type Gfx Points/Rects/Sizes
> 
> Review of attachment 743292 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall I like this approach. Some comments below.
> 
> ::: gfx/2d/Point.h
> @@ +13,5 @@
> >  namespace mozilla {
> >  namespace gfx {
> >  
> > +struct UnknownUnits {};
> > +struct DefaultCoordinate {};
> 
> I assume you're going to #include Units.h instead of declaring these here.

No. We can't because moz2d can't include anything from layout (and shouldn't since it's shared by servo) and moz2d shouldn't know about our units and origins. I figured this would count as a forward declare but looking again it wont because the {} make it a non forward declaration.

> 
> @@ +18,2 @@
> >  
> > +template<typename units = UnknownUnits, typename coordinateSpace = DefaultCoordinate>
> 
> I think I'd rather refer to "origins" rather than "coordinate spaces". So
> this would be | typename origin = UnknownOrigin |, and we'd have things like
> | struct ScreenOrigin {} | and so on. I don't care enough about this to
> fight for it though.
> 

Neither do I. Origin it is.

Comment 13

[Benoit Girard (:BenWa)]

(In reply to Benoit Girard (:BenWa) from comment #12)
> I figured this would count as a forward declare but looking again
> it wont because the {} make it a non forward declaration.

Err actually it is fine. In any case these should be considered internal implementation details for the typedef provided.

Comment 14

[Benoit Girard (:BenWa)]

Attachment: Type Gfx Points/Rects/Sizes

Looping in Bas for moz2d changes and roc for having this header live in layout.

Comment 15

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

Comment on attachment 743725 [details] [diff] [review]
Type Gfx Points/Rects/Sizes

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

::: layout/base/Units.h
@@ +17,5 @@
> +struct CSSPixels {};
> +
> +struct LayerPixels {};
> +
> +struct DevicePixels {};

We should also add comments here indicating what each of these are.
CSSPixels - The pixels that content authors use to specify sizes in.
LayerPixels - The pixels that result after layout draws CSS pixels at the requested resolution. If the resolution is 2.0, each CSS pixel maps to two Layer pixels.
DevicePixels - The hardware pixels being rendered to.

Maybe GLPixels would be a better name for DevicePixels? I don't know how DisplayPixels (that tn mentioned above) come into play.

Comment 16

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

Comment on attachment 743725 [details] [diff] [review]
Type Gfx Points/Rects/Sizes

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

::: layout/base/Units.h
@@ +8,5 @@
> +
> +namespace mozilla {
> +
> +// Used for all existing untyped instances for backwards compability
> +struct UnknownUnits {};

As discussed, this UnknownUnits needs to be removed from this file; the one in Point.h is sufficient.

Comment 17

[Chris Peterson [:cpeterson]]

Should the Pixels classes include conversion functions to or from other units? Explicit "convert to" functions sound safer than implicit "convert from" constructors.

Comment 18

[Benoit Girard (:BenWa)]

(In reply to Chris Peterson (:cpeterson) from comment #17)
> Should the Pixels classes include conversion functions to or from other
> units? Explicit "convert to" functions sound safer than implicit "convert
> from" constructors.

Yes there will be no implicit conversion. I plan on postponing the conversion function as they are needed.

Comment 19

[Benoit Girard (:BenWa)]

Attachment: Type Gfx Points/Rects/Sizes

Comment 20

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

(In reply to Kartikaya Gupta (email:[e-mail], away May2-May20) from comment #5)
> Bug 865804 will add at least one more level of zoom, I think.

Isn't this just OMTC zoom?

Comment 21

[Bas Schouten (:bas.schouten)]

Comment on attachment 743802 [details] [diff] [review]
Type Gfx Points/Rects/Sizes

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

As best I can tell this does not bring the conversion methods we discussed?

Comment 22

[Benoit Girard (:BenWa)]

(In reply to Bas Schouten (:bas.schouten) from comment #21)
> Comment on attachment 743802 [details] [diff] [review]
> Type Gfx Points/Rects/Sizes
> 
> Review of attachment 743802 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As best I can tell this does not bring the conversion methods we discussed?

It does not. This patch does not allow implicit conversion. We'll need to write explicit conversion functions for this later.

Comment 23

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

I don't think we need AppUnits and CSSPixels to be handled here.

nsPoint is the point type for appunit points, and it's already different from nsIntPoint.

We shouldn't have nsIntPoints containing CSS pixels. Almost everywhere we just convert to appunits and use those. Any cases of nsIntPoints containing CSS pixels should be converted to use nsPoint instead.

Disambiguating the remaining uses of nsIntPoint would be great, however!

I'm not sure whether keeping units and origin orthogonal is worthwhile. I wonder whether it would be better to just mint new types the way we do in nsPoint.h. That approach has the advantage that you can add custom conversion methods to the new point types.

Comment 24

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

The problem with baking the origin into the type is that operators like + get messy. It would be nice to have some kind of type-checking of origins but that seems really hard in general (like in layout, where a frame's position is relative to its parent frame). I think that's probably a bit too ambitious at this point and we can make things a lot better just with a notion of coordinate system that's mainly units.

I think the best way to approach this bug would be to break it down into separate bugs that each introduce a new coordinate system with associated Point/Size/Rect types, and convert some uses of nsIntPoint or gfxPoint to use those types.

Comment 25

[Benoit Girard (:BenWa)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23)
> I don't think we need AppUnits and CSSPixels to be handled here.
> 
> nsPoint is the point type for appunit points, and it's already different
> from nsIntPoint. 

I think it's just useful to be consistent. If we let users specify units for nsIntPoint we should let users specify units for all our point/size/region classes.

> 
> We shouldn't have nsIntPoints containing CSS pixels. Almost everywhere we
> just convert to appunits and use those. Any cases of nsIntPoints containing
> CSS pixels should be converted to use nsPoint instead.

I'm not touching the case where people use the wrong classes (nsIntPoints vs. nsPoints) so this wont make it worse or better. I just want units to be explicit documented in the code and have the compiler catch when you're mixing them without explicitly converting them.

> 
> Disambiguating the remaining uses of nsIntPoint would be great, however!
> 
> I'm not sure whether keeping units and origin orthogonal is worthwhile. I
> wonder whether it would be better to just mint new types the way we do in
> nsPoint.h. That approach has the advantage that you can add custom
> conversion methods to the new point types.

We can still have conversion function but they just wont be defined in the original class. Often times it seems overkill to add a new type because your origin is different and it doesn't get done. nsPoint to me just represent a point class when you need floats and doesn't imply that it's storing app units. It wouldn't seem inappropriate to use it for a texture coordinate for example. Instead if I see nsPoint<AppUnits> then I certainly would use that as a texture coordinate.

Comment 26

[Benoit Girard (:BenWa)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> The problem with baking the origin into the type is that operators like +
> get messy. It would be nice to have some kind of type-checking of origins
> but that seems really hard in general (like in layout, where a frame's
> position is relative to its parent frame). I think that's probably a bit too
> ambitious at this point and we can make things a lot better just with a
> notion of coordinate system that's mainly units.

Yes we've discussed this. For the APZC code it would be nice to verify against the different Layers origin (i.e. the leaf's layer origin or the root' layer origin). I think for now we can declare custom Origins for that such as LeafLayerOrigin and RootLayerOrigin. This is a big improvement without introducing a complex solution.

> 
> I think the best way to approach this bug would be to break it down into
> separate bugs that each introduce a new coordinate system with associated
> Point/Size/Rect types, and convert some uses of nsIntPoint or gfxPoint to
> use those types.

Isn't this what we're doing now with templates?

Comment 27

[Benoit Girard (:BenWa)]

Attachment: Type Gfx Points/Rects/Sizes

Fixed build error with forward declare.

Comment 28

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

(In reply to Benoit Girard (:BenWa) from comment #26)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> > I think the best way to approach this bug would be to break it down into
> > separate bugs that each introduce a new coordinate system with associated
> > Point/Size/Rect types, and convert some uses of nsIntPoint or gfxPoint to
> > use those types.
> 
> Isn't this what we're doing now with templates?

There's a few things going on here and I'd like to figure out where all this is going, because you'd be setting a course that would have massive effects throughout our code.

1) Name changes. You're pointing us towards using IntPointTyped<AppUnits> instead of nsPoint, for example. Is that really what we want? It feels too verbose to me.

2) Introducing four new units. I'd rather these were introduced one by one with conversion of code each time. Each unit identifier needs to be individually defined and justified.

3) Moving away from having separate classes to using the template, which has the effect of preventing us from adding type-specific inline methods. I think that's undesirable.

4) Making units and origin orthogonal parameters to the template. I remain unconvinced the extra complexity is necessary. Why not just have a single "coordinate system" identifier?

Comment 29

[Benoit Girard (:BenWa)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> (In reply to Benoit Girard (:BenWa) from comment #26)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> > > I think the best way to approach this bug would be to break it down into
> > > separate bugs that each introduce a new coordinate system with associated
> > > Point/Size/Rect types, and convert some uses of nsIntPoint or gfxPoint to
> > > use those types.
> > 
> > Isn't this what we're doing now with templates?
> 
> There's a few things going on here and I'd like to figure out where all this
> is going, because you'd be setting a course that would have massive effects
> throughout our code.
> 
> 1) Name changes. You're pointing us towards using IntPointTyped<AppUnits>
> instead of nsPoint, for example. Is that really what we want? It feels too
> verbose to me.

I'm not advocating we should replace 'ns' types with 'gfx::' types. But perhaps we could introduce typed nsPoint IF we think it's useful which I don't currently. I'm not concerned about this being too verbose. I see this change like nsRefPtr<Type>. It's more keystrokes but it also makes it harder to forget a release or in this case to forget a conversion. The extra character also carry useful information that currently is in comments IF you're lucky. (See my link below). Note that like nsRefPtr this is optional, gfx::IntPoint still works via a typedef.

> 
> 2) Introducing four new units. I'd rather these were introduced one by one
> with conversion of code each time. Each unit identifier needs to be
> individually defined and justified.
> 

We could delay introduce the units. I just tried 

> 3) Moving away from having separate classes to using the template, which has
> the effect of preventing us from adding type-specific inline methods. I
> think that's undesirable.
> 

We can still have these but they wont be members of the class.

> 4) Making units and origin orthogonal parameters to the template. I remain
> unconvinced the extra complexity is necessary. Why not just have a single
> "coordinate system" identifier?

I'm not convinced of this either. A single identifier is probably sufficient.

The kind of code I'm hoping to clean up are things like this:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#389

There's a whole pile of transformation code but its poorly understand by everyone I ask. It has many transforms, offset, scales, devPixelRatio etc... I think something like this could really help with cleaning up this code. But if we're not convinced on this approach we can certain clean up this code without it.

Comment 30

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

(In reply to Benoit Girard (:BenWa) from comment #29)
> I'm not advocating we should replace 'ns' types with 'gfx::' types. But
> perhaps we could introduce typed nsPoint IF we think it's useful which I
> don't currently.

nsPoint IS typed. It's appunits.

> > 2) Introducing four new units. I'd rather these were introduced one by one
> > with conversion of code each time. Each unit identifier needs to be
> > individually defined and justified.
> > 
> 
> We could delay introduce the units. I just tried 

?

> > 3) Moving away from having separate classes to using the template, which has
> > the effect of preventing us from adding type-specific inline methods. I
> > think that's undesirable.
> 
> We can still have these but they wont be members of the class.

I think pt.ToNearestPixels(appUnitsPerDevPixel) is nicer than ToNearestPixels(pt, appUnitsPerDevPixel). Especially when you chain conversions together.

What's wrong with using separate classes, really? There's a bit more boilerplate defining a new class, but not much really. Is that it?

I think it would be nice to rename nsPoint to AppUnitsPoint, and split nsIntPoint into DevPixelsPoint, CSSPixelsPoint, etc. Even RootLayerPixelsPoint. Would that achieve your goals?

Comment 31

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Benoit Girard (:BenWa) from comment #29)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> > 3) Moving away from having separate classes to using the template, which has
> > the effect of preventing us from adding type-specific inline methods. I
> > think that's undesirable.
> 
> We can still have these but they wont be members of the class.

Can we do something like this?

    template<class R>
    Point<R> Convert(ScalingFactor<T, R> aScale);

Where ScalingFactor is a class that contains a conversion between R and T co-ordinate spaces?

So we'd have:

    pt.Convert(convertAppUnitsToDevPixels)

We could also have an Unconvert operation that applies the reciprocal.

> > 4) Making units and origin orthogonal parameters to the template. I remain
> > unconvinced the extra complexity is necessary. Why not just have a single
> > "coordinate system" identifier?
> 
> I'm not convinced of this either. A single identifier is probably sufficient.

We should have one parameter. It can be a paremeterised class itself if we find it is necessary to eliminate bugs.

Comment 32

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30)
> What's wrong with using separate classes, really? There's a bit more
> boilerplate defining a new class, but not much really. Is that it?
> 
> I think it would be nice to rename nsPoint to AppUnitsPoint, and split
> nsIntPoint into DevPixelsPoint, CSSPixelsPoint, etc. Even
> RootLayerPixelsPoint. Would that achieve your goals?

I think this would achieve the end goals, yes (assuming it was also done for the gfx Point classes). But actually doing the split from nsIntPoint to the other classes is non-trivial and AFAICT can't be done incrementally, which makes it much harder to do.

Comment 33

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

Attachment: Patch v2

Unless I misunderstood the outcome of the meeting, this is what we want to be doing?

Comment 34

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

Attachment: Type Gfx Points/Sizes

Comment 35

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

Attachment: Start propagating a gfx::PointTyped<CSSPixel> around

This an early WIP with me strongly typing the mScrollOffset field in APZC and starting to propagate the strongness outwards from there. For now I just put the CSSPixel struct in FrameMetrics.h but I'd like to hear about a better place to put it.

Comment 36

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

Comment on attachment 744196 [details] [diff] [review]
Type Gfx Points/Rects/Sizes

Obsoleting BenWa's older patch for now.

Comment 37

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

The word Typed in TypedPoint seems redundant. We should default as in:

template<class T = UnknownUnits> Point { ... }

Comment 38

[Chris Lord [:cwiiis]]

Comment on attachment 753974 [details] [diff] [review]
Start propagating a gfx::PointTyped<CSSPixel> around

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

Fly-by to say I really like this direction - it will make the code in things like AZPC and the async shadow transform stuff much easier to understand.

Comment 39

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

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #37)
> The word Typed in TypedPoint seems redundant. We should default as in:
> 
> template<class T = UnknownUnits> Point { ... }

Unfortunately if I do that I have to modify every single reference to "Point" to be "Point<>" or the compiler gives me "invalid use of template-name ... without an argument list" errors.

Comment 40

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

Attachment: Type gfx Point/Size/Rect classes

Updated to also type the Rect and IntRect classes.

Comment 41

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

Attachment: Start propagating a gfx::PointTyped<CSSPixel> around (v2)

Propagated the CSSPixel a little bit further. I'll continue typing some of the other things in APZC/FrameMetrics, maybe with a ScreenPixels class.

Comment 42

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #39)
> (In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #37)
> > The word Typed in TypedPoint seems redundant. We should default as in:
> > 
> > template<class T = UnknownUnits> Point { ... }
> 
> Unfortunately if I do that I have to modify every single reference to
> "Point" to be "Point<>" or the compiler gives me "invalid use of
> template-name ... without an argument list" errors.

I agree that that is a solid argument in favor of TypedPoint; you can then have as many typedefs as you want without 'Typed' in them, for various specializations.

Comment 43

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

That's a good idea! I will update my second patch to have a CSSPoint typedef for TypedPoint<CSSPixel>.

Comment 44

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

Attachment: Start propagating CSSPoint around (v3)

Now with a CSSPoint typedef

Comment 45

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

Comment on attachment 754771 [details] [diff] [review]
Type gfx Point/Size/Rect classes

Requesting review on this since it seems to work pretty well. I'll convert a few of the Rect instances in widget/android to be typed to ensure that works fine as well.

Comment 46

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

Attachment: Type gfx Point/Size/Rect classes

Whoops, missed making the Rect classes extend from the template parameter. Fixed that up (and updated commit message to include Rects).

Comment 47

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #39)
> Unfortunately if I do that I have to modify every single reference to
> "Point" to be "Point<>" or the compiler gives me "invalid use of
> template-name ... without an argument list" errors.

Stupid C++ :-(

Comment 48

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

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #47)
> Stupid C++ :-(

Yeah :(

Also, try build with the patch from comment 46 is showing a suitable amount of green so far: https://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]

Comment 49

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

Err https://tbpl.mozilla.org/?tree=Try&rev=618c300ece30 actually.

Comment 50

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

Attachment: Propagate type information to some Point instances

Comment 51

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

Attachment: Propagate type information to some Rect instances

Please take a look at these patches (specifically the changes to Units.h) and let me know if there's a different way you'd prefer to see these functions defined. I feel this is a little clunky and think that some of the functions might be better as non-static, but would like other opinions as well.

Comment 52

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 755010 [details] [diff] [review]
Propagate type information to some Rect instances

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

::: layout/base/Units.h
@@ +32,5 @@
>    static gfx::Point ToUnknownPoint(const gfx::PointTyped<CSSPixel> &pt) {
>      return gfx::Point(pt.x, pt.y);
>    }
> +
> +  static gfx::RectTyped<CSSPixel> FromUnknownRect(const gfx::Rect &rect) {

Can converions to/from unknown be defined in the base class? We'll most likely just want them everywhere.

@@ +46,5 @@
> +typedef gfx::RectTyped<CSSPixel> CSSRect;
> +typedef gfx::IntRectTyped<CSSPixel> CSSIntRect;
> +
> +struct ScreenPixel {
> +  static gfx::RectTyped<ScreenPixel> FromCSSRectRoundOut(const CSSRect& rect, gfxFloat resolution) {

Resolution should ideally be of type ZoomScale.

Comment 53

[Bas Schouten (:bas.schouten)]

Comment on attachment 754942 [details] [diff] [review]
Type gfx Point/Size/Rect classes

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

This looks good to me!

Comment 54

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

Hmm, so one downside of making a type a typedef instead of a bona-fide class is that you can't forward-declare it. Not a showstopper though.

Comment 55

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

Comment on attachment 754942 [details] [diff] [review]
Type gfx Point/Size/Rect classes

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

Looks good!

Comment 56

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

\o/

Comment 57

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

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #52)
> Can converions to/from unknown be defined in the base class? We'll most
> likely just want them everywhere.

Good call, I have done that locally. I also made to ToUnknownXXX function non-static so that calling it is simpler.

> @@ +46,5 @@
> > +typedef gfx::RectTyped<CSSPixel> CSSRect;
> > +typedef gfx::IntRectTyped<CSSPixel> CSSIntRect;
> > +
> > +struct ScreenPixel {
> > +  static gfx::RectTyped<ScreenPixel> FromCSSRectRoundOut(const CSSRect& rect, gfxFloat resolution) {
> 
> Resolution should ideally be of type ZoomScale.

I'm not sure exactly what you mean here. I didn't see an existing ZoomScale type, so I assume you mean to define a new one? What would the definition be?

Once inbound re-opens I'll land the first patch and let this bug be closed, and move the other two patches (that propagate the type info) to other bugs and continue the propagation incrementally.

Comment 58

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/58bf49390ed4

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/58bf49390ed4

Comment 60

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

Comment on attachment 755009 [details] [diff] [review]
Propagate type information to some Point instances

Moved this patch (updated) to bug 877726.

Comment 61

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

Comment on attachment 755010 [details] [diff] [review]
Propagate type information to some Rect instances

Moved this patch to bug 877728.

Comment 62

[Wayne Chang [:wchang]]

making hd+

Comment 63

[Ryan VanderMeulen [:RyanVM]]

Needs a branch-specific patch for uplift.

Comment 64

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

Why does this need uplifting? It doesn't fix any bugs by itself.

Comment 65

[Wayne Chang [:wchang]]

I was informed that this is required in order to uplift bug 870311, but I see that this dependency no longer exists. Can you clarify how best 870311 can be fixed and uplifted to 1.1hd?

also ni?Kanru here to help with discussion.

Comment 66

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

Bug 870311 isn't fixed yet. This discussion should happen over in that bug.

Comment 67

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

(In particular, see https://bugzilla.mozilla.org/show_bug.cgi?id=870311#c26)

Comment 68

[Lukas Blakk [:lsblakk] use ?needinfo]

Re-adding bug 870311 as a blocker to this bug being fixed on hd branch.

Comment 69

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to [e-mail] [:lsblakk] from comment #68)
> Re-adding bug 870311 as a blocker to this bug being fixed on hd branch.

Actually bugzilla won't let me do this because of circular dependencies - Kats can you sort this out? If this bug can't be fixed on branch without bug 870311 landing we've got to get the dependency here.

Comment 70

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

I don't understand what you're trying to do. The patches on this bug don't fix any bugs. Bug 870311 isn't fixed. Neither of these bugs should be uplifted to any branches because they don't fix anything.

Comment 71

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

For the record I don't understand any of the flag changes on this bug since comment 62.

Comment 72

[Ryan VanderMeulen [:RyanVM]]

So where does this stand? Seems like we either need a branch-specific patch here or we need to clear the blocking-b2g18:hd+ flag. Can we get a final decision please?

Comment 73

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

I think the flag should be cleared. We will have branch-specific patches for some of the patches in bug 883646 and bug 870311.

Comment 74

[Wayne Chang [:wchang]]

ni?Tim/Michael on why this is needed on 1.1HD

Comment 75

[Michael Wu [:mwu]]

We shouldn't need to uplift this - this is a tool that we're using to determine where bugs are. We're currently uplifting the actual fixes without this.

Comment 76

[Ryan VanderMeulen [:RyanVM]]

Per comment 75, in that case.