Bug #10181 - Implement *-reverse flex-directions #10987

shinglyu · 2016-05-03T04:20:44Z

This change is

highfive · 2016-05-03T04:20:47Z

Warning

These commits modify layout code, but no tests are modified.Please consider adding a test!

shinglyu · 2016-05-03T10:02:08Z

I ran the test

./mach test-css tests/wpt/css-tests/css-flexbox-1_dev/html/

but got a LOT of timeouts

Ran 612 tests finished in 2425.0 seconds.
  • 78 ran as expected. 0 tests skipped.
  • 534 tests timed out unexpectedly

dlrobertson · 2016-05-03T13:53:29Z

I get the following on my server with no timeouts or failures. I get a lot of timeouts when I run test-css on my laptop though.

 ▶ PASS [expected FAIL] /css-flexbox-1_dev/html/order-with-column-reverse.htm

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.

components/layout/flex.rs, line 120 [r1] (raw file):
Would this be more readable if this were collapsed into one match statement?

let (main_mode, is_reverse) = match fragment.style.get_position().flex_direction {
    flex_direction::T::row => (Mode::Inline, false),
    flex_direction::T::row_reverse => (Mode::Inline, true),
    ...

Just a thought...

Comments from Reviewable

jdm · 2016-05-03T15:50:50Z

@shinglyu Are you using a debug or release build? Are there still timeouts if you run with test-css --release?

shinglyu · 2016-05-04T02:26:56Z

I didn't specify the build, so I guess it uses the debug build. I'll try it on release build on a more powerful machine.

shinglyu · 2016-05-04T03:44:01Z

Running 612 tests in web-platform-tests

▶ PASS [expected FAIL] /css-flexbox-1_dev/html/flex-flexitem-percentage-prescation.htm

▶ PASS [expected FAIL] /css-flexbox-1_dev/html/order-with-column-reverse.htm

Unsupported test type wdspec for product servo
Ran 612 tests finished in 96.0 seconds.
• 610 ran as expected. 0 tests skipped.
• 2 tests passed unexpectedly

Let me check those two unexpected passing tests.

dlrobertson · 2016-05-04T15:27:42Z

▶ PASS [expected FAIL] /css-flexbox-1_dev/html/order-with-column-reverse.htm

Is definitely expected given the contents of this PR.

▶ PASS [expected FAIL] /css-flexbox-1_dev/html/flex-flexitem-percentage-prescation.htm

Not sure what is going on here. I don't get this on my box.

shinglyu · 2016-05-05T06:14:32Z

flex-flexitem-percentage-prescation.htm on master branch build looks good (with --pref layout.flex.enabled --pref layout.flex-direction.enabled). I'm not sure why it's not passing before. I'll mark the two as passing.

Also I think the order-with-row-reverse.htm should pass too? I'll check it.

highfive · 2016-05-05T06:40:06Z

New code was committed to pull request.

larsbergstrom · 2016-05-10T00:38:23Z

r? @mbrubeck

mbrubeck · 2016-05-10T00:52:36Z

@bors-servo delegate+

r=mbrubeck with one review comment addressed (below).

Previously, larsbergstrom (Lars Bergstrom) wrote…

r? @mbrubeck

Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

components/layout/flex.rs, line 284 [r1] (raw file):

                inline_child_start = inline_child_start - even_content_inline_size;
            }
        }

It would be nice if the if and else cases shared more code. Maybe add aa direction variable that is either 1 or -1, and initialize it along with inline_child_start. Then you could have a single loop that ends with inline_child_start = inline_child_start + even_content_inline_size * direction;

Comments from Reviewable

bors-servo · 2016-05-10T00:52:37Z

✌️ @shinglyu can now approve this pull request

highfive · 2016-05-10T08:46:30Z

New code was committed to pull request.

shinglyu · 2016-05-10T08:49:24Z

Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.

components/layout/flex.rs, line 284 [r1] (raw file):

