Use enum BorderWidth as SpecifiedValue #13908

aethanyc · 2016-10-24T14:31:03Z

Use enum BorderWidth instead of a tuple-like struct to store the specified
value. BorderWidth is needed to be used in both longhand and shorthand
border width properties, so I put it in specified module.

Fixed #13869.

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

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-10-24T14:32:32Z

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

highfive · 2016-10-24T14:32:33Z

Heads up! This PR modifies the following files:

@bholley: components/style/values/specified/mod.rs, components/style/properties/shorthand/border.mako.rs, components/style/properties/longhand/border.mako.rs
@KiChjang: components/script/dom/element.rs
@fitzgen: components/script/dom/element.rs
@emilio: components/style/values/specified/mod.rs, components/style/properties/shorthand/border.mako.rs, components/style/properties/longhand/border.mako.rs

aethanyc · 2016-10-24T14:38:24Z

r? @heycam @Manishearth

aethanyc · 2016-10-24T14:55:47Z

This change only adds keyword support to the specified value. The type of computed value remains as Au. I'm not sure whether I need to take care of the computed value in this issue or not. Because the keyword value turns into Au in to_computed_value, but we don't store other information to turn it back to specified value in from_computed_value.

Also I'm not sure about the test coverage of the serialization of keyword values. Anyway, this is my first servo patch so I feel I'd better post my patch to get feedback as soon as it compiles :)

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

emilio · 2016-10-24T15:55:56Z

This looks pretty good to me!

Because the keyword value turns into Au in to_computed_value, but we don't store other information to turn it back to specified value in from_computed_value.

That's fine, it doesn't need to be a symmetrical operation. The from_computed_value is only going used to put a computed value into the cascade from animations.

The only requisite is that to_computed_value(from_computed_value(computed)) == computed, which holds in this case, but not that from_computed_value(to_computed_value(specified)) == specified.

