Bug 44412

Summary: Teach InlineBox about FloatPoint
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: New BugsAssignee: Eric Seidel (no email) <eric>
Status: RESOLVED FIXED    
Severity: Normal CC: hyatt, jamesr, leviw, leviw, mitz
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 60318    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Eric Seidel (no email) 2010-08-23 03:22:30 PDT
Teach InlineBox about IntPoint
Comment 1 Eric Seidel (no email) 2010-08-23 03:27:44 PDT
Created attachment 65098 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-08-23 03:29:22 PDT
My rules for deciding when to use left() vs. x() are kinda arbitrary.

I used left() when it read better/made more sense.

I used x() when I wasn't sure what to use.

I used left() when using x() was impossible due to a local variable named x.
Comment 3 Darin Adler 2010-08-29 11:55:10 PDT
Comment on attachment 65098 [details]
Patch

Great that you’re taking this on. Looks good!

I can't tell how you are choosing when to use x() and y() and when you are choosing to use left() and top(). I think it would be better not to add top() and left() or to remove x() and y() if you really want to add them. One of th first curious cases I ran into was EllipsisBox::paintSelection, which seems to mix the two in way that makes it non-obvious why one part has one and one part has the other.

> +    // This is currently only used by ElipsesBox

You mean EllipisBox. Comment needs a period. This is a type of comment that can easily become incorrect, and I’m not sure of its value. We might want to just leave it out.

> -        , m_x(x)
> -        , m_y(y)
> +        , m_topLeft(topLeft)

I think position might be a better name than topLeft.

> +    IntRect rect() const
> +    {
> +        // FIXME: Will this want virtualHeight for the SVG case?
> +        return IntRect(topLeft(), IntSize(width(), height()));
> +    }

Is there some way you could resolve this and so avoid adding the FIXME? I don’t like adding confusion here.

I’m going to say review+, but I think this is not 100% improvement.
Comment 4 Eric Seidel (no email) 2010-12-20 22:32:07 PST
I still need to land this, sorry.
Comment 5 Levi Weintraub 2011-05-05 17:28:45 PDT
(In reply to comment #4)
> I still need to land this, sorry.

Unless you object, Eric, I'm going to update this patch and reupload it :)
Comment 6 Levi Weintraub 2011-05-06 13:49:57 PDT
Renaming to reflect that InlineBox now uses floats.
Comment 7 Levi Weintraub 2011-05-06 14:22:26 PDT
Created attachment 92637 [details]
Patch
Comment 8 James Robinson 2011-05-06 14:25:54 PDT
Comment on attachment 92637 [details]
Patch

Are left/right/top/bottom logical left/right/top/bottom or physical left/right/top/bottom?  Could the function names make this clear?
Comment 9 Levi Weintraub 2011-05-06 14:32:03 PDT
They are not the logical coordinates, accessors for which mirror these with "logical" prepended. I could likewise begin these with "physical".
Comment 10 Eric Seidel (no email) 2011-05-09 10:14:54 PDT
Comment on attachment 92637 [details]
Patch

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

> Source/WebCore/rendering/EllipsisBox.cpp:69
> +        tx += right() - m_markupBox->x();
> +        ty += top() + style->fontMetrics().ascent() - (m_markupBox->y() + m_markupBox->renderer()->style(m_firstLine)->fontMetrics().ascent());

Are these going to be confusing for vertical text?  I feel like Darin (or someone) picked on me about this before.  I'm not sure what the "vertical text aware" equivalent of "right" would be.
Comment 11 Levi Weintraub 2011-05-09 11:39:55 PDT
(In reply to comment #10)
> Are these going to be confusing for vertical text?  I feel like Darin (or someone) picked on me about this before.  I'm not sure what the "vertical text aware" equivalent of "right" would be.

That's a fair statement, but unfortunately I'm not entirely sure what the right name would be either. Perhaps Darin or Hyatt could weigh in with some advice?
Comment 12 Eric Seidel (no email) 2011-05-13 11:34:29 PDT
Given Hyatt's silence, I think you shoudl either try to corner him on irc, or just move forward and we can always fix later.
Comment 13 Levi Weintraub 2011-05-13 12:23:48 PDT
(In reply to comment #12)
> Given Hyatt's silence, I think you shoudl either try to corner him on irc, or just move forward and we can always fix later.

Thanks. I've been trying to corner him on this and 60317, but missed him on IRC yesterday. D'oh!
Comment 14 Levi Weintraub 2011-06-01 17:55:01 PDT
Eric, can I get a review for this so we can move forward? This is also getting in the way...
Comment 15 Eric Seidel (no email) 2011-06-01 18:02:32 PDT
Comment on attachment 92637 [details]
Patch

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

>> Source/WebCore/rendering/EllipsisBox.cpp:69
>> +        ty += top() + style->fontMetrics().ascent() - (m_markupBox->y() + m_markupBox->renderer()->style(m_firstLine)->fontMetrics().ascent());
> 
> Are these going to be confusing for vertical text?  I feel like Darin (or someone) picked on me about this before.  I'm not sure what the "vertical text aware" equivalent of "right" would be.

This is my only problem with this patch.  If you can answer if right/left are OK vs. logicalMaxX or whatever the alternative naming is, than this is fine!

> Source/WebCore/rendering/InlineBox.h:239
> +    float right() const { return left() + logicalWidth(); }

I'm confused how this relates to logicalRight.  Should this use width() instead of logicalWidth?
Comment 16 Levi Weintraub 2011-06-01 18:19:42 PDT
Comment on attachment 92637 [details]
Patch

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

>>> Source/WebCore/rendering/EllipsisBox.cpp:69
>>> +        ty += top() + style->fontMetrics().ascent() - (m_markupBox->y() + m_markupBox->renderer()->style(m_firstLine)->fontMetrics().ascent());
>> 
>> Are these going to be confusing for vertical text?  I feel like Darin (or someone) picked on me about this before.  I'm not sure what the "vertical text aware" equivalent of "right" would be.
> 
> This is my only problem with this patch.  If you can answer if right/left are OK vs. logicalMaxX or whatever the alternative naming is, than this is fine!

In the cases we're concerned about vertical text we always explicitly use "logicalRight" or "logicalTop." Either way, this isn't a change in behavior, but logically the original code seems incorrect. Since the logical width can be either a physical width or height, and the left or m_x is always physical, how does it make sense to add the logical width to it? Neither version seems correct.
Comment 17 Levi Weintraub 2011-06-02 16:03:56 PDT
Created attachment 95828 [details]
Patch
Comment 18 Eric Seidel (no email) 2011-06-02 16:20:34 PDT
Comment on attachment 95828 [details]
Patch

OK.
Comment 19 Levi Weintraub 2011-06-02 16:49:11 PDT
Comment on attachment 95828 [details]
Patch

Clearing flags on attachment: 95828

Committed r87964: <http://trac.webkit.org/changeset/87964>
Comment 20 Levi Weintraub 2011-06-02 16:56:32 PDT
Comment on attachment 65098 [details]
Patch

Clearing flags on the old patch.