Previously, mbrubeck (Matt Brubeck) wrote…

It would be nice if the if and else cases shared more code. Maybe add aa direction variable that is either 1 or -1, and initialize it along with inline_child_start. Then you could have a single loop that ends with inline_child_start = inline_child_start + even_content_inline_size * direction;

I end up using an if...else in the for loop, the base.position.start.i line also needs to be handled differently, so this willl make me use two flags, which I believe is dirty.

Comments from Reviewable

mbrubeck · 2016-05-10T15:44:36Z

@bors-servo r+

Previously, highfive wrote…

New code was committed to pull request.

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

bors-servo · 2016-05-10T15:44:37Z

📌 Commit 26f9b08 has been approved by mbrubeck

bors-servo · 2016-05-10T16:29:52Z

⌛ Testing commit 26f9b08 with merge 37ae863...


        Auto merge of #10987 - shinglyu:flex-reverse, r=mbrubeck

Bug #10181 - Implement *-reverse flex-directions  This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10987)

bors-servo · 2016-05-10T17:19:28Z

💔 Test failed - linux-rel

highfive · 2016-05-10T17:19:30Z

  ▶ FAIL [expected PASS] /css-flexbox-1_dev/html/flex-flexitem-percentage-prescation.htm
  └   → /css-flexbox-1_dev/html/flex-flexitem-percentage-prescation.htm 549dc2995e7de786feb6065fc8fc76b53746d920
/css-flexbox-1_dev/html/reference/flex-flexitem-percentage-prescation-ref.htm 516cbcf7b447cf7d5a568a4f8b57f030555e6525
Testing 549dc2995e7de786feb6065fc8fc76b53746d920 == 516cbcf7b447cf7d5a568a4f8b57f030555e6525

dlrobertson · 2016-05-10T17:47:37Z

I get a failure on my box too. I doubt this PR should affect this test... Did it change the outcome of this test on your machine?

Previously, highfive wrote…

  ▶ FAIL [expected PASS] /css-flexbox-1_dev/html/flex-flexitem-percentage-prescation.htm
  └   → /css-flexbox-1_dev/html/flex-flexitem-percentage-prescation.htm 549dc2995e7de786feb6065fc8fc76b53746d920
/css-flexbox-1_dev/html/reference/flex-flexitem-percentage-prescation-ref.htm 516cbcf7b447cf7d5a568a4f8b57f030555e6525
Testing 549dc2995e7de786feb6065fc8fc76b53746d920 == 516cbcf7b447cf7d5a568a4f8b57f030555e6525
</details>

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

mbrubeck · 2016-05-11T00:50:25Z

That test is currently marked as failing on Linux only, but I have gotten unexpected PASS results on my Linux box. It might be font-related.

shinglyu · 2016-05-11T02:21:38Z

This is indeed a font problem. I opened it in Reftest analyzer and the font looks different from my Linux Mint machine. There is a subtle font aliasing effect between the "r" and "b".

shinglyu · 2016-05-11T02:23:56Z

The test is relatively simple, it should have passed even without this patch. I will mark it as conditionally failing on Linux as before, and file a separate issue for it.


        Bug #10181 - Implement *-reverse flex-directions

highfive · 2016-05-11T02:30:42Z

New code was committed to pull request.

shinglyu · 2016-05-11T02:31:16Z

@bors-servo: r+

bors-servo · 2016-05-11T02:31:17Z

📌 Commit 89586ed has been approved by shinglyu

shinglyu · 2016-05-11T02:32:49Z

@mbrubeck I'm r+ing myself since I am only disabled the failing test. Hope you don't mind.

bors-servo · 2016-05-11T02:51:05Z

⌛ Testing commit 89586ed with merge 20f0be2...


        Auto merge of #10987 - shinglyu:flex-reverse, r=shinglyu

Bug #10181 - Implement *-reverse flex-directions  This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10987)

