add parsing/serialisation of 'grid-auto-flow', fixing issue #15313 #15364

galexite · 2017-02-03T07:37:23Z

This pull request adds the parsing and serialisation code for 'grid-auto-flow', as part of issue #15313.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #15313

There are tests for these changes OR
These changes do not require tests because only the parsing/serialisation code is added by this PR

This change is

highfive · 2017-02-03T07:37:29Z

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

highfive · 2017-02-03T07:37:29Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/longhand/position.mako.rs
@emilio: components/style/properties/longhand/position.mako.rs

highfive · 2017-02-03T07:37:30Z

Warning

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

wafflespeanut · 2017-02-03T17:46:56Z

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

+        #[derive(PartialEq, Clone, Eq, Copy, Debug)]
+        #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
+        pub struct T {
+            pub row_or_column: Option<RowOrColumn>,


Gecko uses bit fields to check whether dense exists along with row/column. Let's achieve this simply with a bool.

pub struct T { is_row: bool, dense: bool, }

This will reduce a lot of unwanted complexity in the code :)

@emilio You okay with this? I don't think we need an enum to represent whether it's a row or column.

Sure, as long as we serialize the same way, that's fine. (We could also use bitflags!)

Enums are fine IMO. It's more Rusty to use an enum (whereas they get used less in C++)

But yeah, for the computed value the Option isn't necessary.

+1 for enums.

Fair enough. Then, let's have AutoFlow::{Row, Column} in that case without the Option.

Manishearth · 2017-02-03T18:06:01Z

r? @wafflespeanut

emilio · 2017-02-03T18:08:35Z

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

+        #[derive(PartialEq, Clone, Eq, Copy, Debug)]
+        #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
+        pub struct T {
+            pub row_or_column: Option<RowOrColumn>,


Sure, as long as we serialize the same way, that's fine. (We could also use bitflags!)

emilio · 2017-02-03T18:08:35Z

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

+            }
+        }).is_ok() {}
+
+        if row_or_column | dense { Ok(value) } else { Err(()) }


nit: use || instead of a single bitwise |.

emilio · 2017-02-03T18:08:35Z

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

