Issue 764503002: Make fast/multicol/break-properties independent of the size of <body>.

Issue 764503002: Make fast/multicol/break-properties independent of the size of <body>. (Closed)

Created:
6 years ago by andersr

Modified:
6 years ago

Reviewers:
mstensho (USE GERRIT)

CC:
blink-reviews

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Project:
blink

Visibility:
Public.

More Reviews

Description

Make fast/multicol/break-properties independent of the size of <body>. This test currently relies on our special behavior when <html> and <body> explicitly disagree on 'direction' or 'writing-mode'. I wish to change that behavior, so I removed that dependency here by checking which column the boxes are placed in rather than its exact position in two dimensions. To simplify calculations, the body margin is now equal to the column-gap. Also, since it has no effect on the test, the multicol container is now much smaller to avoid overflow in the vertical-lr/rl versions of the test. R=mstensho@opera.com Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186112

Patch Set 1 #

Total comments: 8

Patch Set 2 : Count from zero, padding:8px, remove <body>. #

Created: 6 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+48 lines, -37 lines)			Patch
M	LayoutTests/fast/multicol/break-properties.html	View	1	2 chunks	+13 lines, -9 lines	0 comments	Download
M	LayoutTests/fast/multicol/break-properties-expected.txt	View	1	1 chunk	+3 lines, -3 lines	0 comments	Download
M	LayoutTests/fast/multicol/vertical-lr/break-properties.html	View	1	2 chunks	+13 lines, -9 lines	0 comments	Download
M	LayoutTests/fast/multicol/vertical-lr/break-properties-expected.txt	View	1	1 chunk	+3 lines, -2 lines	0 comments	Download
M	LayoutTests/fast/multicol/vertical-rl/break-properties.html	View	1	2 chunks	+13 lines, -12 lines	0 comments	Download
M	LayoutTests/fast/multicol/vertical-rl/break-properties-expected.txt	View	1	1 chunk	+3 lines, -2 lines	0 comments	Download

Messages

Total messages: 6 (1 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

mstensho (USE GERRIT)

lgtm https://codereview.chromium.org/764503002/diff/1/LayoutTests/fast/multicol/break-properties.html File LayoutTests/fast/multicol/break-properties.html (right): https://codereview.chromium.org/764503002/diff/1/LayoutTests/fast/multicol/break-properties.html#newcode3 LayoutTests/fast/multicol/break-properties.html:3: body { margin: 5px; } Being explicit about ...

6 years ago (2014-11-27 10:37:54 UTC) #2

andersr

https://codereview.chromium.org/764503002/diff/1/LayoutTests/fast/multicol/break-properties.html File LayoutTests/fast/multicol/break-properties.html (right): https://codereview.chromium.org/764503002/diff/1/LayoutTests/fast/multicol/break-properties.html#newcode3 LayoutTests/fast/multicol/break-properties.html:3: body { margin: 5px; } On 2014/11/27 10:37:53, mstensho ...

6 years ago (2014-11-27 11:03:03 UTC) #3

https://codereview.chromium.org/764503002/diff/1/LayoutTests/fast/multicol/br...
File LayoutTests/fast/multicol/break-properties.html (right):

https://codereview.chromium.org/764503002/diff/1/LayoutTests/fast/multicol/br...
LayoutTests/fast/multicol/break-properties.html:3: body { margin: 5px; }
On 2014/11/27 10:37:53, mstensho wrote:
> Being explicit about the body margin is good, so that we won't have to make
> assumptions about it, but it could be left at 8px. I find setting it to 5px to
> simplify the calculations below confusing.

Done.

https://codereview.chromium.org/764503002/diff/1/LayoutTests/fast/multicol/br...
LayoutTests/fast/multicol/break-properties.html:26: function testBoxColumn(id,
expectedColumn) {
On 2014/11/27 10:37:53, mstensho wrote:
> Can you start counting from 0 instead of 1? That's what we normally do in the
> code (the spec doesn't care).

Done.

https://codereview.chromium.org/764503002/diff/1/LayoutTests/fast/multicol/br...
LayoutTests/fast/multicol/break-properties.html:29: var expectedPosition =
(expectedColumn - 1) * 50 + expectedColumn * 5;
On 2014/11/27 10:37:53, mstensho wrote:
> Could be "8 + 55 * expectedColumn" (if you start numbering columns from 0 (and
> set body margin to 8px)). Or "8 + (50 + 5) * expectedColumn" if you want to be
> more explicit about it.

Done. (Went for the explicit version).

https://codereview.chromium.org/764503002/diff/1/LayoutTests/fast/multicol/ve...
File LayoutTests/fast/multicol/vertical-lr/break-properties.html (right):

https://codereview.chromium.org/764503002/diff/1/LayoutTests/fast/multicol/ve...
LayoutTests/fast/multicol/vertical-lr/break-properties.html:3: body { margin:
5px; }
On 2014/11/27 10:37:54, mstensho wrote:
> Either remove this selector and move the declaration to the BODY style
> attribute, or move -webkit-writing-mode:vertical-lr here and remove the BODY
> tag.
> 
> Likewise for the other tests.

Done. Removed the tags.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/764503002/20001

6 years ago (2014-11-27 11:04:31 UTC) #5

commit-bot: I haz the power

6 years ago (2014-11-27 12:13:44 UTC) #6

Message was sent while issue was closed.

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186112

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages