style: Move border-image-repeat outside of mako. #19668

collares · 2018-01-01T15:51:49Z

Part of #19015.

r? emilio

./mach build -d does not report any errors
./mach build-geckolib does not report any errors
./mach test-tidy does not report any errors
These changes fix #19021
These changes do not require tests

This change is

highfive · 2018-01-01T15:51:54Z

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

highfive · 2018-01-01T15:51:55Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/gecko.mako.rs, components/style/values/specified/border.rs, components/style/values/specified/mod.rs, components/style/values/computed/border.rs, components/style/values/computed/mod.rs and 1 more
@canaltinova: components/style/properties/gecko.mako.rs, components/style/values/specified/border.rs, components/style/values/specified/mod.rs, components/style/values/computed/border.rs, components/style/values/computed/mod.rs and 1 more
@emilio: components/layout/display_list_builder.rs, components/style/properties/gecko.mako.rs, components/style/values/specified/border.rs, components/style/values/specified/mod.rs, components/style/values/computed/border.rs and 2 more

highfive · 2018-01-01T15:51:56Z

Warning

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

CYBAI · 2018-01-01T16:00:01Z

components/style/values/computed/border.rs

+    fn from_computed_value(computed: &Self::ComputedValue) -> Self {
+        SpecifiedBorderImageRepeat(computed.0, Some(computed.1))
+    }
+}


I think you can impl ToComputedValue for BorderImageRepeat inside specified module like BorderSideWidth in specified/border.rs.

Oh yeah, it should have been in specified/border.rs. Thanks for catching that :)

Now that I think about it, I believe the reason BorderSideWidth can be in specified/border.rs is that its computed value is particularly simple (computed::NonNegativeLength). I don't think making specified/border.rs depend on computed/border.rs (for its ComputedValue) would be a good idea. If you think the cyclic dependency is not a problem, though, I can move it.

I think it's okay to make it depend on computed/border.rs.

You can also check specified/font.rs.
It import font as computed

servo/components/style/values/specified/font.rs

Line 22 in 657d8a2

use values::computed::{font as computed, Context, Length, NonNegativeLength, ToComputedValue};

and use in the specified module.

Ex.

servo/components/style/values/specified/font.rs

Lines 1122 to 1142 in 657d8a2

impl ToComputedValue for FontVariantEastAsian {

type ComputedValue = computed::FontVariantEastAsian;

fn to_computed_value(&self, _context: &Context) -> computed::FontVariantEastAsian {

match *self {

FontVariantEastAsian::Value(ref v) => v.clone(),

FontVariantEastAsian::System(_) => {

#[cfg(feature = "gecko")] {

_context.cached_system_font.as_ref().unwrap().font_variant_east_asian.clone()

}

#[cfg(feature = "servo")] {

unreachable!()

}

}

}

}

fn from_computed_value(other: &computed::FontVariantEastAsian) -> Self {

FontVariantEastAsian::Value(other.clone())

}

}

I see, that is good to know!

I don't mean to be difficult, but I wanted to understand why one way is better than the other, as they look mostly the same to me. I grepped for "impl ToComputedValue" and specified/ has 33 impls (12 of them in font.rs), while computed/ has 15 impls (including background-repeat, on which I based this). I understand you have been working on this a lot, so a completely valid reason would be something like "Consistency is good and I prefer us to standardise on this" :)

We're not consistent at this at all, I filed #19670. IIRC @nox had a stronger opinion than me at this regarding dependencies from computed to specified and vice-versa, so if it's not very urgent we could wait for different arguments to come in there and then make a decision and move all of them :)


        style: Move border-image-repeat outside of mako.

emilio

This looks great, thanks @mauricioc!

emilio · 2018-01-01T18:21:56Z

components/style/values/computed/border.rs

