Bug 92374 – Inaccurate layoutMod

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Allan Sandfeld Jensen]

The layoutMod function performs the module in integer instead of FractionalLayoutUnit when subpixel units are enabled. This means the it for its main use; calculating remainer, returns an inaccurate result.

To solve this, modulo operators should be defined for FractionalLayoutUnit and used. 

I here use the common definition of modulo which solves the equation:  a = (a / b) * b + a % b.

[Allan Sandfeld Jensen]

Created attachment 154628 [details]
Patch

[Emil A Eklund]

This is great, thank you.
Lets see what the cr-linux bot thinks as it runs all the tests. We might need to update some expectations.

[WebKit Review Bot]

Comment on attachment 154628 [details]
Patch

Attachment 154628 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13373018

New failing tests:
fast/multicol/float-paginate.html
fast/forms/range/slider-delete-while-dragging-thumb.html
fast/loader/loadInProgress.html
fast/multicol/float-paginate-complex.html
fast/multicol/float-multicol.html
fast/forms/search-vertical-alignment.html
fast/multicol/block-axis-horizontal-tb.html
fast/multicol/hit-test-end-of-column.html
http/tests/security/script-crossorigin-loads-correctly.html
fast/multicol/border-padding-pagination.html
fast/css/input-search-padding.html
fast/block/positioning/offsetLeft-offsetTop-multicolumn.html
fast/multicol/column-break-with-balancing.html
fast/forms/range/slider-mouse-events.html
fast/frames/cached-frame-counter.html
fast/multicol/block-axis-vertical-rl.html
fast/dom/Element/getBoundingClientRect.html
fast/multicol/column-rules-stacking.html
fast/multicol/hit-test-gap-block-axis.html
compositing/columns/composited-in-paginated.html
fast/multicol/image-inside-nested-blocks-with-border.html
fast/multicol/float-paginate-empty-lines.html
fast/multicol/column-rules.html
fast/multicol/block-axis-vertical-lr.html
fast/multicol/block-axis-horizontal-bt.html
fast/forms/range/slider-onchange-event.html
fast/multicol/break-properties.html

[WebKit Review Bot]

Created attachment 154761 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Allan Sandfeld Jensen]

Comment on attachment 154628 [details]
Patch

A few of the tests looks better, but several are obviously broken by this patch, which will require some investigation.

[Emil A Eklund]

Let me know if there is anything I can do to help.

[WebKit Review Bot]

Comment on attachment 154628 [details]
Patch

Attachment 154628 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13377024

New failing tests:
fast/multicol/float-paginate.html
fast/loader/loadInProgress.html
fast/multicol/float-paginate-complex.html
fast/multicol/margin-collapse.html
fast/multicol/hit-test-gap-block-axis.html
fast/forms/search-vertical-alignment.html
fast/multicol/block-axis-horizontal-tb.html
fast/multicol/hit-test-end-of-column.html
http/tests/security/script-crossorigin-loads-correctly.html
fast/multicol/border-padding-pagination.html
fast/css/input-search-padding.html
fast/multicol/layers-in-multicol.html
fast/block/positioning/offsetLeft-offsetTop-multicolumn.html
fast/multicol/column-break-with-balancing.html
fast/frames/cached-frame-counter.html
fast/multicol/nested-columns.html
fast/multicol/block-axis-vertical-rl.html
fast/dom/Element/getBoundingClientRect.html
fast/multicol/column-rules-stacking.html
compositing/columns/composited-in-paginated.html
fast/multicol/image-inside-nested-blocks-with-border.html
fast/multicol/float-paginate-empty-lines.html
fast/multicol/column-rules.html
fast/multicol/block-axis-vertical-lr.html
fast/multicol/float-multicol.html
fast/multicol/block-axis-horizontal-bt.html
fast/multicol/break-properties.html

[WebKit Review Bot]

Created attachment 154784 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Allan Sandfeld Jensen]

Okay, the problem is that layoutMod is used in some places to calculate the number of columns or pages. In those cases the fractional results makes no sense and the division and modulo should be performed in integer.

[Allan Sandfeld Jensen]

This really comes down to two different way to do module for FractionalLayoutUnit:

Imagine  kFixedPointDenominator = 10.

Is 5.5 % 6.0 = 5.5, because 0 * 6 + 5.5 = 5.5
or is 5.5 % 6.0 = 0.1, because 5.5 / 6.0 = 0.9 and 0.9 * 6 + 0.1 = 5.5

The second makes the most sense to me, but we need the first to calculate how much height is remaining after an unknown, but naturally integer, number of pages.

I will make a second function in the next patch to solve this called layoutIntMod.

[Allan Sandfeld Jensen]

Created attachment 154900 [details]
Patch

[Allan Sandfeld Jensen]

Btw, I considered another way of naming these two operations, so if anyone dislikes the naming used in the patch, I offer an alternative:

We could let operator% always be the remainder after a division truncated to integer, and call the real remainder after a fractionlayoutunit division for 'remainder'.

