Bug 136419 – Remove integral snapping functions from InlineBox class.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[alan]

There's no need to do any integral snapping on InlineBox.

[alan]

Created attachment 237431 [details]
Patch

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[alan]

Created attachment 237434 [details]
Patch

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Sam Weinig]

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.

[alan]

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

[Myles C. Maxfield]

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

[alan]

Created attachment 237520 [details]
Patch

[WebKit Commit Bot]

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.

[alan]

Created attachment 237521 [details]
Patch

[WebKit Commit Bot]

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.

[alan]

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

[Darin Adler]

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?

[alan]

Created attachment 237623 [details]
Patch

[alan]

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

[Brent Fulgham]

Comment on attachment 237623 [details]
Patch

Nice work! r=me.

[alan]

Created attachment 261395 [details]
Patch

[alan]

Created attachment 261400 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 261400 [details]
Patch

Clearing flags on attachment: 261400

Committed r189931: <http://trac.webkit.org/changeset/189931>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.