Renamed the style structs #10324

perlun · 2016-03-31T20:48:10Z

Renamed style structs.

The idea is to rename all style structs from Foo to ServoFoo, as described out in #10185.

This change is

highfive · 2016-03-31T20:48:17Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties.mako.rs
@SimonSapin: components/style/properties.mako.rs

highfive · 2016-03-31T20:48:18Z

Warning

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

Manishearth · 2016-03-31T20:48:56Z

r? @bholley

perlun · 2016-03-31T20:49:11Z

components/style/properties.mako.rs

@@ -16,7 +16,7 @@ use std::mem;
 use std::sync::Arc;

 use app_units::Au;
-use cssparser::{Parser, Color, RGBA, AtRuleParser, DeclarationParser, Delimiter,
+use cssparser::{Parser, RGBA, AtRuleParser, DeclarationParser, Delimiter,


This is just me trying to work around name clashes; it may be that removing this causes the generated code to be completely wrong (meaning, that references to cssparser::Color might get incorrectly replaced by our local Color struct...). So this may very well be one of the parts that should get reverted.

perlun · 2016-04-01T06:10:14Z

No rush on reviewing this one - I have a followup coming up, where I remove (postpone) the trait change. Leave this pending for a little while; will let you know when I've amended the previous commit (and then also, it hopefully builds on Travis).

perlun · 2016-04-01T19:33:32Z

@bholley - I managed to sort this out to properly build (both with ./mach build and ./mach build-geckolib), by reverting the trait rename (that caused conflicts). PR text has been adjusted, this should be reviewable now.

perlun · 2016-04-01T19:35:55Z

ports/geckolib/build.rs

@@ -62,8 +62,7 @@ try:
    style_template.render()

    geckolib_template = Template(filename=os.environ['GECKOLIB_TEMPLATE'], input_encoding='utf8')
-    output = geckolib_template.render(STYLE_STRUCTS = style_template.module.STYLE_STRUCTS,
-                                      LONGHANDS = style_template.module.LONGHANDS)


I looked around and couldn't find any actual usage of the LONGHANDS constant, so I dropped it.

bholley · 2016-04-03T06:52:58Z

Reviewed 4 of 8 files at r2.
Review status: 4 of 8 files reviewed at latest revision, 5 unresolved discussions.

components/gfx/font.rs, line 19 [r2] (raw file):
I think that keeping the FontStyle rename here probably makes this more understandable.

components/layout/text.rs, line 27 [r2] (raw file):
Same here.

components/style/properties.mako.rs, line 19 [r1] (raw file):
Looks like this got reverted?

components/style/properties.mako.rs, line 99 [r1] (raw file):
The downside of doing this is that the struct name is different for gecko and servo. And making it servo_struct_name and gecko_struct_name would be confusing, since we already have gecko_name.

Do you have any ideas on how to improve this?

Comments from Reviewable

bholley · 2016-04-03T06:54:01Z

(Not finished with the review, but I need to go to bed, and thought I might as well post the feedback I have. I'll get back to this tomorrow or monday).

bholley · 2016-04-03T06:57:24Z

(one solution to the naming weirdness could be servo_struct_name, gecko_struct_name, and gecko_ffi_name). Or something.

perlun · 2016-04-03T19:50:32Z

I think that would be a reasonable workaround. That would make gecko_name more clear actually, so maybe we go with that.

(I'll wait with implementing the changes until you've reviewed it all, and do all cleanups in one run.)

Review status: 4 of 8 files reviewed at latest revision, 5 unresolved discussions.

components/gfx/font.rs, line 19 [r2] (raw file):
Are you sure? Could you share some more details on your thinking here? You feel that pub type SpecifiedFontStyle = ServoFont doesn't get clear enough?

My gut feeling is that if we need to alias the types when we use them, the struct name isn't clear enough... But I guess we wouldn't want to call them all ServoFontStyle, ServoBorderStyle etc etc...

components/style/properties.mako.rs, line 19 [r1] (raw file):
Yes. My guess is that Reviewable don't (yet) properly handle commits being amended, branches being force-pushed etc. - in the GitHub PR viewer, that comment from me would be filtered out and considered obsolete (which it is).

Comments from Reviewable

bors-servo · 2016-04-04T15:48:58Z

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

perlun · 2016-04-04T20:34:44Z

@bholley - I rebased the PR now, and incorporated some of your suggestions (amended the previous commit). Feel free to re-check it if you like.

bholley · 2016-04-04T23:09:11Z

Reviewed 2 of 8 files at r2, 4 of 4 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

components/gfx/font.rs, line 19 [r2] (raw file):
'Servo' isn't useful in gfx (where there's no Gecko), and 'Style' is useful (since we're not in the style component). So I'm saying that 'FontStyle' is a more useful name than 'ServoFont' in gfx code.

SpecifiedFontStyle is a fine name in this context, FWIW. Given that that's the only usage, maybe we should just make this:

pub use style::properties::style_structs::ServoFont as SpecifiedFontStyle?

Though actually, that also seems wrong because the style struct is computed style, not specified style. So we should probably call it ComputedFontStyle. @pcwalton, can you confirm that this makes sense?

components/layout/text.rs, line 27 [r2] (raw file):
In general, I wouldn't mind doing more local aliases in layout to make the impact of the Servo prefix less-noticeable. |ServoFont as FontStyle|, |ServoBorder as BorderStyle|, etc. Only if you feel like it though.

components/style/properties.mako.rs, line 6124 [r3] (raw file):
There's another patch coming to remove the |T| here right?

Comments from Reviewable

perlun · 2016-04-05T19:47:09Z

I think I'll wait until finalization on the discussions around the naming before completing this on.

Review status: all files reviewed at latest revision, 4 unresolved discussions.

components/style/properties.mako.rs, line 6124 [r3] (raw file):
Yes. I reverted those parts, since it also seemed to cause conflicts with other types (so that we need to alias either one of the types to get it working properly). Like you wrote in the original issue: we should take it one step at a time, to ensure we don't break things. 😄

Comments from Reviewable

bholley · 2016-04-05T23:46:21Z

Ok. needinfo @pcwalton, @SimonSapin, or @mbrubeck about renaming SpecifiedFontStyle to ComputedFontStyle.

SimonSapin · 2016-04-06T07:26:31Z

Yes, every value that layout/gfx gets for CSS properties from the style crate is a "computed value". (In some cases they may go on to calculating "used values" themselves, e.g. percentage widths.) "Specified values" mostly don’t exist outside of the style crate or CSSOM.

https://drafts.csswg.org/css-cascade/#value-stages

So ComputedFontStyle is correct, and dropping the prefix with just FontStyle is also ok since there is no other kind of font-related property values in that context (as far as specs are concerned).

bholley · 2016-04-06T21:06:10Z

@perlun Does that give you what you need to push this over the line?

bors-servo · 2016-04-06T22:30:13Z

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

perlun · 2016-04-07T19:33:22Z

Does that give you what you need to push this over the line?

Yep, sounds good! Will look it into the next few days. I am not (yet! 😉) working for Mozilla full-time so that's why it's taking a bit more time.

bholley · 2016-04-07T23:15:06Z

Of course - let me know if you need anything. This may need to be rebased over various changes that are landing simultaneously, but hopefully it shouldn't be too bad. :-)

The idea is to rename all style structs from Foo to ServoFoo, as described out in servo#10185.

perlun · 2016-04-08T19:29:11Z

@bholley - should be good to go now. Please re-check whenever you have some time. I'll fix the drop-the-T-prefix part once this is landed.

bholley · 2016-04-08T19:52:18Z

Reviewed 5 of 6 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

components/gfx/font.rs, line 19 [r2] (raw file):
Didn't we conclude above that we want to rename SpecifiedFontStyle to ComputedFontStyle?

Comments from Reviewable

bholley · 2016-04-08T19:54:26Z

Though honestly I think we should just get this landed - We'll deal with the rest in the followup.

@bors-servo r+

bors-servo · 2016-04-08T19:54:27Z

📌 Commit 96835c6 has been approved by bholley

bors-servo · 2016-04-08T19:54:29Z

⌛ Testing commit 96835c6 with merge 4da38cd...

Renamed the style structs Renamed style structs. The idea is to rename all style structs from Foo to ServoFoo, as described out in #10185.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10324)

perlun · 2016-04-08T20:19:34Z

Didn't we conclude above that we want to rename SpecifiedFontStyle to ComputedFontStyle?

Ah, you're right... @SimonSapin said either go with ComputedFontStyle or just FontStyle. But like you say we deal that in the followup. Thanks for the fast review! 👍

bors-servo · 2016-04-08T20:36:08Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt

bholley · 2016-04-08T20:49:46Z

Thanks for the contribution! :-)

highfive assigned Manishearth Mar 31, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 31, 2016

highfive assigned bholley and unassigned Manishearth Mar 31, 2016

perlun reviewed Mar 31, 2016
View reviewed changes

perlun force-pushed the rename-style-structs branch from 5f45265 to 5db316f Compare April 1, 2016 19:31

perlun changed the title ~~WIP trying to rename the style structs.~~ Renamed the style structs Apr 1, 2016

perlun reviewed Apr 1, 2016
View reviewed changes

highfive added the S-needs-rebase There are merge conflict errors. label Apr 4, 2016

perlun force-pushed the rename-style-structs branch from 5db316f to 0e758e0 Compare April 4, 2016 20:33

KiChjang removed the S-needs-rebase There are merge conflict errors. label Apr 4, 2016

highfive added the S-needs-rebase There are merge conflict errors. label Apr 6, 2016

Renamed style structs.

96835c6

The idea is to rename all style structs from Foo to ServoFoo, as described out in servo#10185.

perlun force-pushed the rename-style-structs branch from 0e758e0 to 96835c6 Compare April 8, 2016 18:48

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Apr 8, 2016

bors-servo merged commit 96835c6 into servo:master Apr 8, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 8, 2016

perlun deleted the rename-style-structs branch April 8, 2016 20:48

wafflespeanut mentioned this pull request Apr 27, 2016

Rename style struct types to eliminate the T #10185

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renamed the style structs #10324

Renamed the style structs #10324

perlun commented Mar 31, 2016

highfive commented Mar 31, 2016

highfive commented Mar 31, 2016

Manishearth commented Mar 31, 2016

perlun Mar 31, 2016

perlun commented Apr 1, 2016

perlun commented Apr 1, 2016

perlun Apr 1, 2016

bholley commented Apr 3, 2016

bholley commented Apr 3, 2016

bholley commented Apr 3, 2016

perlun commented Apr 3, 2016

bors-servo commented Apr 4, 2016

perlun commented Apr 4, 2016

bholley commented Apr 4, 2016

perlun commented Apr 5, 2016

bholley commented Apr 5, 2016

SimonSapin commented Apr 6, 2016

bholley commented Apr 6, 2016

bors-servo commented Apr 6, 2016

perlun commented Apr 7, 2016

bholley commented Apr 7, 2016

perlun commented Apr 8, 2016

bholley commented Apr 8, 2016

bholley commented Apr 8, 2016

bors-servo commented Apr 8, 2016

bors-servo commented Apr 8, 2016

perlun commented Apr 8, 2016

bors-servo commented Apr 8, 2016

bholley commented Apr 8, 2016

Renamed the style structs #10324

Renamed the style structs #10324

Conversation

perlun commented Mar 31, 2016

highfive commented Mar 31, 2016

highfive commented Mar 31, 2016

Manishearth commented Mar 31, 2016

perlun Mar 31, 2016

Choose a reason for hiding this comment

perlun commented Apr 1, 2016

perlun commented Apr 1, 2016

perlun Apr 1, 2016

Choose a reason for hiding this comment

bholley commented Apr 3, 2016

bholley commented Apr 3, 2016

bholley commented Apr 3, 2016

perlun commented Apr 3, 2016

bors-servo commented Apr 4, 2016

perlun commented Apr 4, 2016

bholley commented Apr 4, 2016

perlun commented Apr 5, 2016

bholley commented Apr 5, 2016

SimonSapin commented Apr 6, 2016

bholley commented Apr 6, 2016

bors-servo commented Apr 6, 2016

perlun commented Apr 7, 2016

bholley commented Apr 7, 2016

perlun commented Apr 8, 2016

bholley commented Apr 8, 2016

bholley commented Apr 8, 2016

bors-servo commented Apr 8, 2016

bors-servo commented Apr 8, 2016

perlun commented Apr 8, 2016

bors-servo commented Apr 8, 2016

bholley commented Apr 8, 2016