+    }
+}
+
+impl ToComputedValue for SpecifiedBorderImageRepeat {


I think it's fine to keep this consistent with BackgroundRepeat for now.

emilio · 2018-01-01T18:21:56Z

components/style/values/computed/border.rs

+///
+/// https://drafts.csswg.org/css-backgrounds/#propdef-border-image-repeat
+#[derive(Clone, Debug, MallocSizeOf, PartialEq, ToCss)]
+pub struct BorderImageRepeat(pub RepeatKeyword, pub RepeatKeyword);


We already have a RepeatKeyword in the background module, maybe rename that one as BackgroundRepeatKeyword, and this as BorderImageRepeatKeyword as a followup?

(Background)RepeatKeyword appeared in gecko.mako.rs in two places before my changes. The first one is clearly rename-able, but the second one is in fn to_servo(repeat: StyleImageLayerRepeat) -> RepeatKeyword. Is RepeatKeyword being used here in the background sense, or is it being reused for some other purpose? Does this affect the renaming decision? Either way, I've renamed the RepeatKeyword I just added to BorderImageRepeatKeyword.

ImageLayerRepeat is definitely related to backgrounds in Gecko terminology, so I think it's fine to rename both.

emilio · 2018-01-01T18:22:09Z

@bors-servo r+

bors-servo · 2018-01-01T18:22:10Z

📌 Commit 9695d49 has been approved by emilio


        style: Rename background::RepeatKeyword and border::RepeatKeyword.

emilio · 2018-01-01T21:49:20Z

@bors-servo r+

bors-servo · 2018-01-01T21:49:21Z

📌 Commit 9a58fd7 has been approved by emilio

bors-servo · 2018-01-01T21:49:26Z

⌛ Testing commit 9a58fd7 with merge 8dacd16...


        Auto merge of #19668 - mauricioc:border-image-repeat, r=emilio

style: Move border-image-repeat outside of mako. Part of #19015. r? emilio --- - [X] `./mach build -d` does not report any errors - [X] `./mach build-geckolib` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #19021 - [X] These changes do not require tests  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19668)

bors-servo · 2018-01-01T22:01:26Z

💔 Test failed - mac-dev-unit

collares · 2018-01-01T22:29:33Z

Due to some miscommunication, there is a parallel PR for the same issue (I noticed this by coincidence while checking bors' queue). Since I believe this is a great first bug, since I forgot about this one for a long time, and since the other PR has been in the works for a while, I am giving it priority and closing this one. I will do #19671 instead :)

emilio · 2018-01-01T22:46:32Z

For the record, the other PR is #19210, sorry I didn't caught that before :(


        Auto merge of #19924 - emilio:bim, r=emilio

style: Move border-image-repeat outside of mako. This is a rebased / nitpick-addressed / bug-fixed version of #19021, and with a commit from #19668 renaming the two `RepeatKeyword`s to different names.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19924)


        Auto merge of #19924 - emilio:bim, r=emilio

style: Move border-image-repeat outside of mako. This is a rebased / nitpick-addressed / bug-fixed version of #19021, and with a commit from #19668 renaming the two `RepeatKeyword`s to different names.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19924)


        Auto merge of #19924 - emilio:bim, r=emilio

style: Move border-image-repeat outside of mako. This is a rebased / nitpick-addressed / bug-fixed version of #19021, and with a commit from #19668 renaming the two `RepeatKeyword`s to different names.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19924)

highfive assigned emilio Jan 1, 2018

highfive added the S-awaiting-review label Jan 1, 2018

CYBAI reviewed Jan 1, 2018

View changes

style: Move border-image-repeat outside of mako.

Loading status checks…

9695d49

emilio approved these changes Jan 1, 2018

View changes

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 1, 2018

style: Rename background::RepeatKeyword and border::RepeatKeyword.

Loading status checks…

9a58fd7

highfive added S-awaiting-review and removed S-awaiting-merge labels Jan 1, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 1, 2018

emilio mentioned this pull request Jan 1, 2018

Unify background placement code #19651

Merged

highfive added S-tests-failed and removed S-awaiting-merge labels Jan 1, 2018

collares closed this Jan 1, 2018

collares mentioned this pull request Jan 1, 2018

Move border-image-repeat outside of mako. #19021

Closed

emilio mentioned this pull request Feb 1, 2018

style: Move border-image-repeat outside of mako. #19924

Merged

emilio mentioned this pull request Feb 3, 2018

Move -moz-border-*-colors outside of mako. #19671

Closed

servo / servo

style: Move border-image-repeat outside of mako. #19668

style: Move border-image-repeat outside of mako. #19668

collares commented Jan 1, 2018 •

edited by emilio

highfive commented Jan 1, 2018

highfive commented Jan 1, 2018

highfive commented Jan 1, 2018

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.

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio commented Jan 1, 2018

bors-servo commented Jan 1, 2018

emilio commented Jan 1, 2018

bors-servo commented Jan 1, 2018

bors-servo commented Jan 1, 2018

bors-servo commented Jan 1, 2018

collares commented Jan 1, 2018 •

edited

emilio commented Jan 1, 2018

	impl ToComputedValue for FontVariantEastAsian {
	type ComputedValue = computed::FontVariantEastAsian;

	fn to_computed_value(&self, _context: &Context) -> computed::FontVariantEastAsian {
	match *self {
	FontVariantEastAsian::Value(ref v) => v.clone(),
	FontVariantEastAsian::System(_) => {
	#[cfg(feature = "gecko")] {
	_context.cached_system_font.as_ref().unwrap().font_variant_east_asian.clone()
	}
	#[cfg(feature = "servo")] {
	unreachable!()
	}
	}
	}
	}

	fn from_computed_value(other: &computed::FontVariantEastAsian) -> Self {
	FontVariantEastAsian::Value(other.clone())
	}
	}

servo / servo

Join GitHub today

style: Move border-image-repeat outside of mako. #19668

style: Move border-image-repeat outside of mako. #19668

Conversation

collares commented Jan 1, 2018 • edited by emilio

highfive commented Jan 1, 2018

highfive commented Jan 1, 2018

highfive commented Jan 1, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

collares Jan 1, 2018 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio commented Jan 1, 2018

bors-servo commented Jan 1, 2018

emilio commented Jan 1, 2018

bors-servo commented Jan 1, 2018

bors-servo commented Jan 1, 2018

bors-servo commented Jan 1, 2018

collares commented Jan 1, 2018 • edited

emilio commented Jan 1, 2018

collares commented Jan 1, 2018 •

edited by emilio

collares Jan 1, 2018 •

edited

collares commented Jan 1, 2018 •

edited