Implement sequential fallback to float speculation #13401

notriddle · 2016-09-23T20:05:58Z

This shouldn't impact any pages that are already rendering correctly, but it is a very naive implementation of this pass.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #13284 and fix #13223
There are tests for these changes

This change is

jdm · 2016-09-23T20:40:21Z

@bors-servo: try

bors-servo · 2016-09-23T20:40:24Z

⌛ Trying commit 681a08c with merge 101f1cb...


        Auto merge of #13401 - notriddle:master, r=<try>

Implement sequential fallback to float speculation This shouldn't impact any pages that are already rendering correctly, but it is a very naive implementation of this pass. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13284 and fix #13223 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13401)

larsbergstrom · 2016-09-23T20:58:26Z

r? @pcwalton

bors-servo · 2016-09-23T22:30:54Z

💔 Test failed - mac-rel-css

highfive · 2016-09-23T22:30:56Z

  ▶ PASS [expected FAIL] /css21_dev/html4/block-formatting-contexts-015.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/floats-bfc-002.htm

  ▶ FAIL [expected PASS] /css21_dev/html4/floats-wrap-bfc-001-left-overflow.htm
  └   → /css21_dev/html4/floats-wrap-bfc-001-left-overflow.htm 2d6a5fdccd9eefb689e8f881df829f743f0e3de4
/css21_dev/html4/reference/floats-wrap-bfc-001-left-overflow-ref.htm c3a278115ff68702eec6eef472830b73b56c5ca4
Testing 2d6a5fdccd9eefb689e8f881df829f743f0e3de4 == c3a278115ff68702eec6eef472830b73b56c5ca4

  ▶ FAIL [expected PASS] /css21_dev/html4/floats-wrap-bfc-002-left-overflow.htm
  └   → /css21_dev/html4/floats-wrap-bfc-002-left-overflow.htm a4bf58f4ae043db01bec5e0d88db4c96fdf0d552
/css21_dev/html4/reference/floats-wrap-bfc-002-left-ref.htm 71b5a9a854228c0d9f4f458558ca9fffb6fc3591
Testing a4bf58f4ae043db01bec5e0d88db4c96fdf0d552 == 71b5a9a854228c0d9f4f458558ca9fffb6fc3591

  ▶ FAIL [expected PASS] /css21_dev/html4/floats-wrap-bfc-003-left-overflow.htm
  └   → /css21_dev/html4/floats-wrap-bfc-003-left-overflow.htm b1fae663feb9fa39bceddc374c3883c5496d2028
/css21_dev/html4/reference/floats-wrap-bfc-003-left-overflow-ref.htm 43f657c70beb6673a6f90ff025c7f06c55e5e20a
Testing b1fae663feb9fa39bceddc374c3883c5496d2028 == 43f657c70beb6673a6f90ff025c7f06c55e5e20a

notriddle · 2016-09-23T23:17:28Z

Let's try that again.

@bors-servo try

bors-servo · 2016-09-23T23:17:30Z

⌛ Trying commit 95f59b5 with merge 336d8dc...


        Auto merge of #13401 - notriddle:master, r=<try>

Implement sequential fallback to float speculation This shouldn't impact any pages that are already rendering correctly, but it is a very naive implementation of this pass. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13284 and fix #13223 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13401)

bors-servo · 2016-09-24T00:09:08Z

💔 Test failed - linux-rel

highfive · 2016-09-24T00:09:11Z

  ▶ FAIL [expected PASS] /_mozilla/css/block_formatting_context_max_width_a.html
  └   → /_mozilla/css/block_formatting_context_max_width_a.html 25eebe380e566e884c3c6ff5ba455c23cf47f934
/_mozilla/css/block_formatting_context_max_width_ref.html 175e96def9539286d756e52a7fef8899877b31e9
Testing 25eebe380e566e884c3c6ff5ba455c23cf47f934 == 175e96def9539286d756e52a7fef8899877b31e9

  ▶ FAIL [expected PASS] /_mozilla/css/block_formatting_context_margin_inout_a.html
  └   → /_mozilla/css/block_formatting_context_margin_inout_a.html 2aee0325975fc9a281e1a63c5e93799687c51c49
/_mozilla/css/block_formatting_context_margin_inout_ref.html 5cc93120b21f24648fd136070ab186250570646f
Testing 2aee0325975fc9a281e1a63c5e93799687c51c49 == 5cc93120b21f24648fd136070ab186250570646f

  ▶ FAIL [expected PASS] /_mozilla/css/block_formatting_context_negative_margins_a.html
  └   → /_mozilla/css/block_formatting_context_negative_margins_a.html cfe4660a21d266bc636625877f5befe2893a00b8
