Bug 44412

Created attachment 65098 [details] Patch

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 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.

I still need to land this, sorry.

(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 :)

Renaming to reflect that InlineBox now uses floats.

Created attachment 92637 [details] Patch

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?

They are not the logical coordinates, accessors for which mirror these with "logical" prepended. I could likewise begin these with "physical".

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.

(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?

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.

(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!

Eric, can I get a review for this so we can move forward? This is also getting in the way...

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 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.

Created attachment 95828 [details] Patch

Comment on attachment 95828 [details] Patch OK.

Comment on attachment 95828 [details] Patch Clearing flags on attachment: 95828 Committed r87964: <http://trac.webkit.org/changeset/87964>

Comment on attachment 65098 [details] Patch Clearing flags on the old patch.