Move overflow-y outside of Mako

[PeterZhizhin]

I think that this should be an easy property to start with. This is a sub issue of #19015.

[olmanz]

@jdm
If it's OK for you, I can try this one.

[KiChjang]

@olmanz I believe @PeterZhizhin is already working on this.

[emilio]

Yeah, note that this one is actually extra-tricky.

[KiChjang]

@PeterZhizhin Do you need any help? Any questions?

[highfive]

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

[KiChjang]

Unassigning due to lack of response. This issue is now open for anyone to work on!

[olmanz]

@highfive: assign me

[highfive]

Hey @olmanz! Thanks for your interest in working on this issue. It's now assigned to you!

[olmanz]

@KiChjang @emilio
I already moved some stuff outside of Mako, as you can see in this commit on my fork:
olmanz@6f0e69f

However, I currently get this message when I build servo with ./mach build --dev:

   Compiling style v0.0.1 (file:///home/olmanz/Schreibtisch/workspaces/Rust/servo/components/style)
error[E0061]: this function takes 1 parameter but 2 parameters were supplied
    --> /home/olmanz/Schreibtisch/workspaces/Rust/servo/target/debug/build/style-fe2cedc35103ef47/out/properties.rs:7826:40
     |
7826 |               specified::Overflow::parse(context, input)
     |                                          ^^^^^^^^^^^^^^ expected 1 parameter
     | 
    ::: components/style/values/specified/box.rs
     |
128  | / define_css_keyword_enum! { Overflow:
129  | |     "visible" => Visible,
130  | |     "hidden" => Hidden,
131  | |     "scroll" => Scroll,
132  | |     "auto" => Auto,
133  | | }
     | |_- defined here
     |
     = note: this error originates in a macro outside of the current crate

error[E0061]: this function takes 1 parameter but 2 parameters were supplied
    --> /home/olmanz/Schreibtisch/workspaces/Rust/servo/target/debug/build/style-fe2cedc35103ef47/out/properties.rs:7937:40
     |
7937 |               specified::Overflow::parse(context, input)
     |                                          ^^^^^^^^^^^^^^ expected 1 parameter
     | 
    ::: components/style/values/specified/box.rs
     |
128  | / define_css_keyword_enum! { Overflow:
129  | |     "visible" => Visible,
130  | |     "hidden" => Hidden,
131  | |     "scroll" => Scroll,
132  | |     "auto" => Auto,
133  | | }
     | |_- defined here
     |
     = note: this error originates in a macro outside of the current crate

error: aborting due to 2 previous errors

error: Could not compile `style`.

To learn more, run the command again with --verbose.
[Warning] Could not generate notification! Optional Python module 'dbus' is not installed.
Build FAILED in 0:01:14

I found out that the file /target/debug/build/style-fe2cedc35103ef47/out/properties.rs is created by components/style/properties/build.py, but I'm not quite sure how to get rid of this error message.

[CYBAI]

I think the root cause is the parse method duplicated between define_css_keyword_enum macro and add_impls_for_keyword_enum macro.

The define_css_keyword_enum is defined under style_traits/values.rs and it will impl your enum with a parse method with only one argument.

servo/components/style_traits/values.rs

Lines 388 to 416 in b7bb6ff

	#[macro_export]
	macro_rules! define_css_keyword_enum {
	($name: ident: values { $( $css: expr => $variant: ident),+, }
	aliases { $( $alias: expr => $alias_variant: ident ),+, }) => {
	__define_css_keyword_enum__add_optional_traits!($name [ $( $css => $variant ),+ ]
	[ $( $alias => $alias_variant ),+ ]);
	};
	($name: ident: values { $( $css: expr => $variant: ident),+, }
	aliases { $( $alias: expr => $alias_variant: ident ),* }) => {
	__define_css_keyword_enum__add_optional_traits!($name [ $( $css => $variant ),+ ]
	[ $( $alias => $alias_variant ),* ]);
	};
	($name: ident: values { $( $css: expr => $variant: ident),+ }
	aliases { $( $alias: expr => $alias_variant: ident ),+, }) => {
	__define_css_keyword_enum__add_optional_traits!($name [ $( $css => $variant ),+ ]
	[ $( $alias => $alias_variant ),+ ]);
	};
	($name: ident: values { $( $css: expr => $variant: ident),+ }
	aliases { $( $alias: expr => $alias_variant: ident ),* }) => {
	__define_css_keyword_enum__add_optional_traits!($name [ $( $css => $variant ),+ ]
	[ $( $alias => $alias_variant ),* ]);
	};
	($name: ident: $( $css: expr => $variant: ident ),+,) => {
	__define_css_keyword_enum__add_optional_traits!($name [ $( $css => $variant ),+ ] []);
	};
	($name: ident: $( $css: expr => $variant: ident ),+) => {
	__define_css_keyword_enum__add_optional_traits!($name [ $( $css => $variant ),+ ] []);
	};
	}

The add_impls_for_keyword_enum is defined under macro.rs and it will impl Parse which will need two arguments.

servo/components/style/macros.rs

Lines 107 to 127 in b7bb6ff

	/// A macro for implementing `ToComputedValue`, and `Parse` traits for
	/// the enums defined using `define_css_keyword_enum` macro.
	///
	/// NOTE: We should either move `Parse` trait to `style_traits`
	/// or `define_css_keyword_enum` macro to this crate, but that
	/// may involve significant cleanup in both the crates.
	macro_rules! add_impls_for_keyword_enum {
	($name:ident) => {
	impl $crate::parser::Parse for $name {
	#[inline]
	fn parse<'i, 't>(
	_context: &$crate::parser::ParserContext,
	input: &mut ::cssparser::Parser<'i, 't>,
	) -> Result<Self, ::style_traits::ParseError<'i>> {
	$name::parse(input)
	}
	}
	trivial_to_computed_value!($name);
	};
	}