/_mozilla/css/block_formatting_context_negative_margins_ref.html c925eae8f74a6adf7bbd32e458d47e6dec397983
Testing cfe4660a21d266bc636625877f5befe2893a00b8 == c925eae8f74a6adf7bbd32e458d47e6dec397983

notriddle · 2016-09-24T02:10:12Z

Those silly margins

@bors-servo try

bors-servo · 2016-09-24T02:10:15Z

⌛ Trying commit 06978f6 with merge 91e771b...


        Auto merge of #13401 - notriddle:master, r=<try>

Implement sequential fallback to float speculation This shouldn't impact any pages that are already rendering correctly, but it is a very naive implementation of this pass. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13284 and fix #13223 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13401)

bors-servo · 2016-09-24T02:20:04Z

💔 Test failed - linux-rel

notriddle · 2016-09-24T02:35:31Z

The annoying git.

@bors-servo retry

bors-servo · 2016-09-24T02:36:15Z

⌛ Trying commit 06978f6 with merge a367404...


        Auto merge of #13401 - notriddle:master, r=<try>

Implement sequential fallback to float speculation This shouldn't impact any pages that are already rendering correctly, but it is a very naive implementation of this pass. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13284 and fix #13223 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13401)

pcwalton

Nice to see that this is so straightforward to implement!

pcwalton · 2016-09-24T03:45:07Z

components/layout/block.rs

