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)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
Details
Attachments
(5 files)
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
This will cut down properties.rs by a lot, which will make easier to debug, hopefully.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•6 years ago
|
||
mozreview-review |
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 5•6 years ago
|
||
mozreview-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-
Assignee | ||
Comment 6•6 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•6 years ago
|
||
mozreview-review |
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 15•6 years ago
|
||
mozreview-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 16•6 years ago
|
||
mozreview-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 17•6 years ago
|
||
mozreview-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+
Comment 18•6 years ago
|
||
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
Comment 19•6 years ago
|
||
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/mozilla-inbound/rev/691fc2e78036 followup: Fix windows build bustage. r=me CLOSED TREE
Comment 20•6 years ago
|
||
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
Comment 21•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e3b98fb8b713 https://hg.mozilla.org/mozilla-central/rev/6b23758e71d0 https://hg.mozilla.org/mozilla-central/rev/005dba89ec04 https://hg.mozilla.org/mozilla-central/rev/378200bd7a02 https://hg.mozilla.org/mozilla-central/rev/471c301d3de6 https://hg.mozilla.org/mozilla-central/rev/691fc2e78036 https://hg.mozilla.org/mozilla-central/rev/21d5712afa0a
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•