Convert column-width to use Either #14394

jcdyer · 2016-11-28T15:46:39Z

Converted column-width to use Either<Length, Auto>
Added gecko glue code
Cleaned up old column-width glue code

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14350 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because tests already surround the refactored code.

This change is

highfive · 2016-11-28T15:46:47Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @metajack (or someone else) soon.

highfive · 2016-11-28T15:46:48Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/gecko.mako.rs, components/style/values/mod.rs, components/style/values/computed/length.rs, components/style/properties/longhand/column.mako.rs, components/style/values/specified/length.rs, components/style/properties/properties.mako.rs, components/style/gecko/values.rs
@emilio: components/style/properties/gecko.mako.rs, components/style/values/mod.rs, components/style/values/computed/length.rs, components/style/properties/longhand/column.mako.rs, components/style/values/specified/length.rs, components/style/properties/properties.mako.rs, components/style/gecko/values.rs, components/layout/multicol.rs

jcdyer · 2016-11-28T17:15:19Z

I just rebased, and it looks like some method signatures changed. I'll give this another look soon.

jcdyer · 2016-11-29T01:17:39Z

I had a little trouble with the gecko glue, but I figured it out. I was originally referencing "LengthOrAuto" in the predefined types in the gecko.mako.rs template, but it should have been "length::LengthOrAuto".

wafflespeanut

Just a few changes before we land :)

wafflespeanut · 2016-11-29T03:14:17Z

components/style/properties/longhand/column.mako.rs

+    "Either::Second(Auto)",
+    parse_method="parse_non_negative_length",
+    products="gecko servo",
+    gecko_ffi_name="mColumnWidth",


We won't be needing products and gecko_ffi_name. The Mako helper will generate this.

wafflespeanut · 2016-11-29T03:14:17Z

components/style/values/specified/length.rs

