There's no need to do any integral snapping on InlineBox.
Created attachment 237431 [details] Patch
Attachment 237431 [details] did not pass style-queue: ERROR: Source/WebCore/rendering/InlineBox.h:378: Wrong number of spaces before statement. (expected: 18) [whitespace/indent] [4] ERROR: Source/WebCore/rendering/InlineBox.h:379: Wrong number of spaces before statement. (expected: 18) [whitespace/indent] [4] ERROR: Source/WebCore/rendering/InlineBox.h:380: Wrong number of spaces before statement. (expected: 18) [whitespace/indent] [4] Total errors found: 3 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 237431 [details] Patch Attachment 237431 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6211110424281088 New failing tests: tables/mozilla/bugs/bug96334.html fast/forms/control-restrict-line-height.html fast/forms/minWidthPercent.html fast/css/text-overflow-input.html fast/forms/input-appearance-selection.html fast/forms/input-text-scroll-left-on-blur.html tables/mozilla/bugs/bug59354.html fast/forms/input-text-drag-down.html fast/forms/input-appearance-preventDefault.html fast/forms/basic-inputs.html fast/forms/search/search-size-with-decorations.html fast/forms/input-text-word-wrap.html editing/input/caret-at-the-edge-of-input.html http/tests/navigation/javascriptlink-frames.html
Created attachment 237432 [details] Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 237431 [details] Patch Attachment 237431 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6005782533373952 New failing tests: tables/mozilla/bugs/bug96334.html fast/forms/control-restrict-line-height.html fast/forms/minWidthPercent.html editing/pasteboard/drop-text-without-selection.html fast/css/text-overflow-input.html fast/forms/input-appearance-selection.html fast/forms/input-text-scroll-left-on-blur.html tables/mozilla/bugs/bug59354.html fast/forms/input-text-drag-down.html fast/forms/input-appearance-preventDefault.html fast/forms/basic-inputs.html fast/forms/search/search-size-with-decorations.html fast/forms/input-text-word-wrap.html editing/input/caret-at-the-edge-of-input.html http/tests/navigation/javascriptlink-frames.html
Created attachment 237433 [details] Archive of layout-test-results from webkit-ews-08 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 237434 [details] Patch
Attachment 237434 [details] did not pass style-queue: ERROR: Source/WebCore/rendering/InlineBox.h:378: Wrong number of spaces before statement. (expected: 18) [whitespace/indent] [4] ERROR: Source/WebCore/rendering/InlineBox.h:379: Wrong number of spaces before statement. (expected: 18) [whitespace/indent] [4] ERROR: Source/WebCore/rendering/InlineBox.h:380: Wrong number of spaces before statement. (expected: 18) [whitespace/indent] [4] Total errors found: 3 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 237434 [details] Patch Attachment 237434 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5048435396837376 New failing tests: editing/pasteboard/drop-text-without-selection.html
Created attachment 237435 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 237434 [details] Patch Attachment 237434 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6194158188363776 New failing tests: editing/pasteboard/drop-text-without-selection.html
Created attachment 237437 [details] Archive of layout-test-results from webkit-ews-04 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 237434 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=237434&action=review > Source/WebCore/ChangeLog:8 > + We should not integral snap during layout time. It is against subpixel layout. "It is against sub pixel layout" is an awkward sentence. > Source/WebCore/rendering/InlineBox.h:177 > + void setY(float y) { m_topLeft.setY(y); } // y() is the top side of the box in the containing block's coordinate system. It seems odd to move the comment for y() but not the one for x(). Either move both, or neither.
(In reply to comment #13) > (From update of attachment 237434 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=237434&action=review > > > Source/WebCore/ChangeLog:8 > > + We should not integral snap during layout time. It is against subpixel layout. > > "It is against sub pixel layout" is an awkward sentence. > > > Source/WebCore/rendering/InlineBox.h:177 > > + void setY(float y) { m_topLeft.setY(y); } // y() is the top side of the box in the containing block's coordinate system. > > It seems odd to move the comment for y() but not the one for x(). Either move both, or neither. I must have missed that. thanks!
Comment on attachment 237434 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=237434&action=review Unofficial r=me (assuming you work through the remaining test failures) > Source/WebCore/rendering/InlineBox.h:-175 > - void setX(float x) { m_topLeft.setX(x); } > float x() const { return m_topLeft.x(); } > - float left() const { return m_topLeft.x(); } > - > - // y() is the top side of the box in the containing block's coordinate system. > - void setY(float y) { m_topLeft.setY(y); } > float y() const { return m_topLeft.y(); } > - float top() const { return m_topLeft.y(); } > - > - const FloatPoint& topLeft() const { return m_topLeft; } > - > float width() const { return isHorizontal() ? logicalWidth() : logicalHeight(); } > float height() const { return isHorizontal() ? logicalHeight() : logicalWidth(); } > - FloatSize size() const { return FloatSize(width(), height()); } Why did you move these? > Source/WebCore/rendering/InlineBox.h:270 > +protected: > + FloatPoint m_topLeft; > + float m_logicalWidth; > + I don't see you accessing these members in a child class in this patch. Why did you move them to protected:? > Source/WebCore/rendering/InlineFlowBox.cpp:894 > + LayoutUnit logicalTopVisualOverflow = std::min<LayoutUnit>(textBox.logicalTop() + childOverflowLogicalTop, logicalVisualOverflow.y()); > + LayoutUnit logicalBottomVisualOverflow = std::max<LayoutUnit>(textBox.logicalBottom() + childOverflowLogicalBottom, logicalVisualOverflow.maxY()); > + LayoutUnit logicalLeftVisualOverflow = std::min<LayoutUnit>(textBox.logicalLeft() + childOverflowLogicalLeft, logicalVisualOverflow.x()); > + LayoutUnit logicalRightVisualOverflow = std::max<LayoutUnit>(textBox.logicalRight() + childOverflowLogicalRight, logicalVisualOverflow.maxX()); Is the template argument really necessary? If I'm not mistaken, the two arguments are already LayoutUnits
Created attachment 237520 [details] Patch
Attachment 237520 [details] did not pass style-queue: ERROR: Source/WebCore/rendering/InlineBox.h:379: Wrong number of spaces before statement. (expected: 18) [whitespace/indent] [4] ERROR: Source/WebCore/rendering/InlineBox.h:380: Wrong number of spaces before statement. (expected: 18) [whitespace/indent] [4] ERROR: Source/WebCore/rendering/InlineBox.h:381: Wrong number of spaces before statement. (expected: 18) [whitespace/indent] [4] Total errors found: 3 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 237521 [details] Patch
Attachment 237521 [details] did not pass style-queue: ERROR: Source/WebCore/rendering/InlineBox.h:379: Wrong number of spaces before statement. (expected: 18) [whitespace/indent] [4] ERROR: Source/WebCore/rendering/InlineBox.h:380: Wrong number of spaces before statement. (expected: 18) [whitespace/indent] [4] ERROR: Source/WebCore/rendering/InlineBox.h:381: Wrong number of spaces before statement. (expected: 18) [whitespace/indent] [4] Total errors found: 3 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #15) > (From update of attachment 237434 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=237434&action=review > > Unofficial r=me (assuming you work through the remaining test failures) > > > Source/WebCore/rendering/InlineBox.h:-175 > > - void setX(float x) { m_topLeft.setX(x); } > > float x() const { return m_topLeft.x(); } > > - float left() const { return m_topLeft.x(); } > > - > > - // y() is the top side of the box in the containing block's coordinate system. > > - void setY(float y) { m_topLeft.setY(y); } > > float y() const { return m_topLeft.y(); } > > - float top() const { return m_topLeft.y(); } > > - > > - const FloatPoint& topLeft() const { return m_topLeft; } > > - > > float width() const { return isHorizontal() ? logicalWidth() : logicalHeight(); } > > float height() const { return isHorizontal() ? logicalHeight() : logicalWidth(); } > > - FloatSize size() const { return FloatSize(width(), height()); } > > Why did you move these? To have them grouped the same way we group geometry functions in RenderBox. > > Source/WebCore/rendering/InlineBox.h:270 > > +protected: > > + FloatPoint m_topLeft; > > + float m_logicalWidth; > > + > > I don't see you accessing these members in a child class in this patch. Why did you move them to protected:? I moved them from public to protected. > > > Source/WebCore/rendering/InlineFlowBox.cpp:894 > > + LayoutUnit logicalTopVisualOverflow = std::min<LayoutUnit>(textBox.logicalTop() + childOverflowLogicalTop, logicalVisualOverflow.y()); > > + LayoutUnit logicalBottomVisualOverflow = std::max<LayoutUnit>(textBox.logicalBottom() + childOverflowLogicalBottom, logicalVisualOverflow.maxY()); > > + LayoutUnit logicalLeftVisualOverflow = std::min<LayoutUnit>(textBox.logicalLeft() + childOverflowLogicalLeft, logicalVisualOverflow.x()); > > + LayoutUnit logicalRightVisualOverflow = std::max<LayoutUnit>(textBox.logicalRight() + childOverflowLogicalRight, logicalVisualOverflow.maxX()); > > Is the template argument really necessary? If I'm not mistaken, the two arguments are already LayoutUnits textBox.logical*() functions return float and LayoutUnit operator+'s return type is float. Fixed it though to save one implicit float -> LayoutUnit conversion. Thanks for the review.
Comment on attachment 237521 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=237521&action=review The patch should be tighter with fewer unnecessary changes. The rearranging of data members and function members should be done separately. > Source/WebCore/rendering/InlineBox.h:168 > + float left() const { return m_topLeft.x(); } Can we save the reordering of these functions for a later patch? it makes this patch much harder to read. I’m also not sure it’s a big improvement. > Source/WebCore/rendering/InlineBox.h:270 > +protected: > + FloatPoint m_topLeft; > + float m_logicalWidth; Do these need to stay protected? Can we make them private and add protected setters? Can we just leave these sorted where they were before if we think they can eventually be private?
Created attachment 237623 [details] Patch
(In reply to comment #21) > (From update of attachment 237521 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=237521&action=review > > The patch should be tighter with fewer unnecessary changes. The rearranging of data members and function members should be done separately. Removed. > > > Source/WebCore/rendering/InlineBox.h:168 > > + float left() const { return m_topLeft.x(); } > > Can we save the reordering of these functions for a later patch? it makes this patch much harder to read. I’m also not sure it’s a big improvement. I think RenderBox offers better geometry function grouping than InlineBox so I was trying to apply RenderBox's pattern here. The diff does not reflect the difference very well: current grouping: void setX(float x) float x() const float left() const void setY(float y) float y() const float top() const const FloatPoint& topLeft() const float width() const float height() const FloatSize size() const float right() const float bottom() const float logicalLeft() const float logicalRight() const void setLogicalLeft(float left) float logicalTop() const float logicalBottom() const void setLogicalTop(float top) void setLogicalWidth(float w) float logicalWidth() const float logicalHeight() const; FloatRect logicalFrameRect() const FloatRect frameRect() const proposed in the patch: float x() const float y() const float width() const float height() const float left() const float right() const float top() const float bottom() const FloatRect frameRect() const FloatSize size() const const FloatPoint& topLeft() const void setX(float x) void setY(float y) float logicalLeft() const float logicalRight() const float logicalTop() const float logicalBottom() const float logicalWidth() const float logicalHeight() const; FloatRect logicalFrameRect() const void setLogicalLeft(float left) void setLogicalTop(float top) void setLogicalWidth(float w) > > > Source/WebCore/rendering/InlineBox.h:270 > > +protected: > > + FloatPoint m_topLeft; > > + float m_logicalWidth; > > Do these need to stay protected? Can we make them private and add protected setters? > > Can we just leave these sorted where they were before if we think they can eventually be private? I'll do that in a separate patch. Thanks!
Comment on attachment 237623 [details] Patch Nice work! r=me.
Created attachment 261395 [details] Patch
Created attachment 261400 [details] Patch
Comment on attachment 261400 [details] Patch Clearing flags on attachment: 261400 Committed r189931: <http://trac.webkit.org/changeset/189931>
All reviewed patches have been landed. Closing bug.