Or, we could not have any of them use operator%, to force programmers to actually think about which one they need, since we can't cover all common use-cases of integer-modulo in a single fractionallayoutunit modulo.

[WebKit Review Bot]

Comment on attachment 154900 [details]
Patch

Attachment 154900 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13378371

New failing tests:
fast/multicol/vertical-rl/float-multicol.html
fast/multicol/nested-columns.html
fast/multicol/vertical-lr/float-multicol.html
fast/multicol/vertical-rl/nested-columns.html
fast/speech/input-appearance-searchandspeech.html
fast/multicol/vertical-lr/nested-columns.html
fast/multicol/span/span-as-immediate-columns-child.html
fast/multicol/span/anonymous-split-block-crash.html
fast/forms/search-vertical-alignment.html
fast/css/input-search-padding.html
fast/table/colspanMinWidth-vertical.html
fast/multicol/span/clone-anonymous-block-non-inline-child-crash.html

[WebKit Review Bot]

Created attachment 154945 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Allan Sandfeld Jensen]

(In reply to comment #13)
> (From update of attachment 154900 [details])
> Attachment 154900 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/13378371
> 
> New failing tests:
> fast/multicol/vertical-rl/float-multicol.html
> fast/multicol/nested-columns.html
> fast/multicol/vertical-lr/float-multicol.html
> fast/multicol/vertical-rl/nested-columns.html
> fast/speech/input-appearance-searchandspeech.html
> fast/multicol/vertical-lr/nested-columns.html
> fast/multicol/span/span-as-immediate-columns-child.html
> fast/multicol/span/anonymous-split-block-crash.html
> fast/forms/search-vertical-alignment.html
> fast/css/input-search-padding.html
> fast/table/colspanMinWidth-vertical.html
> fast/multicol/span/clone-anonymous-block-non-inline-child-crash.html

These are all small 1px difference, which is too be expected when the precision of the layout is improved. None if them look wrong, though at 1px difference, they don't fix any obvious mistakes either.

[WebKit Review Bot]

Comment on attachment 154900 [details]
Patch

Attachment 154900 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13375522

New failing tests:
fast/multicol/vertical-rl/float-multicol.html
fast/multicol/nested-columns.html
fast/multicol/vertical-lr/float-multicol.html
fast/multicol/vertical-rl/nested-columns.html
fast/speech/input-appearance-searchandspeech.html
fast/multicol/vertical-lr/nested-columns.html
fast/multicol/span/span-as-immediate-columns-child.html
fast/multicol/span/anonymous-split-block-crash.html
fast/forms/search-vertical-alignment.html
fast/css/input-search-padding.html
fast/table/colspanMinWidth-vertical.html
fast/multicol/span/clone-anonymous-block-non-inline-child-crash.html

[WebKit Review Bot]

Created attachment 154964 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Allan Sandfeld Jensen]

I can not create new baselines for chromium, so I propose landing the patch as is, and let chromium developers rebase. Is that acceptable?

[Levi Weintraub]