Thus, when the predefined_type create the file,

servo/components/style/properties/helpers.mako.rs

Lines 36 to 48 in b7bb6ff

	#[allow(unused_variables)]
	#[inline]
	pub fn parse<'i, 't>(context: &ParserContext,
	input: &mut Parser<'i, 't>)
	-> Result<SpecifiedValue, ParseError<'i>> {
	% if allow_quirks:
	specified::${type}::${parse_method}_quirky(context, input, AllowQuirks::Yes)
	% elif needs_context:
	specified::${type}::${parse_method}(context, input)
	% else:
	specified::${type}::${parse_method}(input)
	% endif
	}

it will generate the file with specified::Overflow::parse(context, input).

So, all you need is passing needs_context=False in predefined_type.

You can reference to ScrollSnapType or OverscrollBehavior which are also in box.mako.rs.

[olmanz]

Thanks, that worked!

[olmanz]

Pull Request is out.
#19448

[jdm]

The previous author is busy in school right now. If anybody would like to tackle this, it is available. See #19448 for prior work.

[nicknadeau]

I'd like to work on this one if it's still open!

[jdm]

@nicknadeau Please do!

[nicknadeau]

So I've done some investigating and just want to make sure I understand correctly.
I'm essentially taking lines 212-216 of box.mako.rs, the overflow-y bit, and feeding it into helpers.predefined_type(...)?
Am I doing the same with overflow-x?

[jdm]

It's a combination of changing those mako lines to use predefined_type, as well as extracting the actual types that used to be generated by mako into proper Rust definitions in style/values, and ensuring that ./mach build-geckolib continues to work (based on #19448 (comment)).

[emilio]

Hi @nicknadeau, have you managed to make any progress on this?

Thanks!

[nicknadeau]

Hi @emilio, progress has been a little slow, just got out of midterms. I'll be giving this bug a go over the next few days and I'll update my status by mid-late week and let you know if I have any questions.

I hope that's alright, sorry for the slow start.

[emilio]

Sounds great, no worries, and thanks! :)

[nicknadeau]

Ok I've started out on a similar foot to olmanz and used overscroll-behaviour as an example to mimic.
So far I'm passing only a subset of the params into predefined_type and ./mach build seems to work fine but of course I've not taken care of the gecko bit yet so the gecko build breaks.

I'm just stuck here, I'm really not sure how to deal with some of these params. For instance, I've tried sending gecko_constant_prefix into predefined_type and I get:
TypeError: __init__() got an unexpected keyword argument 'gecko_constant_prefix' - triggered by line 455 in declare_longhand.
(sorry I'm working on another PC and don't have the full error msg with me)

So I'm wondering, am I trying to get things like gecko_constant_prefix, custom_consts etc. into predefined_type as well? Is it appropriate to use the single_keyword function to handle the gecko stuff? Sorry, still trying to understand how the code is working.

[62mkv]

maybe someone removes E-easy label ? judging by history it's "anything but" easy

[nicknadeau]

Well to be fair I've only spent ~2 hours on this so far, I'll keep tracing things and see what I can figure out.

[emilio]

maybe someone removes E-easy label ? judging by history it's "anything but" easy

FWIW, it should be easier now, the relevant prerequisites have already landed, like #19578.

[emilio]

So I'm wondering, am I trying to get things like gecko_constant_prefix, custom_consts etc. into predefined_type as well? Is it appropriate to use the single_keyword function to handle the gecko stuff? Sorry, still trying to understand how the code is working.

Sorry I missed this before, you should stop using gecko_constant_prefix into predefined_type, and then write the conversion manually like we do for other keywords, like overscroll-behavior.

[nicknadeau]

My progress so far:
https://github.com/nicknadeau/servo/commit/97d138dc370e542e5ec0ef04b31541da37238732
https://github.com/nicknadeau/servo/commit/ed6524aa94b03d248bbd90885c7f117e06c66098

In gecko build I get the following error coming from set_overflow_y in gecko.mako.rs
Still trying to figure out what overflow_x.keyword is doing

Traceback (most recent call last):
...
File "/home/nick/Desktop/servo/components/style/properties/gecko.mako.rs", line 1535, in render_impl_trait
${caller.body().strip()}
File "/home/nick/Desktop/servo/components/style/properties/gecko.mako.rs", line 3105, in body
$ for value in overflow_x.keyword.values_for('gecko'):
AttributeError: 'NoneType' object has no attribute 'values_for

[nicknadeau]

I'm sorry guys, I really don't know how to progress from here, I don't understand enough of this code base in terms of the gecko stuff

[jdm]

@emilio This is probably going to need your attention to answer the previous question.

[emilio]

Oh, sorry, thanks!

@nicknadeau: So that error means that overflow_x.keyword is None, which is expected because we're using predefined_type instead of keyword. That's fine, and the only thing that needs to happen is defining the keyword ourselves like overflow_behavior does:

https://searchfox.org/mozilla-central/rev/c217fbde244344fedfd07b57a740c694a456dbca/servo/components/style/properties/gecko.mako.rs#3423

[PhilParisot]

Hey @emilio and @jdm, I'd like to try and attempt this issue if that's OK with you. I just finished reading the book.

I'll say hi on #servo on Matrix.

Let me know!

Thanks!

[emilio]

I think this was effectively done in https://bugzilla.mozilla.org/show_bug.cgi?id=1470695. @jdm do you have any other potential good first issue for @PhilParisot?