Botond Ballo [:botond] Assignee
	Description • 11 years ago

Our strongly typed point, size, rect, and margin classes have been very useful for catching bugs caused by mismatched units.

One hole in this safety net is single coordinates, for which we still use plain floats. 'Axis' in particular uses single coordinates extensively. Strong typing for these could cause bugs like bug 923431 to be discovered sooner.

I propose that we introduce a new Coord class, templated on a pixel type, which represents a single coordinate. I think we can add this without disturbing any other code for now, and just use it in Axis. We can then gradually introduce it into other code if desired.

	Botond Ballo [:botond] Assignee
	Comment 1 • 11 years ago

Bas, what do you think about this idea?

	Bas Schouten (:bas.schouten)
	Comment 2 • 11 years ago

I think it might certainly be a valid idea. Although since this will be mainly used in layout I suppose the layout team's opinion might matter more :).

	Botond Ballo [:botond] Assignee
	Comment 3 • 10 years ago

Attached is a WIP patch that:

   - introduces BaseCoord<>, IntCoordTyped<> and CoordTyped<> class templates,
     in similar vein to BasePoint<>, IntPointTyped<> and PointTyped<>

   - modifies APZ's Axis class to use these instead of plain floats

   - make necessary changes to other parts of the codebase to keep it
     compilable

This currently compiles for me locally, with a Linux desktop build, except for an internal compiler error in nsEventStateManager.cpp.

	Botond Ballo [:botond] Assignee
	Comment 4 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #3)
> This currently compiles for me locally, with a Linux desktop build, except
> for an internal compiler error in nsEventStateManager.cpp.

I reported the ICE at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61959.

	(no longer active)
	Comment 5 • 10 years ago

(In reply to comment #4)
> (In reply to Botond Ballo [:botond] from comment #3)
> > This currently compiles for me locally, with a Linux desktop build, except
> > for an internal compiler error in nsEventStateManager.cpp.
> 
> I reported the ICE at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61959.

Living on the edge.  :-)

	Botond Ballo [:botond] Assignee
	Comment 6 • 10 years ago

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #5)
> > I reported the ICE at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61959.
> 
> Living on the edge.  :-)

I posted a minimal example that reproduces the crash at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61959#c4. It doesn't do anything novel, it just extends a pattern we already used for one concept (BasePoint/PointTyped) to another (BaseCoord/CoordTyped).

	Botond Ballo [:botond] Assignee
	Comment 7 • 10 years ago

I found a workaround for the ICE: not deriving 'TypedCoord<units>' and 'IntTypedCoord<units>' from 'units'.

The purpose of deriving 'TypedPoint<units>' and 'IntTypedPoint<units>' from 'units' is so that we can do things like 'LayoutDevicePoint::FromUntyped(point)' (with 'FromUntyped' being defined by 'LayoutDevicePixel'). I think we can live with not being able to call functions defined in the pixel classes via 'LayoutDeviceCoord::' and such.

	Botond Ballo [:botond] Assignee
	Comment 8 • 10 years ago

Updated patch to fix ICE. Patch now compiles locally.

Now, let's see if it compilers on other platforms: https://tbpl.mozilla.org/?tree=Try&rev=2306c0af7b54

	Botond Ballo [:botond] Assignee
	Comment 9 • 10 years ago

Cleaned up the patch a bit. Will wait for Try results before posting for review.

	Botond Ballo [:botond] Assignee
	Comment 10 • 10 years ago

Fixed some Werror and Windows bustage. ReTrying (build-only): https://tbpl.mozilla.org/?tree=Try&rev=f8f1c55dc774

	Botond Ballo [:botond] Assignee
	Comment 11 • 10 years ago

Fixed more Windows bustage. Trying again, Windows builds only this time: https://tbpl.mozilla.org/?tree=Try&rev=3189dc876731

	Botond Ballo [:botond] Assignee
	Comment 12 • 10 years ago

Updated patch will all bustages fixed. Now builds on all platforms.

