Bug 62313 – Frobnosticate the RenderEmbeddedObject::getReplacementTextGeometry

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

[Emil A Eklund]

This is the last remaining occurrence of "int tx, int ty". Yay!

[Emil A Eklund]

Created attachment 96472 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 96472 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96472&action=review

> Source/WebCore/platform/graphics/FloatRect.h:110
> +    void moveBy(const IntPoint& delta) { m_location.move(delta.x(), delta.y()); }

This should be FloatPoint, no?

[Emil A Eklund]

(In reply to comment #2)
> (From update of attachment 96472 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96472&action=review
> 
> > Source/WebCore/platform/graphics/FloatRect.h:110
> > +    void moveBy(const IntPoint& delta) { m_location.move(delta.x(), delta.y()); }
> 
> This should be FloatPoint, no?

No I actually want it to be IntPoint here, that way we save an extra conversion from int->float. I could add a FloatPoint version of moveBy in addition to the IntPoint one if you'd like.

[Eric Seidel (no email)]

Comment on attachment 96472 [details]
Patch

Please add a Float version and a comment as to why you're adding the Int version. We don't normally have Int versions in these Float files.

[Emil A Eklund]

Created attachment 96484 [details]
Patch

[Emil A Eklund]

PTAL. I'd be happy to revert the FloatRect changes and just do

rect.move(accumulatedOffset.x(), accumulatedOffset.y())

if you think that makes more sense.

[Eric Seidel (no email)]

Comment on attachment 96484 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96484&action=review

Otherwise seems OK.

> Source/WebCore/platform/graphics/FloatRect.h:111
> +    void moveBy(const IntPoint& delta) { m_location.move(delta.x(), delta.y()); }

You should put a comment next to the IntPoint moveby as to why it's desired.

Something like
// We have an IntPoint version of moveBy to avoid unecessary float->int conversion when moving by an IntPoint.

Actually, I don't see how your version helps at all?

We already do the float to int conversion in the move( ) call, why not just do it once with the INtPoint->FloatPoint implicit converstion.

I think we should remove your moveBy(IntPoint).

[Eric Seidel (no email)]

No, I think the FloatRect addition of moveBy(FloatPoint) is good.  I just don't think you want an IntPoint version.

[Emil A Eklund]

Created attachment 96489 [details]
Patch

[Emil A Eklund]

Thanks Eric!

[Levi Weintraub]

Finally! Frobnostication!!

[WebKit Review Bot]

Comment on attachment 96489 [details]
Patch

Clearing flags on attachment: 96489

Committed r88400: <http://trac.webkit.org/changeset/88400>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.