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

Emil A Eklund
Reported 2011-06-08 13:13:04 PDT
This is the last remaining occurrence of "int tx, int ty". Yay!
Attachments
Patch (4.50 KB, patch)
2011-06-08 13:46 PDT, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
Patch (4.80 KB, patch)
2011-06-08 14:42 PDT, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
Patch (4.50 KB, patch)
2011-06-08 14:58 PDT, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Emil A Eklund
Comment 1 2011-06-08 13:46:07 PDT
Created attachment 96472 [details] Patch
Eric Seidel (no email)
Comment 2 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?
Emil A Eklund
Comment 3 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.
Eric Seidel (no email)
Comment 4 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.
Emil A Eklund
Comment 5 2011-06-08 14:42:15 PDT
Created attachment 96484 [details] Patch
Emil A Eklund
Comment 6 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.
Eric Seidel (no email)
Comment 7 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).
Eric Seidel (no email)
Comment 8 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.
Emil A Eklund
Comment 9 2011-06-08 14:58:03 PDT
Created attachment 96489 [details] Patch
Emil A Eklund
Comment 10 2011-06-08 15:21:42 PDT
Thanks Eric!
Levi Weintraub
Comment 11 2011-06-08 15:54:26 PDT
Finally! Frobnostication!!
WebKit Review Bot
Comment 12 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>
WebKit Review Bot
Comment 13 2011-06-08 16:00:21 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.