Bug 44412 - Teach InlineBox about FloatPoint
Summary: Teach InlineBox about FloatPoint
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Eric Seidel
URL:
Keywords:
Depends on:
Blocks: 60318
  Show dependency treegraph
 
Reported: 2010-08-23 03:22 PDT by Eric Seidel
Modified: 2011-06-02 16:56 PDT (History)
5 users (show)

See Also:


Attachments
Patch (44.23 KB, patch)
2010-08-23 03:27 PDT, Eric Seidel
no flags Details | Formatted Diff | Diff
Patch (22.11 KB, patch)
2011-05-06 14:22 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (22.51 KB, patch)
2011-06-02 16:03 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel 2010-08-23 03:22:30 PDT
Teach InlineBox about IntPoint
Comment 1 Eric Seidel 2010-08-23 03:27:44 PDT
Created attachment 65098 [details]
Patch
Comment 2 Eric Seidel 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 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 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 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 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 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.