Posting for review. A few design notes:

   - I transitioned the 'x' and 'y' fields of the Point classes
     to be Coords rather than plain numbers, because this was
     needed for using coordinate classes effectively in Axis
     (which is my poster child for this change). This had
     repercussions in the rest of the codebase, but they were
     relatively limited, i.e. I only had to make changes in
     a handful of places.

     I plan to similarly transition Size, Rect, and Margin,
     but I propose doing this as a follow-up, and possibly
     after some other parts of the code have been transitioned
     to use Coord, because doing it now would involve a lot
     of changes in many places.

   - Coords are currently implicitly convertible to the numeric
     type they wrap, and I provide operator overloads for most
     mixed-type operations on Coords (e.g. adding a Coord and
     a float). This, again, is to ease the transition to Coords.
     Once enough of the codebase has been transitioned to use
     Coords, most of these can be removed, and the benefits of
     strong typing realized more fully.

	Botond Ballo [:botond] Assignee
	Comment 13 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #12)
> Posting for review.

Suggested review order:

  - BaseCoord.h, Coord.h, BasePoint.h, Point.h, Units.h 
    (and moz.build, GfxMessageUtils.h)
    These are the core changes.

  - Axis.h/cpp, AsyncPanZoomController.cpp, APZCTreeManager.cpp
    These demonstrate the coordinate classes in action.

  - Everything else is fallout from changing the types of Point's
    x and y coordinates.

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

Comment on attachment 8465016 [details] [diff] [review]
Introduce strongly-typed coordinate classes

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

Bas should review the changes in gfx/2d/. Minusing for the Truncate[d] issue, but generally this looks ok to me. Do you know what kind of impact this will have on performance?