bors-servo · 2016-05-11T04:43:22Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

highfive assigned pcwalton May 3, 2016

highfive added the S-awaiting-review label May 3, 2016

highfive assigned mbrubeck and unassigned pcwalton May 10, 2016

shinglyu force-pushed the shinglyu:flex-reverse branch from 787350a to 26f9b08 May 10, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels May 10, 2016

highfive removed the S-awaiting-merge label May 10, 2016

highfive added the S-tests-failed label May 10, 2016

Bug #10181 - Implement *-reverse flex-directions

Loading status checks…

89586ed

shinglyu force-pushed the shinglyu:flex-reverse branch from 26f9b08 to 89586ed May 11, 2016

highfive added S-awaiting-review and removed S-tests-failed labels May 11, 2016

highfive assigned shinglyu and unassigned mbrubeck May 11, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels May 11, 2016

bors-servo merged commit 89586ed into servo:master May 11, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label May 11, 2016

bors-servo mentioned this pull request May 11, 2016

Determine a flex-items used flex-basis #10945

Closed

timvandermeij mentioned this pull request May 11, 2016

Implement *-reverse flex-directions #10181

Closed

servo / servo

Bug #10181 - Implement *-reverse flex-directions #10987

Bug #10181 - Implement *-reverse flex-directions #10987

shinglyu commented May 3, 2016 •

edited by larsbergstrom

highfive commented May 3, 2016

shinglyu commented May 3, 2016

dlrobertson commented May 3, 2016

jdm commented May 3, 2016

shinglyu commented May 4, 2016

shinglyu commented May 4, 2016

dlrobertson commented May 4, 2016

shinglyu commented May 5, 2016

highfive commented May 5, 2016

larsbergstrom commented May 10, 2016

mbrubeck commented May 10, 2016

bors-servo commented May 10, 2016

highfive commented May 10, 2016

shinglyu commented May 10, 2016

mbrubeck commented May 10, 2016

bors-servo commented May 10, 2016

bors-servo commented May 10, 2016

bors-servo commented May 10, 2016

highfive commented May 10, 2016

dlrobertson commented May 10, 2016

mbrubeck commented May 11, 2016

shinglyu commented May 11, 2016

shinglyu commented May 11, 2016

highfive commented May 11, 2016

shinglyu commented May 11, 2016

bors-servo commented May 11, 2016

shinglyu commented May 11, 2016

bors-servo commented May 11, 2016

bors-servo commented May 11, 2016

servo / servo

Join GitHub today

Bug #10181 - Implement *-reverse flex-directions #10987

Bug #10181 - Implement *-reverse flex-directions #10987

Conversation

shinglyu commented May 3, 2016 • edited by larsbergstrom

highfive commented May 3, 2016

shinglyu commented May 3, 2016

dlrobertson commented May 3, 2016

jdm commented May 3, 2016

shinglyu commented May 4, 2016

shinglyu commented May 4, 2016

dlrobertson commented May 4, 2016

shinglyu commented May 5, 2016

highfive commented May 5, 2016

larsbergstrom commented May 10, 2016

mbrubeck commented May 10, 2016

bors-servo commented May 10, 2016

highfive commented May 10, 2016

shinglyu commented May 10, 2016

mbrubeck commented May 10, 2016

bors-servo commented May 10, 2016

bors-servo commented May 10, 2016

bors-servo commented May 10, 2016

highfive commented May 10, 2016

dlrobertson commented May 10, 2016

mbrubeck commented May 11, 2016

shinglyu commented May 11, 2016

shinglyu commented May 11, 2016

highfive commented May 11, 2016

shinglyu commented May 11, 2016

bors-servo commented May 11, 2016

shinglyu commented May 11, 2016

bors-servo commented May 11, 2016

bors-servo commented May 11, 2016

shinglyu commented May 3, 2016 •

edited by larsbergstrom