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.
Created attachment 154628 [details] Patch
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.
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
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
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.
Let me know if there is anything I can do to help.
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
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
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.
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.
Created attachment 154900 [details] Patch
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.
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
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
(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.
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
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
I can not create new baselines for chromium, so I propose landing the patch as is, and let chromium developers rebase. Is that acceptable?
(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.
(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.
(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.
Created attachment 156061 [details] Patch
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
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
Comment on attachment 156061 [details] Patch Wrong syntax in the testexpectations.
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
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
Created attachment 156310 [details] Patch Fixed TestExpectation syntax
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.
(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?
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 :)
(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.
Committed r124745: <http://trac.webkit.org/changeset/124745>
Re-opened since this is blocked by 93249
Bug #93249 has been closed again.
Rebaseline of skipped tests landed in r124874