::: gfx/2d/Coord.h
@@ +104,5 @@
> +  IntCoordTyped<units> Rounded() const {
> +    return IntCoordTyped<units>(int32_t(floor(this->value + 0.5)));
> +  }
> +  IntCoordTyped<units> Truncated() const {
> +    return IntCoordTyped<units>(int32_t(this->value));

Truncate() and Truncated() will give you different results for negative numbers. floor(-3.5) will give -4 but int32_t(-3.5) will give you -3. At least that's what happens on my mac.

::: gfx/layers/apz/src/Axis.cpp
@@ +22,1 @@
>  namespace mozilla {

Put blank line back. Also in general better to separate the cosmetic changes from the functional changes in this file.

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +760,5 @@
>    // target to stop tests failing - this is only used by the Android browser
>    // UI for its dynamic toolbar.
>    IntPoint origin;
>    if (!mTarget) {
> +    origin = -RoundedToInt(mRenderOffset.ToUnknownPoint());

This adds a rounding where there was none previously. Intentional?

	Botond Ballo [:botond] Assignee
	Comment 15 • 10 years ago

Updated patch; addresses review comments.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> Do you know what kind of impact this will have on performance?

I would expect no performance impact, as the compiler should easily be able to optimize away the extra class wrapper we're adding.

> ::: gfx/2d/Coord.h
> @@ +104,5 @@
> > +  IntCoordTyped<units> Rounded() const {
> > +    return IntCoordTyped<units>(int32_t(floor(this->value + 0.5)));
> > +  }
> > +  IntCoordTyped<units> Truncated() const {
> > +    return IntCoordTyped<units>(int32_t(this->value));
> 
> Truncate() and Truncated() will give you different results for negative
> numbers. floor(-3.5) will give -4 but int32_t(-3.5) will give you -3. At
> least that's what happens on my mac.

Good catch! I changed both to be int32_t(value) to be consistent.

> ::: gfx/layers/apz/src/Axis.cpp
> @@ +22,1 @@
> >  namespace mozilla {
> 
> Put blank line back. Also in general better to separate the cosmetic changes
> from the functional changes in this file.

Eek! I must have invoked Eclipse's auto-format shortcut, and then saved without realizing it. Fixed.

> ::: gfx/layers/opengl/CompositorOGL.cpp
> @@ +760,5 @@
> >    // target to stop tests failing - this is only used by the Android browser
> >    // UI for its dynamic toolbar.
> >    IntPoint origin;
> >    if (!mTarget) {
> > +    origin = -RoundedToInt(mRenderOffset.ToUnknownPoint());
> 
> This adds a rounding where there was none previously. Intentional?

There was an implicit truncation previously. To preserve the behaviour I changed this to be an (explicit) truncation.

	Botond Ballo [:botond] Assignee
	Comment 16 • 10 years ago

Comment on attachment 8465556 [details] [diff] [review]
Introduce strongly-typed coordinate classes

Not sure why this had Kats marked for review twice.

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

Comment on attachment 8465556 [details] [diff] [review]
Introduce strongly-typed coordinate classes

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

I looked at all the stuff outside gfx/2d/ and the {Base,}{Point,Coord}.h files inside gfx/2d/. Didn't look at the other changes in gfx/2d/.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +614,5 @@
>      gfx3DMatrix transformToGecko;
>      GetInputTransforms(apzc, transformToApzc, transformToGecko);
>      gfx3DMatrix outTransform = transformToApzc * transformToGecko;
> +    aOutTransformedPoint->x = LayoutDeviceIntCoord(aPoint.x);
> +    aOutTransformedPoint->y = LayoutDeviceIntCoord(aPoint.y);

Already getting into bad habits of casting from one pixel type to another without explanation! :p

::: gfx/layers/apz/src/Axis.h
@@ +54,5 @@
>    /**
>     * Notify this Axis that a new touch has been received, including a timestamp
>     * for when the touch was received. This triggers a recalculation of velocity.
>     */
> +  void UpdateWithTouchAtDevicePoint(ScreenIntCoord aPos, uint32_t aTimestampMs);

We should probably change the ScreenIntCoords in this file to ScreenCoords. No sense rounding/truncating unless needed. That should be deferred to a follow-up bug though.

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +271,2 @@
>    const gfx::Point layerTransform = GetScrollData(aLayer);
> +  const gfx::Point layerScroll = scrollOffset.ToUnknownPoint() - layerTransform;

Check with BenWa to make sure the rounding that was here before isn't intentional. It looks kind of suspicious.

	Botond Ballo [:botond] Assignee
	Comment 18 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17)
> ::: gfx/layers/apz/src/APZCTreeManager.cpp
> @@ +614,5 @@
> >      gfx3DMatrix transformToGecko;
> >      GetInputTransforms(apzc, transformToApzc, transformToGecko);
> >      gfx3DMatrix outTransform = transformToApzc * transformToGecko;
> > +    aOutTransformedPoint->x = LayoutDeviceIntCoord(aPoint.x);
> > +    aOutTransformedPoint->y = LayoutDeviceIntCoord(aPoint.y);
> 
> Already getting into bad habits of casting from one pixel type to another
> without explanation! :p

I believe the code is correct, since we are applying a transform to the point to get it into LayoutDevice space.

One day, I hope to introduce strongly-typed matrix classes so that we can encode into the type of 'outTransform' that it converts from Screen space to LayoutDevice space.

In the meantime, I rewrote the code to use TransformTo<>(), which makes it look a bit better. This involved introducing an overload of TransformTo<>() that applies to IntPoints. Re-requesting review for these bits.

> ::: gfx/layers/apz/src/Axis.h
> @@ +54,5 @@
> >    /**
> >     * Notify this Axis that a new touch has been received, including a timestamp
> >     * for when the touch was received. This triggers a recalculation of velocity.
> >     */
> > +  void UpdateWithTouchAtDevicePoint(ScreenIntCoord aPos, uint32_t aTimestampMs);
> 
> We should probably change the ScreenIntCoords in this file to ScreenCoords.
> No sense rounding/truncating unless needed. That should be deferred to a
> follow-up bug though.

Filed bug 1046993 for this.

> ::: gfx/layers/composite/ContainerLayerComposite.cpp
> @@ +271,2 @@
> >    const gfx::Point layerTransform = GetScrollData(aLayer);
> > +  const gfx::Point layerScroll = scrollOffset.ToUnknownPoint() - layerTransform;
> 
> Check with BenWa to make sure the rounding that was here before isn't
> intentional. It looks kind of suspicious.

Checked with Mason and BenWa; the rounding wasn't intentional.

	Bas Schouten (:bas.schouten)
	Comment 19 • 10 years ago

I've already looked a little bit at the Moz2D parts and haven't seen anything particularly objectionable there, why are there a few places where we need to .value though?

Also, could you please split Moz2D changes up in a separate patch that goes in the queue before the rest? That makes everything a little cleaner.

	Botond Ballo [:botond] Assignee
	Comment 20 • 10 years ago

(In reply to Bas Schouten (:bas.schouten) from comment #19)
> I've already looked a little bit at the Moz2D parts and haven't seen
> anything particularly objectionable there, why are there a few places where
> we need to .value though?

Sometimes we do things like 'sqrt(point.x * point.x + point.y * point.y)'. I don't want to define an operator*(Coord, Coord) until we have a meaningful strongly-typed result type that would make sense for it (like an Area or something).

> Also, could you please split Moz2D changes up in a separate patch that goes
> in the queue before the rest? That makes everything a little cleaner.

Will do.

	Botond Ballo [:botond] Assignee
	Comment 22 • 10 years ago

Carrying r+ from Kats. Bas, if you'd like to review this part as well, please feel free.

	Bas Schouten (:bas.schouten)
	Comment 23 • 10 years ago

Comment on attachment 8465754 [details] [diff] [review]
Part 1 - Moz2D changes

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

I'm not 100% sure I agree with not allowing multiplication. But I'm also sensitive to your argument of it being nice to have a valid unit for those. For now let's just get this stuff in.

	Botond Ballo [:botond] Assignee
	Comment 24 • 10 years ago

This additional bit became necessary during rebasing.

	Botond Ballo [:botond] Assignee
	Comment 25 • 10 years ago

Rebased, carrying r+

	Botond Ballo [:botond] Assignee
	Comment 26 • 10 years ago

Rebased, carrying r+

	Botond Ballo [:botond] Assignee
	Comment 27 • 10 years ago
green try

Another build-only Try push to make sure nothing has broken since last time:

https://tbpl.mozilla.org/?tree=Try&rev=982e5d1e578e

	Botond Ballo [:botond] Assignee
	Comment 28 • 10 years ago
landing

https://hg.mozilla.org/integration/mozilla-inbound/rev/dff51863663c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f431232084b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8720d81b74f

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 29 • 10 years ago

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d577815636c6 for mochitest-2 failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=45371969&tree=Mozilla-Inbound


Reftest failed once on this push with https://tbpl.mozilla.org/php/getParsedLog.php?id=45369171&tree=Mozilla-Inbound but I don't know yet if it was just a one-off failure or is related to this patch.

	Botond Ballo [:botond] Assignee
	Comment 30 • 10 years ago

The test failures caught a legitimate, and tricky, bug!

The problem was that the mixed strongly-typed/primitive operations were improperly returning the primitive type as their result type. This is wrong when e.g. the strongly-typed argument is a float-valued Coord, and the primitive argument is an integer, because then the result gets truncated to an integer, so that e.g. ScreenCoord(50) / 100 would return 0 instead of 0.5.

The fix is to return the "common type" of the Coord's underlying type and the primitive argument type, i.e. the same type that C++ would return for the same operation performed on primitive types.

The only modification is to the return types in CoordOperationsHelper, and the addition of CommonType to help compute them. Asking for re-review for these changes.

I'd Try, but Try is closed.

	Botond Ballo [:botond] Assignee
	Comment 31 • 10 years ago
green try

(In reply to Botond Ballo [:botond] from comment #30)
> I'd Try, but Try is closed.

https://tbpl.mozilla.org/?tree=Try&rev=6017ad4b667e

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

Comment on attachment 8469600 [details] [diff] [review]
Part 1 - Moz2D changes

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

Stealing review back since Bas is on PTO or something. The changes look ok to me.

	Botond Ballo [:botond] Assignee
	Comment 33 • 10 years ago
green try

Doing another build-only Try push since some time has elapsed since the last one:

https://tbpl.mozilla.org/?tree=Try&rev=67e9c0e7ee35

	Botond Ballo [:botond] Assignee
	Comment 34 • 10 years ago
landing

https://hg.mozilla.org/integration/mozilla-inbound/rev/4df5daa7ca34
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a9f66a511d2

	Ed Morley [:emorley]
	Comment 35 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/4df5daa7ca34
https://hg.mozilla.org/mozilla-central/rev/6a9f66a511d2