Bug 136419 - Remove integral snapping functions from InlineBox class.
Summary: Remove integral snapping functions from InlineBox class.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-08-31 11:23 PDT by zalan
Modified: 2015-09-17 14:22 PDT (History)
8 users (show)

See Also:


Attachments
Patch (25.36 KB, patch)
2014-08-31 11:32 PDT, zalan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (986.84 KB, application/zip)
2014-08-31 13:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (1012.79 KB, application/zip)
2014-08-31 14:04 PDT, Build Bot
no flags Details
Patch (44.05 KB, patch)
2014-08-31 14:30 PDT, zalan
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (502.51 KB, application/zip)
2014-08-31 16:03 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (507.18 KB, application/zip)
2014-08-31 17:05 PDT, Build Bot
no flags Details
Patch (45.00 KB, patch)
2014-09-02 15:27 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (44.98 KB, patch)
2014-09-02 15:34 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (39.78 KB, patch)
2014-09-04 08:02 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (8.67 KB, patch)
2015-09-17 10:15 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (8.73 KB, patch)
2015-09-17 10:56 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2014-08-31 11:23:54 PDT
There's no need to do any integral snapping on InlineBox.
Comment 1 zalan 2014-08-31 11:32:32 PDT
Created attachment 237431 [details]
Patch
Comment 2 WebKit Commit Bot 2014-08-31 11:35:13 PDT
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 3 Build Bot 2014-08-31 13:41:35 PDT
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
Comment 4 Build Bot 2014-08-31 13:41:39 PDT
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 5 Build Bot 2014-08-31 14:04:20 PDT
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
Comment 6 Build Bot 2014-08-31 14:04:24 PDT
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
Comment 7 zalan 2014-08-31 14:30:47 PDT
Created attachment 237434 [details]
Patch
Comment 8 WebKit Commit Bot 2014-08-31 14:32:34 PDT
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 9 Build Bot 2014-08-31 16:03:49 PDT
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
Comment 10 Build Bot 2014-08-31 16:03:54 PDT
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 11 Build Bot 2014-08-31 17:05:33 PDT
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
Comment 12 Build Bot 2014-08-31 17:05:39 PDT
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 13 Sam Weinig 2014-08-31 20:59:54 PDT
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.
Comment 14 zalan 2014-08-31 21:01:19 PDT
(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 15 Myles C. Maxfield 2014-09-02 11:17:41 PDT
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
Comment 16 zalan 2014-09-02 15:27:49 PDT
Created attachment 237520 [details]
Patch
Comment 17 WebKit Commit Bot 2014-09-02 15:29:50 PDT
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.
Comment 18 zalan 2014-09-02 15:34:26 PDT
Created attachment 237521 [details]
Patch
Comment 19 WebKit Commit Bot 2014-09-02 15:36:51 PDT
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.
Comment 20 zalan 2014-09-02 20:17:10 PDT
(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 21 Darin Adler 2014-09-04 01:35:04 PDT
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?
Comment 22 zalan 2014-09-04 08:02:44 PDT
Created attachment 237623 [details]
Patch
Comment 23 zalan 2014-09-04 08:29:47 PDT
(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 24 Brent Fulgham 2014-10-31 11:10:23 PDT
Comment on attachment 237623 [details]
Patch

Nice work! r=me.
Comment 25 zalan 2015-09-17 10:15:31 PDT
Created attachment 261395 [details]
Patch
Comment 26 zalan 2015-09-17 10:56:40 PDT
Created attachment 261400 [details]
Patch
Comment 27 WebKit Commit Bot 2015-09-17 14:22:21 PDT
Comment on attachment 261400 [details]
Patch

Clearing flags on attachment: 261400

Committed r189931: <http://trac.webkit.org/changeset/189931>
Comment 28 WebKit Commit Bot 2015-09-17 14:22:27 PDT
All reviewed patches have been landed.  Closing bug.