style: Move text-overflow outside of mako #19044

cbrewster · 2017-10-27T21:14:16Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes are apart of Move CSS longhands outside of mako #19015 (github issue number if applicable).

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

This change is

highfive · 2017-10-27T21:14:21Z

Heads up! This PR modifies the following files:

@bholley: components/style/values/generics/text.rs, components/style/values/computed/text.rs, components/style/values/specified/mod.rs, components/style/properties/gecko.mako.rs, components/style/properties/longhand/text.mako.rs and 2 more
@canaltinova: components/style/values/generics/text.rs, components/style/values/computed/text.rs, components/style/values/specified/mod.rs, components/style/properties/gecko.mako.rs, components/style/properties/longhand/text.mako.rs and 2 more
@emilio: components/style/values/generics/text.rs, components/style/values/computed/text.rs, components/style/values/specified/mod.rs, components/style/properties/gecko.mako.rs, components/style/properties/longhand/text.mako.rs and 3 more

highfive · 2017-10-27T21:14:22Z

Warning

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

cbrewster · 2017-10-27T21:14:44Z

r? @emilio
I'm not sure I am doing the right thing here with Side.

emilio

r=me

emilio · 2017-10-27T23:11:20Z

components/style/values/computed/text.rs

+impl ToCss for TextOverflow {
+    fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
+        if self.sides_are_logical {
+            assert!(self.first == TextOverflowSide::Clip);


nit: if you want you can change this to assert_eq! or debug_assert_eq!.

emilio · 2017-10-27T23:12:11Z

components/style/values/specified/text.rs

+    #[inline]
+    fn to_computed_value(&self, _context: &Context) -> Self::ComputedValue {
+        if let Some(ref second) = self.second {
+            Self::ComputedValue { first: self.first.clone(),


nit: Feel free to fixup the indentation while you're touching this :)

emilio · 2017-10-27T23:12:37Z

components/style/values/specified/text.rs

+    fn parse<'i, 't>(context: &ParserContext, input: &mut Parser<'i, 't>) -> Result<TextOverflow, ParseError<'i>> {
+        let first = TextOverflowSide::parse(context, input)?;
+        let second = input.try(|input| TextOverflowSide::parse(context, input)).ok();
+        Ok(TextOverflow {


nit: This can use TextOverflow { first, second }, if you want.

cbrewster · 2017-10-28T01:09:01Z

@bors-servo r=emilio

bors-servo · 2017-10-28T01:09:02Z

📌 Commit b0ac6d9 has been approved by emilio

emilio · 2017-10-28T01:40:55Z

components/style/values/generics/text.rs

+
+/// A generic value for the `text-overflow` property.
+#[derive(Clone, Debug, Eq, MallocSizeOf, PartialEq, ToCss)]
+pub enum TextOverflowSide {


Hmm, though now that I realize of this, this is not a generic value per se (those are supposed to actually have generic arguments and such). Usually when the computed and specified values are the same, we just define it in the specified module and re-export it if needed from the computed one.

Of course this can be fixed in a followup if you want. Thanks for working on this btw!

No problem! I moved TextOverflowSide to the specified module, is that all that is needed?

bors-servo · 2017-10-28T01:45:55Z

⌛ Testing commit b0ac6d9 with merge 955bbea...

style: Move text-overflow outside of mako  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes are apart of #19015 (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because refactoring.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19044)

bors-servo · 2017-10-28T02:33:11Z

💔 Test failed - mac-rel-wpt3

emilio · 2017-10-28T09:42:32Z

@bors-servo r+

bors-servo · 2017-10-28T09:42:33Z

📌 Commit deb9327 has been approved by emilio

emilio · 2017-10-28T09:42:56Z

(You could have just landed yourself, no need to ask for re-review for a trivial change like that IMO :P)

cbrewster · 2017-10-28T16:48:47Z

@bors-servo retry

time-out in css blur test

bors-servo · 2017-10-28T16:49:55Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt4, windows-msvc-dev are reusable. Rebuilding only mac-rel-wpt3...

bors-servo · 2017-10-28T17:42:23Z

💔 Test failed - mac-rel-wpt3

cbrewster · 2017-10-28T18:20:52Z

@bors-servo retry

Timeout in CSS blur test

bors-servo · 2017-10-28T18:20:55Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt4, windows-msvc-dev are reusable. Rebuilding only mac-rel-wpt3...

bors-servo · 2017-10-28T19:13:23Z

💔 Test failed - mac-rel-wpt3

cbrewster · 2017-10-29T03:18:01Z

@bors-servo retry

time-out in css blur test

bors-servo · 2017-10-29T03:18:07Z

⌛ Testing commit deb9327 with merge 4078927...

style: Move text-overflow outside of mako  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes are apart of #19015 (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because refactoring.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19044)

bors-servo · 2017-10-29T04:12:43Z

💔 Test failed - mac-rel-wpt3

jdm · 2017-10-29T13:16:23Z

It should be clear to everyone by now that retrying will not change the result.

emilio · 2017-10-30T16:55:24Z

It should be clear to everyone by now that retrying will not change the result.

Well, a WR update landed in the meantime, and given this didn't timeout locally, it seemed reasonable to retry :)

jdm · 2017-10-30T17:05:04Z

That being said, the same timeout was observed in #18882 earlier today. It's still weird how it's not hitting any other PRs at anywhere near the same pervasive failure rate.

emilio · 2017-10-30T17:21:52Z

I'm 99.9% sure that this PR isn't to blame for that failure, that test doesn't even use text-overflow, and the only occurrence of it in servo.css uses text-overflow: inherit, which doesn't even use the paths this patch is moving.

emilio · 2017-10-30T18:15:52Z

@bors-servo retry #19064

bors-servo · 2017-10-30T19:04:54Z

⌛ Testing commit deb9327 with merge 7f8842f...

style: Move text-overflow outside of mako  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes are apart of #19015 (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because refactoring.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19044)

bors-servo · 2017-10-30T19:59:59Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: emilio
Pushing 7f8842f to master...

highfive assigned jdm Oct 27, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 27, 2017

highfive assigned emilio and unassigned jdm Oct 27, 2017

emilio mentioned this pull request Oct 27, 2017

Move CSS longhands outside of mako #19015

Closed

49 tasks

cbrewster force-pushed the text_overflow_nomako branch from a4693d0 to 9fdcc06 Compare October 27, 2017 22:58

emilio approved these changes Oct 27, 2017

View reviewed changes

cbrewster force-pushed the text_overflow_nomako branch from 9fdcc06 to b0ac6d9 Compare October 28, 2017 01:08

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 28, 2017

emilio reviewed Oct 28, 2017

View reviewed changes

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 28, 2017

style: Move text-overflow outside of mako

deb9327

cbrewster force-pushed the text_overflow_nomako branch from b0ac6d9 to deb9327 Compare October 28, 2017 04:09

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Oct 28, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 28, 2017

highfive added the S-tests-failed The changes caused existing tests to fail. label Oct 28, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Oct 29, 2017

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 29, 2017

emilio mentioned this pull request Oct 30, 2017

Intermittent timeout /_mozilla/css/blur_a.html #19064

Closed

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Oct 30, 2017

bors-servo merged commit deb9327 into servo:master Oct 30, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style: Move text-overflow outside of mako #19044

style: Move text-overflow outside of mako #19044

cbrewster commented Oct 27, 2017 •

edited by SimonSapin

Loading

highfive commented Oct 27, 2017

highfive commented Oct 27, 2017

cbrewster commented Oct 27, 2017

emilio left a comment

emilio Oct 27, 2017

emilio Oct 27, 2017

emilio Oct 27, 2017

cbrewster commented Oct 28, 2017

bors-servo commented Oct 28, 2017

emilio Oct 28, 2017

cbrewster Oct 28, 2017

bors-servo commented Oct 28, 2017

bors-servo commented Oct 28, 2017

emilio commented Oct 28, 2017

bors-servo commented Oct 28, 2017

emilio commented Oct 28, 2017

cbrewster commented Oct 28, 2017 •

edited

Loading

bors-servo commented Oct 28, 2017

bors-servo commented Oct 28, 2017

cbrewster commented Oct 28, 2017

bors-servo commented Oct 28, 2017

bors-servo commented Oct 28, 2017

cbrewster commented Oct 29, 2017

bors-servo commented Oct 29, 2017

bors-servo commented Oct 29, 2017

jdm commented Oct 29, 2017 •

edited

Loading

emilio commented Oct 30, 2017

jdm commented Oct 30, 2017

emilio commented Oct 30, 2017 •

edited

Loading

emilio commented Oct 30, 2017 •

edited

Loading

bors-servo commented Oct 30, 2017

bors-servo commented Oct 30, 2017

style: Move text-overflow outside of mako #19044

style: Move text-overflow outside of mako #19044

Conversation

cbrewster commented Oct 27, 2017 • edited by SimonSapin Loading

highfive commented Oct 27, 2017

highfive commented Oct 27, 2017

cbrewster commented Oct 27, 2017

emilio left a comment

Choose a reason for hiding this comment

emilio Oct 27, 2017

Choose a reason for hiding this comment

emilio Oct 27, 2017

Choose a reason for hiding this comment

emilio Oct 27, 2017

Choose a reason for hiding this comment

cbrewster commented Oct 28, 2017

bors-servo commented Oct 28, 2017

emilio Oct 28, 2017

Choose a reason for hiding this comment

cbrewster Oct 28, 2017

Choose a reason for hiding this comment

bors-servo commented Oct 28, 2017

bors-servo commented Oct 28, 2017

emilio commented Oct 28, 2017

bors-servo commented Oct 28, 2017

emilio commented Oct 28, 2017

cbrewster commented Oct 28, 2017 • edited Loading

bors-servo commented Oct 28, 2017

bors-servo commented Oct 28, 2017

cbrewster commented Oct 28, 2017

bors-servo commented Oct 28, 2017

bors-servo commented Oct 28, 2017

cbrewster commented Oct 29, 2017

bors-servo commented Oct 29, 2017

bors-servo commented Oct 29, 2017

jdm commented Oct 29, 2017 • edited Loading

emilio commented Oct 30, 2017

jdm commented Oct 30, 2017

emilio commented Oct 30, 2017 • edited Loading

emilio commented Oct 30, 2017 • edited Loading

bors-servo commented Oct 30, 2017

bors-servo commented Oct 30, 2017

cbrewster commented Oct 27, 2017 •

edited by SimonSapin

Loading

cbrewster commented Oct 28, 2017 •

edited

Loading

jdm commented Oct 29, 2017 •

edited

Loading

emilio commented Oct 30, 2017 •

edited

Loading

emilio commented Oct 30, 2017 •

edited

Loading