Disambiguate between Point instances in different coordinate systems

RESOLVED FIXED in mozilla24

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

(Depends on: 1 bug)

23 Branch
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 12 obsolete attachments)

10.47 KB, patch
roc
: review+
bas
: review+
Details | Diff | Splinter Review
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.
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.
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.
I support this initiative.
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.
Bug 865804 will add at least one more level of zoom, I think.
There is at least one more type of pixel now (thanks to retina MacBooks): we have both display pixels and device pixels.
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>.
Yeah I think that makes sense.

Updated

5 years ago
Blocks: 866265
(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.
Created attachment 743292 [details] [diff] [review]
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?
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #743292 - Flags: review?(bugmail.mozilla)
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.)
Attachment #743292 - Flags: review?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(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.
Created attachment 743725 [details] [diff] [review]
Type Gfx Points/Rects/Sizes

Looping in Bas for moz2d changes and roc for having this header live in layout.
Attachment #743292 - Attachment is obsolete: true
Attachment #743725 - Flags: review?(roc)
Attachment #743725 - Flags: review?(bugmail.mozilla)
Attachment #743725 - Flags: review?(bas)
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 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.
Attachment #743725 - Flags: review?(bugmail.mozilla) → review+
Should the Pixels classes include conversion functions to or from other units? Explicit "convert to" functions sound safer than implicit "convert from" constructors.
(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.
Created attachment 743802 [details] [diff] [review]
Type Gfx Points/Rects/Sizes
Attachment #743725 - Attachment is obsolete: true
Attachment #743725 - Flags: review?(roc)
Attachment #743725 - Flags: review?(bas)
Attachment #743802 - Flags: review?(roc)

Updated

5 years ago
Attachment #743802 - Flags: review?(bas)
(In reply to Kartikaya Gupta (email:kats@mozilla.com, 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 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?
(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.
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.
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.
(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.
(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?
Created attachment 744196 [details] [diff] [review]
Type Gfx Points/Rects/Sizes

Fixed build error with forward declare.
Attachment #743802 - Attachment is obsolete: true
Attachment #743802 - Flags: review?(roc)
Attachment #743802 - Flags: review?(bas)
Attachment #744196 - Flags: review?(roc)
Attachment #744196 - Flags: review?(bas)

Updated

5 years ago
Attachment #744196 - Flags: review?(roc)
(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?
(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.
(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?
(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.
(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.
Created attachment 753941 [details] [diff] [review]
Patch v2

Unless I misunderstood the outcome of the meeting, this is what we want to be doing?
Created attachment 753964 [details] [diff] [review]
Type Gfx Points/Sizes
Attachment #753941 - Attachment is obsolete: true
Created attachment 753974 [details] [diff] [review]
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 on attachment 744196 [details] [diff] [review]
Type Gfx Points/Rects/Sizes

Obsoleting BenWa's older patch for now.
Attachment #744196 - Attachment is obsolete: true
Attachment #744196 - Flags: review?(roc)
Attachment #744196 - Flags: review?(bas)
Attachment #753964 - Attachment description: Patch v2 → Type Gfx Points/Sizes
The word Typed in TypedPoint seems redundant. We should default as in:

template<class T = UnknownUnits> Point { ... }
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.
Attachment #753974 - Flags: feedback+
(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.
Created attachment 754771 [details] [diff] [review]
Type gfx Point/Size/Rect classes

Updated to also type the Rect and IntRect classes.
Attachment #753964 - Attachment is obsolete: true
Created attachment 754772 [details] [diff] [review]
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.
Attachment #753974 - Attachment is obsolete: true
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
That's a good idea! I will update my second patch to have a CSSPoint typedef for TypedPoint<CSSPixel>.
Created attachment 754803 [details] [diff] [review]
Start propagating CSSPoint around (v3)

Now with a CSSPoint typedef
Attachment #754772 - Attachment is obsolete: true
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.
Attachment #754771 - Flags: review?(roc)
Attachment #754771 - Flags: review?(bas)
Created attachment 754942 [details] [diff] [review]
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).
Attachment #754771 - Attachment is obsolete: true
Attachment #754771 - Flags: review?(roc)
Attachment #754771 - Flags: review?(bas)
Attachment #754942 - Flags: review?(roc)
Attachment #754942 - Flags: review?(bas)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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++ :-(
(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=kgupta@mozilla.com
Created attachment 755009 [details] [diff] [review]
Propagate type information to some Point instances
Attachment #754803 - Attachment is obsolete: true
Attachment #755009 - Flags: feedback?
Created attachment 755010 [details] [diff] [review]
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.
Attachment #755010 - Flags: feedback?
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 on attachment 754942 [details] [diff] [review]
Type gfx Point/Size/Rect classes

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

This looks good to me!
Attachment #754942 - Flags: review?(bas) → review+
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 on attachment 754942 [details] [diff] [review]
Type gfx Point/Size/Rect classes

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

Looks good!
Attachment #754942 - Flags: review?(roc) → review+
\o/
(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.
Assignee: bgirard → bugmail.mozilla
https://hg.mozilla.org/mozilla-central/rev/58bf49390ed4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Blocks: 877726
Blocks: 877728
Comment on attachment 755009 [details] [diff] [review]
Propagate type information to some Point instances

Moved this patch (updated) to bug 877726.
Attachment #755009 - Attachment is obsolete: true
Attachment #755009 - Flags: feedback?
Comment on attachment 755010 [details] [diff] [review]
Propagate type information to some Rect instances

Moved this patch to bug 877728.
Attachment #755010 - Attachment is obsolete: true
Attachment #755010 - Flags: feedback?

Updated

5 years ago
Blocks: 870311

Updated

5 years ago
Blocks: 878874
Blocks: 879004
Blocks: 879011
No longer blocks: 870311

Updated

5 years ago
Depends on: 879739
Blocks: 879739
No longer depends on: 879739
making hd+
blocking-b2g: --- → hd+
Blocks: 880676
Blocks: 883646
Needs a branch-specific patch for uplift.
Flags: needinfo?(bugmail.mozilla)
Keywords: branch-patch-needed
Why does this need uplifting? It doesn't fix any bugs by itself.
Flags: needinfo?(bugmail.mozilla) → needinfo?(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.
Flags: needinfo?(wchang)
Bug 870311 isn't fixed yet. This discussion should happen over in that bug.
status-b2g-v1.1hd: --- → affected

Updated

5 years ago
Blocks: 888235

Updated

5 years ago
Blocks: 888318

Updated

5 years ago
Blocks: 888321

Updated

5 years ago
Blocks: 888365
Re-adding bug 870311 as a blocker to this bug being fixed on hd branch.
(In reply to lsblakk@mozilla.com [: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.
Flags: needinfo?(bugmail.mozilla)
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.
Flags: needinfo?(bugmail.mozilla)
For the record I don't understand any of the flag changes on this bug since comment 62.

Updated

5 years ago
Blocks: 890821
Blocks: 890938
Blocks: 891468
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?
Flags: needinfo?(wchang)
I think the flag should be cleared. We will have branch-specific patches for some of the patches in bug 883646 and bug 870311.
ni?Tim/Michael on why this is needed on 1.1HD
Flags: needinfo?(wchang)
Flags: needinfo?(timdream)
Flags: needinfo?(mwu)

Comment 75

5 years ago
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.
Flags: needinfo?(mwu)
Per comment 75, in that case.
blocking-b2g: hd+ → ---
status-b2g-v1.1hd: affected → ---
Flags: needinfo?(timdream)
Keywords: branch-patch-needed
Depends on: 912166
Depends on: 912167
You need to log in before you can comment on or make changes to this bug.