Closed Bug 1468651 Opened 6 years ago Closed 6 years ago

Generate different files for different style structs.

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: emilio, Assigned: emilio)

Details

Attachments

(5 files)

This will cut down properties.rs by a lot, which will make easier to debug, hopefully.
Comment on attachment 8985272 [details]
Bug 1468651: remove shorthand/serialize.mako.rs.

https://reviewboard.mozilla.org/r/250880/#review257812

::: servo/components/style/properties/properties.mako.rs:135
(Diff revision 1)
>      use cssparser::Parser;
>      use parser::{Parse, ParserContext};
>      use style_traits::{ParseError, StyleParseErrorKind};
>      use values::specified;
>  
> -    <%include file="/shorthand/serialize.mako.rs" />
> +    use style_traits::{CssWriter, ToCss};
> +    use values::specified::{BorderStyle, Color};
> +    use std::fmt::{self, Write};

Nit: Let's merge these two lists to look a bit neater.
Attachment #8985272 - Flags: review?(cam) → review+
Comment on attachment 8985273 [details]
Bug 1468651: Rename the directories from "shorthand" to "shorthands", "longhand" to "longhands".

https://reviewboard.mozilla.org/r/250882/#review257814

We do we need to create an empty properties/shorthands/inheritedbox.mako.rs file?

The renamings of the directories really should be in a separate patch.  Please split this out.

Overall this looks good, even though it renders some of what I mentioned on Wednesday out of date. :-)

::: commit-message-afac9:8
(Diff revision 2)
> +Also, renames the inherited_foo files to be inheritedfoo, to match
> +StyleStruct.name_lower.

Can we do the opposite, i.e. make StyleStruct.name_lower get set to "inherited_foo"?  "inheritedfoo.mako.rs" looks a bit ugly to me. :-)

::: commit-message-afac9:11
(Diff revision 2)
> + shorthand -> shorthands
> +
> +Also, renames the inherited_foo files to be inheritedfoo, to match
> +StyleStruct.name_lower.
> +
> +Other than that the changes should be straight-forward.

Nit: straightforward has no hyphen.  Although probably this line doesn't need to be in the commit message.

::: servo/components/style/properties/build.py:24
(Diff revision 2)
>  
>  RE_PYTHON_ADDR = re.compile(r'<.+? object at 0x[0-9a-fA-F]+>')
>  
> +OUT_DIR = os.environ.get("OUT_DIR", "")
> +
> +STYLE_STRUCT_LIST = [

I wonder if we should try to infer this list based on the files that exist under properties/longhands/?  Up to you.

::: servo/components/style/properties/properties.mako.rs:101
(Diff revision 2)
>      use cssparser::Parser;
>      use parser::{Parse, ParserContext};
>      use style_traits::{ParseError, StyleParseErrorKind};
>      use values::specified;
>  
>      use style_traits::{CssWriter, ToCss};
>      use values::specified::{BorderStyle, Color};
>      use std::fmt::{self, Write};

These don't need to be in scope for the longhands mod.

::: servo/components/style/properties/properties.mako.rs:110
(Diff revision 2)
>  
>      use style_traits::{CssWriter, ToCss};
>      use values::specified::{BorderStyle, Color};
>      use std::fmt::{self, Write};
>  
> +    macro_rules! unwrap_or_initial {

This doesn't need to be defined in the longhands mod.

::: servo/components/style/properties/properties.mako.rs:116
(Diff revision 2)
> +        ($prop: ident) => (unwrap_or_initial!($prop, $prop));
> +        ($prop: ident, $expr: expr) =>
> +            ($expr.unwrap_or_else(|| $prop::get_initial_specified_value()));
> +    }
> +
>      fn serialize_directional_border<W, I,>(

This also doesn't need to be defined in the longhands mod.

::: servo/components/style/properties/properties.mako.rs:139
(Diff revision 2)
> +    % for style_struct in data.style_structs:
> +    include!("${os.path.join(OUT_DIR, kind, '{}.rs'.format(style_struct.name_lower))}");
> +    % endfor

Given this is the only part we really want to be common to both the longhands and shorthands mods, I think it will be clearer not to loop over `["longhands", "shorthands"]`, and instead just keep separate mods and duplicate these three lines.
Attachment #8985273 - Flags: review?(cam) → review-
Comment on attachment 8985273 [details]
Bug 1468651: Rename the directories from "shorthand" to "shorthands", "longhand" to "longhands".

https://reviewboard.mozilla.org/r/250882/#review257814

Nope, that file is an overlook. Will split this up.

> Can we do the opposite, i.e. make StyleStruct.name_lower get set to "inherited_foo"?  "inheritedfoo.mako.rs" looks a bit ugly to me. :-)

I agree though it's more work and I need to change all Servo consumers...

> These don't need to be in scope for the longhands mod.

They're not though, right? It's wrapped in an `% if kind == "shorthands":`. Though agreed probably just splitting this is clearer.

> This doesn't need to be defined in the longhands mod.

Same.

> Given this is the only part we really want to be common to both the longhands and shorthands mods, I think it will be clearer not to loop over `["longhands", "shorthands"]`, and instead just keep separate mods and duplicate these three lines.

Agreed.
Comment on attachment 8985273 [details]
Bug 1468651: Rename the directories from "shorthand" to "shorthands", "longhand" to "longhands".

https://reviewboard.mozilla.org/r/250882/#review258066
Attachment #8985273 - Flags: review?(cam) → review+
Comment on attachment 8986143 [details]
Bug 1468651: Rename mask.mako.rs to svg.mako.rs for consistency.

https://reviewboard.mozilla.org/r/251588/#review258068
Attachment #8986143 - Flags: review?(cam) → review+
Comment on attachment 8986144 [details]
Bug 1468651: Make StyleStruct.name_lower snake case.

https://reviewboard.mozilla.org/r/251590/#review258070
Attachment #8986144 - Flags: review?(cam) → review+
Comment on attachment 8986145 [details]
Bug 1468651: Generate different files for different structs.

https://reviewboard.mozilla.org/r/251592/#review258072

Looks great, thanks!
Attachment #8986145 - Flags: review?(cam) → review+
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3b98fb8b713
remove shorthand/serialize.mako.rs. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b23758e71d0
Rename the properties directories from "shorthand" to "shorthands", "longhand" to "longhands". r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/005dba89ec04
Rename mask.mako.rs to svg.mako.rs for consistency. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/378200bd7a02
Make StyleStruct.name_lower snake case. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/471c301d3de6
Generate different files for different structs. r=heycam
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/691fc2e78036
followup: Fix windows build bustage. r=me CLOSED TREE
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21d5712afa0a
followup: Regenerate the devtools database on a CLOSED TREE. r=me
You need to log in before you can comment on or make changes to this bug.