Bug 28067 – TransformationMatrix should have rectToRect convenience function

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Adam Treat]

The following patch adds rectToRect convenience function which returns the TransformationMatrix which maps the 'from' rectangle to the 'to' rectangle.

No tests as this would require binding the method to js and since it is static that doesn't seem possible.  Please check the math.

[Adam Treat]

Created attachment 34271 [details]
Adds method rectToRect

[Darin Adler]

Comment on attachment 34271 [details]
Adds method rectToRect

Does this have the desirable behavior when from.width() or from.height() is 0?

This does not match the pattern of other functions that manipulate the matrix. For example, we don't have a function to return the identity matrix. Instead we have a setter, makeIdentity(). Probably this should be makeRectToRect() or makeIdentity() should be changed.

We don't have any other static member functions to create new matrices. And in classes where we do have such functions we normally use the word "create" in the name of the function.

Seems OK to add it.

I'm going to say r=me, but I think the class's interface is a little weak and getting a little weaker.

[Adam Treat]

Created attachment 34272 [details]
Adds asserts

[Adam Treat]

makeRectToRect is ugly imo.  In this case, we want to create a transform from scratch, not modify an existing one.  I guess we could make a new constructor that takes to rects, but that seems ugly too.

[Eric Seidel (no email)]

Comment on attachment 34272 [details]
Adds asserts

I think we should deploy this in at least one place to make sure it works and is tested.  I think this patch is OK as is, but I'd rather see it used in at least one of the places where we already do this by hand.  For example, the SVG viewport code does this kind of thing.

SVGPaintServerGradient:
    TransformationMatrix matrix;
    if (gradientServer->boundingBoxMode()) {
        matrix.translate(maskBBox.x(), maskBBox.y());
        matrix.scaleNonUniform(maskBBox.width(), maskBBox.height());
    }

TransformationMatrix SVGPreserveAspectRatio::getCTM(double logicX, double logicY,

There are many other examples in the code.  Not all of them would need to be deployed to for this to be tested. :)

[George Staikos]

Comment on attachment 34272 [details]
Adds asserts

No reason to ASSERT(!to.isEmpty()), but from is fine.

[Adam Treat]

Created attachment 34279 [details]
Fix assert as pointed out by staikos

[George Staikos]

Comment on attachment 34279 [details]
Fix assert as pointed out by staikos

Patch on its own is fine now.  Would be a good idea to follow up with a usage somewhere as Eric pointed out.

[Adam Treat]

Landed with r46893.

[Darin Adler]

(In reply to comment #4)
> makeRectToRect is ugly imo.  In this case, we want to create a transform from
> scratch, not modify an existing one.

OK, that addresses one of my comments.

What about the comment that we normally use the name "create" in all of our named constructor names?

And what about changing makeIdentity?

It seems like you responded to one part of the comment and didn't address the rest. I'd really like to hear your thoughts.

[Adam Treat]

(In reply to comment #10)
> (In reply to comment #4)
> > makeRectToRect is ugly imo.  In this case, we want to create a transform from
> > scratch, not modify an existing one.
> 
> OK, that addresses one of my comments.
> 
> What about the comment that we normally use the name "create" in all of our
> named constructor names?
> 
> And what about changing makeIdentity?
> 
> It seems like you responded to one part of the comment and didn't address the
> rest. I'd really like to hear your thoughts.

Ah, sorry Darin.  My eyes missed the "create" comment.  I can certainly change it to 'createRectToRect', but I've noticed that other implementations use the 'rectToRect' naming pattern or 'quadToQuad'.  This is true of Qt's API and OpenVG API for instance.  Given that we already use such substandard naming patterns for consistency with other standard API's (getCTM and concatCTM come to mind... *ugly*) I'm not sure what is best.  'createRectToRect' seems good to me, but if we do that then I think we should change 'getCTM' and 'concatCTM'.  Better names are better names :)

As for changing 'makeIdentity' I had looked at this, but forgot to comment.  If you want to create a new identity TransformationMatrix there is no reason to use 'makeIdentity' as the default ctor returns an identity matrix.  The only time someone would use 'makeIdentity' is if you wanted to reset an existing matrix.  To me, makeIdentity is strictly not necessary and should be private.  matrix = TransformationMatrix() accomplishes the same.  I can change this and refactor parts that use 'makeIdentity' if you like?

[Darin Adler]

(In reply to comment #11)
> Ah, sorry Darin.  My eyes missed the "create" comment.  I can certainly change
> it to 'createRectToRect', but I've noticed that other implementations use the
> 'rectToRect' naming pattern or 'quadToQuad'.  This is true of Qt's API and
> OpenVG API for instance.  Given that we already use such substandard naming
> patterns for consistency with other standard API's (getCTM and concatCTM come
> to mind... *ugly*) I'm not sure what is best.  'createRectToRect' seems good to
> me, but if we do that then I think we should change 'getCTM' and 'concatCTM'. 
> Better names are better names :)

Yes, I think this class is currently not using good names across the board and is worth improving.

It seems a shame that this operation is unique at this point. The only named constructor in the class. But I guess we've discussed it sufficiently.

> As for changing 'makeIdentity' I had looked at this, but forgot to comment.  If
> you want to create a new identity TransformationMatrix there is no reason to
> use 'makeIdentity' as the default ctor returns an identity matrix.  The only
> time someone would use 'makeIdentity' is if you wanted to reset an existing
> matrix.  To me, makeIdentity is strictly not necessary and should be private. 
> matrix = TransformationMatrix() accomplishes the same.  I can change this and
> refactor parts that use 'makeIdentity' if you like?

I'm not sure it would be an improvement. I guess we can just leave it alone for now. The class's interface seems a little weak at the moment but I don't see an obvious change that will make things better.