Bug 62313

Summary: Frobnosticate the RenderEmbeddedObject::getReplacementTextGeometry
Product: WebKit Reporter: Emil A Eklund <eae>
Component: Layout and RenderingAssignee: Emil A Eklund <eae>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, leviw, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 60318    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Emil A Eklund 2011-06-08 13:13:04 PDT
This is the last remaining occurrence of "int tx, int ty". Yay!
Comment 1 Emil A Eklund 2011-06-08 13:46:07 PDT
Created attachment 96472 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-06-08 14:08:43 PDT
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?
Comment 3 Emil A Eklund 2011-06-08 14:10:44 PDT
(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.
Comment 4 Eric Seidel (no email) 2011-06-08 14:18:01 PDT
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.
Comment 5 Emil A Eklund 2011-06-08 14:42:15 PDT
Created attachment 96484 [details]
Patch
Comment 6 Emil A Eklund 2011-06-08 14:44:49 PDT
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.
Comment 7 Eric Seidel (no email) 2011-06-08 14:45:27 PDT
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).
Comment 8 Eric Seidel (no email) 2011-06-08 14:46:11 PDT
No, I think the FloatRect addition of moveBy(FloatPoint) is good.  I just don't think you want an IntPoint version.
Comment 9 Emil A Eklund 2011-06-08 14:58:03 PDT
Created attachment 96489 [details]
Patch
Comment 10 Emil A Eklund 2011-06-08 15:21:42 PDT
Thanks Eric!
Comment 11 Levi Weintraub 2011-06-08 15:54:26 PDT
Finally! Frobnostication!!
Comment 12 WebKit Review Bot 2011-06-08 16:00:17 PDT
Comment on attachment 96489 [details]
Patch

Clearing flags on attachment: 96489

Committed r88400: <http://trac.webkit.org/changeset/88400>
Comment 13 WebKit Review Bot 2011-06-08 16:00:21 PDT
All reviewed patches have been landed.  Closing bug.