@@ -953,6 +953,18 @@ impl Parse for LengthOrPercentageOrNone {

 pub type LengthOrNone = Either<Length, None_>;

+pub type LengthOrAuto = Either<Length, Auto>;
+
+impl LengthOrAuto {


This impl won't be necessary, I think, since we're going for the generic parse_non_negative_length introduced lately?

wafflespeanut · 2016-11-29T03:14:17Z

components/style/values/mod.rs

@@ -104,6 +104,19 @@ pub enum Either<A, B> {
    Second(B),
 }

+impl<A, B> Either<A, B> {
+    pub fn is_first(&self) -> bool {


We had these methods earlier, but ended up removing them, because they give vague/no information about the underlying type (which is the important downside of Either). Instead, we could just use if let patterns wherever possible to remind ourselves about Either type. Anyway, please remove this impl

wafflespeanut · 2016-11-29T03:14:17Z

components/style/properties/properties.mako.rs

@@ -1145,7 +1145,7 @@ impl ComputedValues {
    #[inline]
    pub fn is_multicol(&self) -> bool {
        let style = self.get_column();
-        style.column_count.0.is_some() || style.column_width.0.is_some()
+        style.column_count.0.is_some() || style.column_width.is_first()


We should do something like,

match (style.column_count, style.column_width) { (Some(ref _count), Either::First(ref _length)) => true, _ => false, }

... so that it'll be self-explanatory :)

I think your version is the equivalent of &&, not ||. But I get the idea. Will do.

jcdyer · 2016-11-29T16:45:22Z

That failure seems unrelated to my work. Any suggestions?

wafflespeanut · 2016-11-29T16:48:37Z

Yep, it's unrelated :)

wafflespeanut · 2016-11-29T17:01:38Z

Squash the commits, and we'll be good to go :)

jcdyer · 2016-11-29T17:37:38Z

Appveyor failed again. https://ci.appveyor.com/project/servo/servo/build/1.0.11157/job/8ye06qtvjnak6i1p Different crate (angle this time). The previous build was fontsan.

KiChjang · 2016-11-29T18:28:04Z

@bors-servo r=Wafflespeanut

bors-servo · 2016-11-29T18:28:06Z

📌 Commit 7c22143 has been approved by Wafflespeanut

wafflespeanut · 2016-12-01T08:37:36Z

@emilio I'm re-enabling the test (it passes for me locally)

@bors-servo r+

bors-servo · 2016-12-01T08:37:36Z

📌 Commit e6c22ec has been approved by Wafflespeanut

bors-servo · 2016-12-01T09:36:10Z

⌛ Testing commit e6c22ec with merge 4f9a66a...


        Auto merge of #14394 - jcdyer:jcdyer/either-column-width, r=Wafflespe…

…anut Convert column-width to use Either  * Converted column-width to use `Either<Length, Auto>` * Added gecko glue code * Cleaned up old column-width glue code ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14350 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because tests already surround the refactored code.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14394)


        Convert column-width to use Either

Fixes #14350

wafflespeanut · 2016-12-01T09:49:08Z

@bors-servo r+

bors-servo · 2016-12-01T09:49:09Z

📌 Commit 5821ff7 has been approved by Wafflespeanut

bors-servo · 2016-12-01T09:49:11Z

⌛ Testing commit 5821ff7 with merge 5a6cf40...


        Auto merge of #14394 - jcdyer:jcdyer/either-column-width, r=Wafflespe…

…anut Convert column-width to use Either  * Converted column-width to use `Either<Length, Auto>` * Added gecko glue code * Cleaned up old column-width glue code ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14350 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because tests already surround the refactored code.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14394)

bors-servo · 2016-12-01T11:04:48Z

💔 Test failed - linux-rel-wpt

wafflespeanut · 2016-12-02T07:03:35Z

@bors-servo retry #14437 #14438

bors-servo · 2016-12-02T07:03:38Z

⌛ Testing commit 5821ff7 with merge a913815...


        Auto merge of #14394 - jcdyer:jcdyer/either-column-width, r=Wafflespe…

…anut Convert column-width to use Either  * Converted column-width to use `Either<Length, Auto>` * Added gecko glue code * Cleaned up old column-width glue code ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14350 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because tests already surround the refactored code.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14394)

bors-servo · 2016-12-02T07:55:07Z

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

highfive assigned metajack Nov 28, 2016

highfive added the S-awaiting-review label Nov 28, 2016

jcdyer force-pushed the jcdyer:jcdyer/either-column-width branch from 879b5ee to 2b759bc Nov 28, 2016

jcdyer force-pushed the jcdyer:jcdyer/either-column-width branch 2 times, most recently from 3f03cde to d2f26f5 Nov 28, 2016

jcdyer force-pushed the jcdyer:jcdyer/either-column-width branch from d2f26f5 to 92d2f05 Nov 29, 2016

wafflespeanut requested changes Nov 29, 2016

View changes

wafflespeanut added S-needs-code-changes and removed S-awaiting-review labels Nov 29, 2016

wafflespeanut assigned wafflespeanut and unassigned metajack Nov 29, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 29, 2016

jcdyer force-pushed the jcdyer:jcdyer/either-column-width branch from abbca03 to b7e1173 Nov 29, 2016

wafflespeanut added S-needs-squash and removed S-awaiting-review labels Nov 29, 2016

jcdyer force-pushed the jcdyer:jcdyer/either-column-width branch from b7e1173 to 7c22143 Nov 29, 2016

highfive added the S-awaiting-review label Nov 29, 2016

highfive removed S-awaiting-review S-needs-squash labels Nov 29, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Dec 1, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 1, 2016

Convert column-width to use Either

Loading status checks…

5821ff7

Fixes #14350

wafflespeanut force-pushed the jcdyer:jcdyer/either-column-width branch from e6c22ec to 5821ff7 Dec 1, 2016

highfive added S-awaiting-review and removed S-awaiting-merge labels Dec 1, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 1, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 1, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 2, 2016

bors-servo merged commit 5821ff7 into servo:master Dec 2, 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 Dec 2, 2016

bors-servo mentioned this pull request Dec 2, 2016

WIP: Add support for keyword values for min-width, min-height, max-width, and max-height #14432

Closed

0 of 5 tasks complete

servo / servo

Convert column-width to use Either #14394

Convert column-width to use Either #14394

jcdyer commented Nov 28, 2016 •

edited

highfive commented Nov 28, 2016

highfive commented Nov 28, 2016

jcdyer commented Nov 28, 2016

jcdyer commented Nov 29, 2016

wafflespeanut 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.

jcdyer commented Nov 29, 2016

wafflespeanut commented Nov 29, 2016

wafflespeanut commented Nov 29, 2016

jcdyer commented Nov 29, 2016 •

edited

KiChjang commented Nov 29, 2016

bors-servo commented Nov 29, 2016

wafflespeanut commented Dec 1, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

wafflespeanut commented Dec 1, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

wafflespeanut commented Dec 2, 2016

bors-servo commented Dec 2, 2016

bors-servo commented Dec 2, 2016

servo / servo

Join GitHub today

Convert column-width to use Either #14394

Convert column-width to use Either #14394

Conversation

jcdyer commented Nov 28, 2016 • edited

highfive commented Nov 28, 2016

highfive commented Nov 28, 2016

jcdyer commented Nov 28, 2016

jcdyer commented Nov 29, 2016

wafflespeanut left a comment

This comment has been minimized.

wafflespeanut Nov 29, 2016

This comment has been minimized.

wafflespeanut Nov 29, 2016

This comment has been minimized.

wafflespeanut Nov 29, 2016

This comment has been minimized.

wafflespeanut Nov 29, 2016

This comment has been minimized.

jcdyer Nov 29, 2016

This comment has been minimized.

wafflespeanut Nov 29, 2016

jcdyer commented Nov 29, 2016

wafflespeanut commented Nov 29, 2016

wafflespeanut commented Nov 29, 2016

jcdyer commented Nov 29, 2016 • edited

KiChjang commented Nov 29, 2016

bors-servo commented Nov 29, 2016

wafflespeanut commented Dec 1, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

wafflespeanut commented Dec 1, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

wafflespeanut commented Dec 2, 2016

bors-servo commented Dec 2, 2016

bors-servo commented Dec 2, 2016

jcdyer commented Nov 28, 2016 •

edited

jcdyer commented Nov 29, 2016 •

edited