Bug 61562 – Switch paintCustomHighlight to use IntPoint

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Levi Weintraub]

Removing tx/ty.

[Levi Weintraub]

The paint code *mostly* treats tx/ty like a size, but Darin's comment https://bugs.webkit.org/show_bug.cgi?id=60783#c6 is still echoing in my mind before I convert all the paint code to one or the other...

[Darin Adler]

It’s hard to tell points and sizes apart when we have nested coordinate systems. The distance from the top left to a rect is a “size”, yet it’s expressed as an origin point. I think that whenever we can’t decide, we should use IntPoint, and we should use IntSize only when it’s clearly the size of something, not just a distance (a point described relative to another point).

[Levi Weintraub]

Thanks Darin. I like that idea a lot.

[Emil A Eklund]

One of the drawbacks of using IntPoint for this is lack of an easy way to adjust one point by another. Adding IntPoint, IntPoint versions of the + and += operators would alleviate this concern even though strictly speaking adding two points doesn't make a whole lot of sense.

[Levi Weintraub]

Plus currently adding two points yields a size, which conceptually makes sense, but can be quite unfortunate in practice...

[Eric Seidel (no email)]

I can't tell if our problem is that the Size/Point division is broken, or if that the Rendering tree is just not designed in a way to use Size/Point elegantly.  Probably a combination of both.

[Eric Seidel (no email)]

(In reply to comment #6)
> I can't tell if our problem is that the Size/Point division is broken, or if that the Rendering tree is just not designed in a way to use Size/Point elegantly.  Probably a combination of both.

"broken" is a stronger word than I mean.  I suspect that one could re-design the rendering tree to work nicely with Points and Sizes.  But as written, the division is very cumbersome to work with.

[Levi Weintraub]

Created attachment 95065 [details]
Patch

[Darin Adler]

(In reply to comment #4)
> One of the drawbacks of using IntPoint for this is lack of an easy way to adjust one point by another. Adding IntPoint, IntPoint versions of the + and += operators would alleviate this concern even though strictly speaking adding two points doesn't make a whole lot of sense.

Yes, we should add those. There is almost no downside.

[Darin Adler]

(In reply to comment #5)
> Plus currently adding two points yields a size, which conceptually makes sense, but can be quite unfortunate in practice...

You mean subtracting, right?

[Darin Adler]

(In reply to comment #7)
> (In reply to comment #6)
> > I can't tell if our problem is that the Size/Point division is broken, or if that the Rendering tree is just not designed in a way to use Size/Point elegantly.  Probably a combination of both.
> 
> "broken" is a stronger word than I mean.  I suspect that one could re-design the rendering tree to work nicely with Points and Sizes.  But as written, the division is very cumbersome to work with.

I don’t agree that you could do that. The fundamental question is whether an origin point is a point or a size. Making it a size sounds logical but it’s no good, because there is a thing called a coordinate system, where a point is an origin and another point is relative to that point.

[Eric Seidel (no email)]

(In reply to comment #11)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > I can't tell if our problem is that the Size/Point division is broken, or if that the Rendering tree is just not designed in a way to use Size/Point elegantly.  Probably a combination of both.
> > 
> > "broken" is a stronger word than I mean.  I suspect that one could re-design the rendering tree to work nicely with Points and Sizes.  But as written, the division is very cumbersome to work with.
> 
> I don’t agree that you could do that. The fundamental question is whether an origin point is a point or a size. Making it a size sounds logical but it’s no good, because there is a thing called a coordinate system, where a point is an origin and another point is relative to that point.

We could use a system more like SVG where instead of passing in the origin relative to the containing block, we instead transform the points/CTM as we go up the call stack (also seen as transforming the resulting rects/points as we come back down the call stack).

This would mean changing hit testing to translate the passed in hit point into the child's coordinate system before passing it in, instead of building up a tx, ty (which we can't decide if it's a point vs. size).
http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/svg/RenderSVGContainer.cpp#L165

Similarly, we could have all methods on objects return things in local coordinates with the responsibility of the calling parent to translate as necessary.
http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/svg/SVGRenderSupport.cpp#L61
http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/svg/SVGRenderSupport.cpp#L168

But my thinking may be too tied to the SVG world and not fitting what is actually needed for CSS.

[Levi Weintraub]

*Ping* for review. This is starting to get in the way.

[Eric Seidel (no email)]

Comment on attachment 95065 [details]
Patch

Sorry, I just didn't see it go by.

[Levi Weintraub]

(In reply to comment #14)
> (From update of attachment 95065 [details])
> Sorry, I just didn't see it go by.

Thanks! I tried to ping ya on IRC but you weren't there :)

[Eric Seidel (no email)]

Sorry, been doing an OS re-installs and forget to log back on.

[Levi Weintraub]

Committed r87866: <http://trac.webkit.org/changeset/87866>