+        let mut dense = false;
+        let mut value = SpecifiedValue { row_or_column: None, dense: false };
+        while input.try(|input| {
+            if let Ok(ident) = input.expect_ident() {


nit: match_ignore_ascii_case! { input.expect_ident()?,, so you don't need to indent this more than necessary.

emilio · 2017-02-03T18:08:35Z

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

+        let mut row_or_column = false;
+        let mut dense = false;
+        let mut value = SpecifiedValue { row_or_column: None, dense: false };
+        while input.try(|input| {


why not just loop? We're not trying multiple things, so this try is doing just nothing.

emilio · 2017-02-03T18:08:35Z

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

+    /// [ row | column ] || dense
+    pub fn parse(_context: &ParserContext, input: &mut Parser) -> Result<SpecifiedValue, ()> {
+        let mut row_or_column = false;
+        let mut dense = false;


You can just use value.dense, and remove this variable.

wafflespeanut · 2017-02-05T03:26:13Z

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

+                           else { value.dense = true; continue },
+                _ => break
+            }
+        }


We don't parse more than 2 keywords here. I don't think there's much need for a loop at all. We could have something like,

if input.try(|i| i.expect_ident_matching("row")) { // row thing } else if ... { // column thing } // finally check dense

That'd be wrong, dense row is also a valid value for this property, the loop is required.

I didn't realize that until now. I thought the order actually mattered 😄

Yes, I did wonder how that could work out! Sorry for delay: busy since.

wafflespeanut · 2017-02-05T03:28:32Z

Everything looks good (except the parsing code). Once that's fixed, we'll land this :)

emilio · 2017-02-06T14:19:02Z

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

+        let mut value = get_initial_value();
+        loop {
+            match_ignore_ascii_case! { input.expect_ident()?,
+                "row" => if autoflow { break }


This should return an error right? otherwise we'd be parsing things like "row column".

emilio · 2017-02-06T14:19:02Z

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

+                           else { value.dense = true; continue },
+                _ => break
+            }
+        }


That'd be wrong, dense row is also a valid value for this property, the loop is required.

emilio · 2017-02-06T14:19:02Z

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

+                            },
+                "dense" => if value.dense { break }
+                           else { value.dense = true; continue },
+                _ => break


Similarly, this should return an error.

emilio · 2017-02-06T14:19:02Z

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

+                             value.autoflow = computed_value::AutoFlow::Row;
+                             continue
+                         },
+                "column" => if autoflow { break }


Similarly here.

emilio · 2017-02-06T14:19:02Z

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

+                                value.autoflow = computed_value::AutoFlow::Column;
+                                continue
+                            },
+                "dense" => if value.dense { break }


Similarly, this would parse dense dense correctly. Please fix and add a test case. You may need to change the loop to while !input.is_exhausted().

Please add tests for all the test cases I've mentioned.

bors-servo · 2017-02-10T20:21:25Z

☔ The latest upstream changes (presumably #15466) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2017-02-18T13:53:17Z

@galexite Are you planning to address the review comments?

wafflespeanut · 2017-02-24T11:46:25Z

I never got notified of this. Next time, feel free to ping us whenever you want something to be landed :)

wafflespeanut · 2017-02-24T11:47:02Z

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

+        let mut value = get_initial_value();
+        loop {
+            match_ignore_ascii_case! { input.expect_ident()?,
+                "row" => if autoflow { Err(()) }


I don't think this works as expected. Shouldn't all these be return Err(())?

wafflespeanut · 2017-02-24T11:49:36Z

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

+                _ => Err(())
+            }
+        }
+


Also, shouldn't the loop break at some point of time?

wafflespeanut · 2017-02-24T11:53:22Z

Like @emilio said, tests could help you with seeing what the code really serializes into. Please consider adding unit tests for this :)

galexite · 2017-02-27T04:39:12Z

@wafflespeanut @emilio Working on adding unit tests!

jdm · 2017-02-27T04:48:30Z

   Compiling style v0.0.1 (file:///C:/projects/servo/components/style)
error[E0432]: unresolved import `values::NoViewportPercentage`
     --> C:\projects\servo\target\debug\build\style-477c81918bb3d90e\out/properties.rs:43553:9
      |
43553 |     use values::NoViewportPercentage;
      |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `NoViewportPercentage` in `values`. Did you mean to use `HasViewportPercentage`?
error: cannot continue compilation due to previous error
error: Could not compile `style`.
To learn more, run the command again with --verbose.

galexite · 2017-02-27T04:52:02Z

@jdm yes, I haven't uploaded my updated code, I just resynched my fork with upstream!

Is it OK to replace the loop with while !input.expect_semicolon().is_ok()? If not, how do I break the loop when the statement ends?

wafflespeanut · 2017-02-27T09:02:49Z

I think we can loop as long as we get an identifier from the parser.

galexite · 2017-02-27T09:08:30Z

@wafflespeanut but what happens if an integer or string is encountered? The loop breaks and there will be no error.

galexite · 2017-02-27T09:12:21Z

If I looped until the semicolon, then if an integer or string appeared, &try! returns Err(()) early, as it should.

wafflespeanut · 2017-02-27T09:29:16Z

Why wouldn't there be an error? Once the loop ends, we should check that one of dense or row or column keywords have been parsed successfully. If not, we'll return an error.

galexite · 2017-02-27T09:34:59Z

@wafflespeanut then how do I break the loop when the statement ends? i.e.

grid-auto-flow: row dense;
/*                       ^ what happens here? */

Because of try!, the function will return Err(()) when the semicolon is encountered as it's not an ident.

rustc believes that the loop won't end either: error: unreachable code at the last line (if autoflow...) of the parse function.

If I were to replace loop with while a semicolon is not present, the while loop breaks when the semicolon is parsed.

emilio · 2017-02-27T09:54:41Z

Note that the parser has already looked for the semicolon so in your example you'll only be parsing row dense, which produces the behavior you expect.

Also, the parser looks for exhaustion, so if you return Ok(()) but the input is not exhausted, it will discard the result and return an error.

galexite · 2017-02-27T10:05:05Z

@emilio I couldn't grasp that from the http://docs.rs/ documentation. Great, thank you.

galexite · 2017-02-27T11:57:48Z

ready @wafflespeanut @emilio

wafflespeanut

Thanks! I've got some comments.

wafflespeanut · 2017-02-27T12:09:59Z

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

+    pub fn parse(_context: &ParserContext, input: &mut Parser) -> Result<SpecifiedValue, ()> {
+        let mut autoflow = false;
+        let mut value = get_initial_value();
+        while !input.is_exhausted() {


This could simply be a loop

How would it finish without returning an error then?

wafflespeanut · 2017-02-27T12:09:59Z

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

+        let mut value = get_initial_value();
+        while !input.is_exhausted() {
+            match_ignore_ascii_case! { &try!(input.expect_ident()),
+                "row" => if autoflow { return Err(()) }


Since they're Copy, everything could be handled with pattern guards like so,

"row" if !autoflow => { }

and everything else will be matched with the wildcard pattern, returning Err(())

I don't think that match_ignore_ascii_case! supports guards, so this is fine IMO.

wafflespeanut · 2017-02-27T12:09:59Z

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

+                         else {
+                             autoflow = true;
+                             value.autoflow = computed_value::AutoFlow::Row;
+                             continue


We could also avoid continue everywhere.

wafflespeanut · 2017-02-27T12:09:59Z

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

+        let mut autoflow = false;
+        let mut value = get_initial_value();
+        while !input.is_exhausted() {
+            match_ignore_ascii_case! { &try!(input.expect_ident()),


This should be input.try(|input| input.expect_ident()) so that once we find an error, we can break out of the loop. Here's a similar case that may be of help (it makes use of bit fields, but the essential idea is the same).

I think the code as written before worked fine? What's the advantage of that? This function only parses idents.

It's just that I've never seen using input.is_exhausted anywhere in our parsing code. Mostly, we just loop, input.try(...) and break on getting an error. Here, the loop doesn't break at all.

wafflespeanut · 2017-02-27T12:10:31Z

Also, please don't specify our names in the commit message 😄


        add parsing/serialisation of 'grid-auto-flow'

wafflespeanut · 2017-02-28T14:01:10Z

For the record, servo/rust-cssparser#123 adds support for match_ascii_ignore_case! with pattern guards, but this doesn't have to block on that. We'll land it whenever this is ready!

wafflespeanut · 2017-03-03T11:36:45Z

@galexite FYI, the cssparser bump landed a few days back. You can now add pattern guards to the macro. You planning on finishing this off? I think the code needs changes (it's not what I or emilio suggested).

galexite · 2017-03-04T03:08:08Z

OK, thank you!

wafflespeanut · 2017-03-18T18:24:47Z

Updated and reopened in #16024


        Auto merge of #16024 - Wafflespeanut:gal, r=Wafflespeanut

Add parsing/serialisation for 'grid-auto-flow' Didn't want the work (and review comments) in #15364 to go wasted. Fixes #15313  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16024)

highfive assigned Manishearth Feb 3, 2017

highfive added the S-awaiting-review label Feb 3, 2017

wafflespeanut reviewed Feb 3, 2017

View changes

wafflespeanut added S-needs-code-changes and removed S-awaiting-review labels Feb 3, 2017

highfive assigned wafflespeanut and unassigned Manishearth Feb 3, 2017

emilio reviewed Feb 3, 2017

View changes

highfive added S-awaiting-review and removed S-needs-code-changes labels Feb 4, 2017

wafflespeanut reviewed Feb 5, 2017

View changes

wafflespeanut added S-needs-code-changes and removed S-awaiting-review labels Feb 6, 2017

emilio requested changes Feb 6, 2017

View changes

highfive added the S-needs-rebase label Feb 10, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Feb 19, 2017

wafflespeanut reviewed Feb 24, 2017

View changes

components/style/properties/longhand/position.mako.rs Outdated

_ => Err(())

}

}

This comment has been minimized.

Sign in to view

wafflespeanut Feb 24, 2017
Member

Also, shouldn't the loop break at some point of time?

wafflespeanut added S-needs-tests S-needs-code-changes labels Feb 24, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Feb 26, 2017

wafflespeanut requested changes Feb 27, 2017

View changes

wafflespeanut added S-needs-code-changes and removed S-awaiting-review S-needs-tests labels Feb 27, 2017

add parsing/serialisation of 'grid-auto-flow'

Loading status checks…

82b198b

highfive added S-awaiting-review and removed S-needs-code-changes labels Feb 27, 2017

wafflespeanut added S-needs-code-changes and removed S-awaiting-review labels Mar 3, 2017

wafflespeanut mentioned this pull request Mar 18, 2017

Add parsing/serialisation for 'grid-auto-flow' #16024

Merged

wafflespeanut closed this Mar 18, 2017

servo / servo

Join GitHub today

add parsing/serialisation of 'grid-auto-flow', fixing issue #15313 #15364

add parsing/serialisation of 'grid-auto-flow', fixing issue #15313 #15364

Conversation

galexite commented Feb 3, 2017 • edited

highfive commented Feb 3, 2017

highfive commented Feb 3, 2017

highfive commented Feb 3, 2017

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.

Manishearth commented Feb 3, 2017

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

wafflespeanut commented Feb 5, 2017

This comment has been minimized.

emilio Feb 6, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Feb 10, 2017

jdm commented Feb 18, 2017

wafflespeanut commented Feb 24, 2017

This comment has been minimized.

This comment has been minimized.

wafflespeanut commented Feb 24, 2017

galexite commented Feb 27, 2017

jdm commented Feb 27, 2017

galexite commented Feb 27, 2017 • edited

wafflespeanut commented Feb 27, 2017

galexite commented Feb 27, 2017 • edited

galexite commented Feb 27, 2017 • edited

wafflespeanut commented Feb 27, 2017

galexite commented Feb 27, 2017 • edited

emilio commented Feb 27, 2017

galexite commented Feb 27, 2017

galexite commented Feb 27, 2017

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.

This comment has been minimized.

This comment has been minimized.

wafflespeanut commented Feb 27, 2017

wafflespeanut commented Feb 28, 2017 • edited

wafflespeanut commented Mar 3, 2017

galexite commented Mar 4, 2017

wafflespeanut commented Mar 18, 2017

galexite commented Feb 3, 2017 •

edited

emilio Feb 6, 2017 •

edited

galexite commented Feb 27, 2017 •

edited

galexite commented Feb 27, 2017 •

edited

galexite commented Feb 27, 2017 •

edited

galexite commented Feb 27, 2017 •

edited

wafflespeanut commented Feb 28, 2017 •

edited