About serialization, we might have WPT tests in place, or otherwise we have a few files with tests around (see, for example, #13886). I'll trigger a try run so you don't have to do work that is already done by someone else.

@bors-servo: try

bors-servo · 2016-10-24T15:56:00Z

⌛ Trying commit ce2ff3e with merge 8ab8cf2...


        Auto merge of #13908 - aethanyc:add-border-width-keyword, r=<try>

Use enum BorderWidth as SpecifiedValue Use enum BorderWidth instead of a tuple-like struct to store the specified value. BorderWidth is needed to be used in both longhand and shorthand border width properties, so I put it in `specified` module. Fixed #13869. ---  - [x] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [x] These changes fix #13869 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13908)

Manishearth · 2016-10-24T16:18:55Z

components/style/values/specified/mod.rs

+    #[inline]
+    fn to_computed_value(&self, context: &Context) -> Self::ComputedValue {
+        match *self {
+            BorderWidth::Thin => Length::from_px(1.).to_computed_value(context),


We should perhaps leave a comment here linking to both the spec and the gecko constants for this

Manishearth · 2016-10-24T16:21:24Z

but we don't store other information to turn it back to specified value in from_computed_value.

from_computed_value does not need to recover the original specified value. It must only satisfy the property that to_computed_value(from_computed_value(..)) is idempotent.

Manishearth · 2016-10-24T16:21:56Z

r? @emilio

bors-servo · 2016-10-25T02:52:37Z

💥 Test timed out

Manishearth · 2016-10-25T03:02:01Z

@bors-servo retry try

bors-servo · 2016-10-25T03:02:04Z

⌛ Trying commit ce2ff3e with merge 7567333...


        Auto merge of #13908 - aethanyc:add-border-width-keyword, r=<try>

Use enum BorderWidth as SpecifiedValue Use enum BorderWidth instead of a tuple-like struct to store the specified value. BorderWidth is needed to be used in both longhand and shorthand border width properties, so I put it in `specified` module. Fixed #13869. ---  - [x] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [x] These changes fix #13869 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13908)

bors-servo · 2016-10-25T03:35:57Z

💔 Test failed - linux-rel-wpt

aethanyc · 2016-10-25T15:00:22Z

@emilio @Manishearth: Thank you for the review and clarify the invariant of from_computed_value.

@bors-servo retry try

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

components/style/values/specified/mod.rs, line 1360 at r1 (raw file):

Previously, Manishearth (Manish Goregaokar) wrote…

We should perhaps leave a comment here linking to both the spec and the gecko constants for this

Done.

Comments from Reviewable

aethanyc · 2016-10-25T15:14:39Z

@bors-servo retry try

emilio · 2016-10-25T15:27:33Z

@bors-servo: try

@aethanyc: homu only listens to a group of people that have try access (see servo/saltfs). Feel free to make a PR adding yourself to the try list if you want :)

Edit: see servo/saltfs@ba6ee27 for example.

It seems there are no tests passing due to this change, could you write one? Thanks!

Manishearth · 2016-10-25T15:28:07Z

@bors-servo retry try


        Auto merge of #13908 - aethanyc:add-border-width-keyword, r=<try>

Use enum BorderWidth as SpecifiedValue Use enum BorderWidth instead of a tuple-like struct to store the specified value. BorderWidth is needed to be used in both longhand and shorthand border width properties, so I put it in `specified` module. Fixed #13869. ---  - [x] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [x] These changes fix #13869 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13908)

bors-servo · 2016-10-25T15:28:16Z

⌛ Trying commit 3f59a0b with merge b3aa782...

bors-servo · 2016-10-25T15:31:05Z

⌛ Trying commit 3f59a0b with merge 1623034...

bors-servo · 2016-10-27T20:18:21Z

💔 Test failed - linux-rel-wpt

jdm · 2016-10-29T21:08:20Z

@bors-servo: retry

bors-servo · 2016-10-29T21:16:56Z

⌛ Testing commit 0fc38f7 with merge 7fbbbb2...


        Auto merge of #13908 - aethanyc:add-border-width-keyword, r=emilio

Use enum BorderWidth as SpecifiedValue Use enum BorderWidth instead of a tuple-like struct to store the specified value. BorderWidth is needed to be used in both longhand and shorthand border width properties, so I put it in `specified` module. Fixed #13869. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13869 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13908)

bors-servo · 2016-10-29T22:14:09Z

💔 Test failed - linux-rel-wpt

emilio · 2016-10-29T22:45:27Z

@bors-servo retry

#13776

bors-servo · 2016-10-29T23:15:23Z

⌛ Testing commit 0fc38f7 with merge 39958ff...


        Auto merge of #13908 - aethanyc:add-border-width-keyword, r=emilio

Use enum BorderWidth as SpecifiedValue Use enum BorderWidth instead of a tuple-like struct to store the specified value. BorderWidth is needed to be used in both longhand and shorthand border width properties, so I put it in `specified` module. Fixed #13869. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13869 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13908)

bors-servo · 2016-10-30T00:17:22Z

💔 Test failed - linux-rel-wpt

emilio · 2016-10-30T00:24:19Z

@bors-servo retry

#12797

bors-servo · 2016-10-30T00:48:28Z

⌛ Testing commit 0fc38f7 with merge 357e746...


        Auto merge of #13908 - aethanyc:add-border-width-keyword, r=emilio

Use enum BorderWidth as SpecifiedValue Use enum BorderWidth instead of a tuple-like struct to store the specified value. BorderWidth is needed to be used in both longhand and shorthand border width properties, so I put it in `specified` module. Fixed #13869. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13869 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13908)

bors-servo · 2016-10-30T01:43:53Z

☀️ 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 glennw Oct 24, 2016

highfive added the S-awaiting-review label Oct 24, 2016

Manishearth reviewed Oct 24, 2016

View changes

highfive assigned emilio and unassigned glennw Oct 24, 2016

bors-servo mentioned this pull request Oct 25, 2016

Update WR - includes a number of optimizations, bsd build fixes, tidy ups. #13900

Merged

highfive added the S-tests-failed label Oct 25, 2016

aethanyc force-pushed the aethanyc:add-border-width-keyword branch from ce2ff3e to 3f59a0b Oct 25, 2016

highfive removed the S-tests-failed label Oct 25, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Oct 27, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 27, 2016

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

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

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

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 30, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Oct 30, 2016

bors-servo merged commit 0fc38f7 into servo:master Oct 30, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Oct 30, 2016

aethanyc deleted the aethanyc:add-border-width-keyword branch Mar 21, 2017

servo / servo

Use enum BorderWidth as SpecifiedValue #13908

Use enum BorderWidth as SpecifiedValue #13908

aethanyc commented Oct 24, 2016 •

edited

highfive commented Oct 24, 2016

highfive commented Oct 24, 2016

aethanyc commented Oct 24, 2016

aethanyc commented Oct 24, 2016

emilio commented Oct 24, 2016 •

edited

bors-servo commented Oct 24, 2016

This comment has been minimized.

Manishearth commented Oct 24, 2016

Manishearth commented Oct 24, 2016

bors-servo commented Oct 25, 2016

Manishearth commented Oct 25, 2016

bors-servo commented Oct 25, 2016

bors-servo commented Oct 25, 2016

aethanyc commented Oct 25, 2016

aethanyc commented Oct 25, 2016

emilio commented Oct 25, 2016 •

edited

Manishearth commented Oct 25, 2016

bors-servo commented Oct 25, 2016

bors-servo commented Oct 25, 2016

bors-servo commented Oct 27, 2016

jdm commented Oct 29, 2016

bors-servo commented Oct 29, 2016

bors-servo commented Oct 29, 2016

emilio commented Oct 29, 2016

bors-servo commented Oct 29, 2016

bors-servo commented Oct 30, 2016

emilio commented Oct 30, 2016 •

edited

bors-servo commented Oct 30, 2016

bors-servo commented Oct 30, 2016

servo / servo

Join GitHub today

Use enum BorderWidth as SpecifiedValue #13908

Use enum BorderWidth as SpecifiedValue #13908

Conversation

aethanyc commented Oct 24, 2016 • edited

highfive commented Oct 24, 2016

highfive commented Oct 24, 2016

aethanyc commented Oct 24, 2016

aethanyc commented Oct 24, 2016

emilio commented Oct 24, 2016 • edited

bors-servo commented Oct 24, 2016

This comment has been minimized.

Manishearth Oct 24, 2016

Manishearth commented Oct 24, 2016

Manishearth commented Oct 24, 2016

bors-servo commented Oct 25, 2016

Manishearth commented Oct 25, 2016

bors-servo commented Oct 25, 2016

bors-servo commented Oct 25, 2016

aethanyc commented Oct 25, 2016

aethanyc commented Oct 25, 2016

emilio commented Oct 25, 2016 • edited

Manishearth commented Oct 25, 2016

bors-servo commented Oct 25, 2016

bors-servo commented Oct 25, 2016

bors-servo commented Oct 27, 2016

jdm commented Oct 29, 2016

bors-servo commented Oct 29, 2016

bors-servo commented Oct 29, 2016

emilio commented Oct 29, 2016

bors-servo commented Oct 29, 2016

bors-servo commented Oct 30, 2016

emilio commented Oct 30, 2016 • edited

bors-servo commented Oct 30, 2016

bors-servo commented Oct 30, 2016

aethanyc commented Oct 24, 2016 •

edited

emilio commented Oct 24, 2016 •

edited

emilio commented Oct 25, 2016 •

edited

emilio commented Oct 30, 2016 •

edited