+            self.base.block_container_inline_size - self.fragment.margin.inline_start_end(),
+        );
+        match rect {
+            Some(rect) => {


nit: use if let Some(rect) = rect { to avoid a level of indentation

pcwalton · 2016-09-24T03:45:07Z

components/layout/block.rs

+                    min_inline_size
+                } else {
+                    rect.size.inline
+                };


Do we have a clamp function for this? Should we make one in util?

pcwalton · 2016-09-24T03:45:07Z

components/layout/block.rs

+            Some(rect) => {
+                self.base.position.start.i = max(
+                    rect.start.i,
+                    self.fragment.margin.inline_start,


nit: either put this comma in on all of the function calls or remove it from all of them. Style in this file usually has the comma removed, but it's up to you.

Now that I look at this again, it's wrong anyway. My base position should not be offset by the inline_start, because it includes inline_start. Position includes the margins in the inline direction.

pcwalton · 2016-09-24T03:45:07Z

components/layout/block.rs

-        // TODO(pcwalton): If the inline-size of this flow is different from the size we estimated
-        // earlier, lay it out again.
+        // If float speculation failed, fixup our layout, and re-layout all the children.
+        if self.fragment.margin_box_inline_size() != self.base.position.size.inline {


Could you add a debug!() log message here? For performance reasons, it's nice to know when we hit this case.

pcwalton · 2016-09-24T03:45:07Z

components/layout/block.rs


-        self.base.restyle_damage.remove(REFLOW_OUT_OF_FLOW | REFLOW);
-        self.fragment.restyle_damage.remove(REFLOW_OUT_OF_FLOW | REFLOW);
+        assert_eq!(self.fragment.margin_box_inline_size(), self.base.position.size.inline);


Use debug_assert!

bors-servo · 2016-09-24T03:47:36Z

💔 Test failed - mac-rel-wpt

bors-servo · 2016-09-28T03:41:32Z

💔 Test failed - mac-dev-unit

KiChjang · 2016-09-28T04:00:34Z

@bors-servo retry

notriddle · 2016-09-29T05:21:41Z

@bors-servo retry


        Auto merge of #13401 - notriddle:master, r=pcwalton

Implement sequential fallback to float speculation This shouldn't impact any pages that are already rendering correctly, but it is a very naive implementation of this pass. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13284 and fix #13223 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13401)

bors-servo · 2016-09-29T12:19:25Z

⌛ Testing commit bf69b76 with merge 5a64f3e...

bors-servo · 2016-09-29T12:23:03Z

💔 Test failed - linux-dev

Ms2ger · 2016-09-29T12:25:11Z

./components/util/lib.rs:49: tab on line
./components/util/lib.rs:50: tab on line
./components/util/lib.rs:51: tab on line
./components/util/lib.rs:52: tab on line
./components/util/lib.rs:53: tab on line
./components/util/lib.rs:54: tab on line
./components/util/lib.rs:55: tab on line
./components/util/lib.rs:56: no newline at EOF


        Add assertion that this is not a flex item

bors-servo · 2016-09-29T17:30:13Z

📌 Commit 81676ea has been approved by pcwalton

bors-servo · 2016-09-29T18:16:25Z

⌛ Testing commit 81676ea with merge 4ebecc9...


        Auto merge of #13401 - notriddle:master, r=pcwalton

Implement sequential fallback to float speculation This shouldn't impact any pages that are already rendering correctly, but it is a very naive implementation of this pass. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13284 and fix #13223 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13401)

bors-servo · 2016-09-29T19:28:51Z

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

highfive assigned larsbergstrom Sep 23, 2016

highfive added the S-awaiting-review label Sep 23, 2016

highfive assigned pcwalton and unassigned larsbergstrom Sep 23, 2016

highfive added the S-tests-failed label Sep 23, 2016

highfive removed the S-tests-failed label Sep 23, 2016

highfive added the S-tests-failed label Sep 24, 2016

highfive removed the S-tests-failed label Sep 24, 2016

highfive added the S-tests-failed label Sep 24, 2016

pcwalton reviewed Sep 24, 2016

View changes

highfive added the S-tests-failed label Sep 24, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 28, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Sep 29, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 29, 2016

highfive added S-awaiting-review S-awaiting-merge and removed S-tests-failed S-awaiting-review labels Sep 29, 2016

Add assertion that this is not a flex item

Loading status checks…

81676ea

highfive added S-awaiting-review and removed S-awaiting-merge labels Sep 29, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 29, 2016

bors-servo merged commit 81676ea into servo:master Sep 29, 2016
2 of 3 checks passed

2 of 3 checks passed

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

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Sep 29, 2016

notriddle mentioned this pull request Oct 10, 2016

Broken layout on Waffle's page #13622

Closed

stshine mentioned this pull request Nov 8, 2016

Implement column flexbox #14123

Open

servo / servo

Implement sequential fallback to float speculation #13401

Implement sequential fallback to float speculation #13401

notriddle commented Sep 23, 2016 •

edited by larsbergstrom

jdm commented Sep 23, 2016

bors-servo commented Sep 23, 2016

larsbergstrom commented Sep 23, 2016

bors-servo commented Sep 23, 2016

highfive commented Sep 23, 2016

notriddle commented Sep 23, 2016

bors-servo commented Sep 23, 2016

bors-servo commented Sep 24, 2016

highfive commented Sep 24, 2016

notriddle commented Sep 24, 2016 •

edited

bors-servo commented Sep 24, 2016

bors-servo commented Sep 24, 2016

notriddle commented Sep 24, 2016 •

edited

bors-servo commented Sep 24, 2016

pcwalton left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Sep 24, 2016

bors-servo commented Sep 28, 2016

KiChjang commented Sep 28, 2016 •

edited by notriddle

notriddle commented Sep 29, 2016

bors-servo commented Sep 29, 2016

bors-servo commented Sep 29, 2016

Ms2ger commented Sep 29, 2016

bors-servo commented Sep 29, 2016

bors-servo commented Sep 29, 2016

bors-servo commented Sep 29, 2016

servo / servo

Join GitHub today

Implement sequential fallback to float speculation #13401

Implement sequential fallback to float speculation #13401

Conversation

notriddle commented Sep 23, 2016 • edited by larsbergstrom

jdm commented Sep 23, 2016

bors-servo commented Sep 23, 2016

larsbergstrom commented Sep 23, 2016

bors-servo commented Sep 23, 2016

highfive commented Sep 23, 2016

notriddle commented Sep 23, 2016

bors-servo commented Sep 23, 2016

bors-servo commented Sep 24, 2016

highfive commented Sep 24, 2016

notriddle commented Sep 24, 2016 • edited

bors-servo commented Sep 24, 2016

bors-servo commented Sep 24, 2016

notriddle commented Sep 24, 2016 • edited

bors-servo commented Sep 24, 2016

pcwalton left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Sep 24, 2016

bors-servo commented Sep 28, 2016

KiChjang commented Sep 28, 2016 • edited by notriddle

notriddle commented Sep 29, 2016

bors-servo commented Sep 29, 2016

bors-servo commented Sep 29, 2016

Ms2ger commented Sep 29, 2016

bors-servo commented Sep 29, 2016

bors-servo commented Sep 29, 2016

bors-servo commented Sep 29, 2016

notriddle commented Sep 23, 2016 •

edited by larsbergstrom

notriddle commented Sep 24, 2016 •

edited

notriddle commented Sep 24, 2016 •

edited

KiChjang commented Sep 28, 2016 •

edited by notriddle