(In reply to comment #18)
> I can not create new baselines for chromium, so I propose landing the patch as is, and let chromium developers rebase. Is that acceptable?

I'm not necessarily opposed to landing this without baselines, but I haven't had a chance to actually look at the diffs to verify they don't regress. If you're confident this doesn't regress, I can take your word.

Also, you don't need Chromium Dev's to rebase, the proper solution is to mark them all as expected failures in Chromium's test expectations, then use the Rebaseline Tool http://trac.webkit.org/wiki/Rebaseline. This shouldn't be landed in a way that makes the bots red.

[Allan Sandfeld Jensen]

(In reply to comment #19)
> (In reply to comment #18)
> > I can not create new baselines for chromium, so I propose landing the patch as is, and let chromium developers rebase. Is that acceptable?
> 
> I'm not necessarily opposed to landing this without baselines, but I haven't had a chance to actually look at the diffs to verify they don't regress. If you're confident this doesn't regress, I can take your word.
> 
I would feel more comfortable if someone else confirmed it. Especially someone with experience in looking at changes due to subpixel layout and chromium test results.

> Also, you don't need Chromium Dev's to rebase, the proper solution is to mark them all as expected failures in Chromium's test expectations, then use the Rebaseline Tool http://trac.webkit.org/wiki/Rebaseline. This shouldn't be landed in a way that makes the bots red.

That sounds nice. I will take a look.

[Allan Sandfeld Jensen]

(In reply to comment #20)
> (In reply to comment #19)
> > Also, you don't need Chromium Dev's to rebase, the proper solution is to mark them all as expected failures in Chromium's test expectations, then use the Rebaseline Tool http://trac.webkit.org/wiki/Rebaseline. This shouldn't be landed in a way that makes the bots red.
> 
> That sounds nice. I will take a look.

Or I could just use the output provided by the bots and put in the archives here, but I guess that would be too easy.

[Allan Sandfeld Jensen]

Created attachment 156061 [details]
Patch

[WebKit Review Bot]

Comment on attachment 156061 [details]
Patch

Attachment 156061 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13430049

New failing tests:
fast/multicol/vertical-rl/float-multicol.html
fast/multicol/nested-columns.html
fast/multicol/vertical-lr/float-multicol.html
fast/multicol/vertical-rl/nested-columns.html
fast/speech/input-appearance-searchandspeech.html
fast/multicol/vertical-lr/nested-columns.html
fast/multicol/span/span-as-immediate-columns-child.html
fast/multicol/span/anonymous-split-block-crash.html
fast/forms/search-vertical-alignment.html
fast/css/input-search-padding.html
fast/table/colspanMinWidth-vertical.html
fast/multicol/span/clone-anonymous-block-non-inline-child-crash.html

[WebKit Review Bot]

Created attachment 156121 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Allan Sandfeld Jensen]

Comment on attachment 156061 [details]
Patch

Wrong syntax in the testexpectations.

[WebKit Review Bot]

Comment on attachment 156061 [details]
Patch

Attachment 156061 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13426268

New failing tests:
fast/multicol/vertical-rl/float-multicol.html
fast/multicol/nested-columns.html
fast/multicol/vertical-lr/float-multicol.html
fast/multicol/vertical-rl/nested-columns.html
fast/speech/input-appearance-searchandspeech.html
fast/multicol/vertical-lr/nested-columns.html
fast/multicol/span/span-as-immediate-columns-child.html
fast/multicol/span/anonymous-split-block-crash.html
fast/forms/search-vertical-alignment.html
fast/css/input-search-padding.html
fast/table/colspanMinWidth-vertical.html
fast/multicol/span/clone-anonymous-block-non-inline-child-crash.html

[WebKit Review Bot]

Created attachment 156133 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Allan Sandfeld Jensen]

Created attachment 156310 [details]
Patch

Fixed TestExpectation syntax

[Levi Weintraub]

Comment on attachment 156310 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156310&action=review

Overall, great change!

> Source/WebCore/rendering/LayoutTypes.h:150
>  inline LayoutUnit layoutMod(const LayoutUnit& numerator, const LayoutUnit& denominator)

So is the non-integer layoutMod used anywhere now?

> Source/WebCore/rendering/LayoutTypes.h:156
> +inline LayoutUnit layoutIntMod(const LayoutUnit& numerator, const LayoutUnit& denominator)

I think this level of indirection should just go away and callers should call directly into FractionalLayoutUnit's functions. LayoutTypes was created largely to ease the transition to FractionalLayoutUnits, which has occurred.

[Allan Sandfeld Jensen]

(In reply to comment #29)
> (From update of attachment 156310 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156310&action=review
> 
> Overall, great change!
> 
> > Source/WebCore/rendering/LayoutTypes.h:150
> >  inline LayoutUnit layoutMod(const LayoutUnit& numerator, const LayoutUnit& denominator)
> 
> So is the non-integer layoutMod used anywhere now?
> 
Yes, I think I left two or three places using layoutMod unchanged. 

> > Source/WebCore/rendering/LayoutTypes.h:156
> > +inline LayoutUnit layoutIntMod(const LayoutUnit& numerator, const LayoutUnit& denominator)
> 
> I think this level of indirection should just go away and callers should call directly into FractionalLayoutUnit's functions. LayoutTypes was created largely to ease the transition to FractionalLayoutUnits, which has occurred.

I thought that would be done when most platforms had switched subpixel layout, and  FractionLayoutX could be renamed LayoutX?

[Levi Weintraub]

Comment on attachment 156310 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156310&action=review

>>> Source/WebCore/rendering/LayoutTypes.h:156
>>> +inline LayoutUnit layoutIntMod(const LayoutUnit& numerator, const LayoutUnit& denominator)
>> 
>> I think this level of indirection should just go away and callers should call directly into FractionalLayoutUnit's functions. LayoutTypes was created largely to ease the transition to FractionalLayoutUnits, which has occurred.
> 
> I thought that would be done when most platforms had switched subpixel layout, and  FractionLayoutX could be renamed LayoutX?

We would like to do this rename, but we should minimize the duplication of functionality in LayoutTypes.h now that all platforms are using FractionalLayoutUnits. In the meantime, this isn't a gating issue. r=me pending results from the cr-linux bot, and provided you follow up with updated expectations once the bots have cycled with the patch. Let me know if you need help with that last part :)

[Allan Sandfeld Jensen]

(In reply to comment #31)
> We would like to do this rename, but we should minimize the duplication of functionality in LayoutTypes.h now that all platforms are using FractionalLayoutUnits. In the meantime, this isn't a gating issue. r=me pending results from the cr-linux bot

The cr-linux bot was shortly green and then seemed to die for reason. I will assume the fact that it ran one iteration completely green that it actually work, so I will land the patch and watch the waterfall.

[Allan Sandfeld Jensen]

Committed r124745: <http://trac.webkit.org/changeset/124745>

[WebKit Review Bot]

Re-opened since this is blocked by 93249

[Allan Sandfeld Jensen]

Bug #93249 has been closed again.

[Allan Sandfeld Jensen]

Rebaseline of skipped tests landed in r124874