Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add aliases for geckolib for supported properties #14939

Merged
merged 4 commits into from Jan 11, 2017

Conversation

@upsuper
Copy link
Member

commented Jan 10, 2017

This fixes ~7.5k failures in style system mochitests.

r? @Manishearth


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because it is for geckolib only

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Jan 10, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/longhand/inherited_text.mako.rs, components/style/properties/longhand/effects.mako.rs, components/style/properties/longhand/margin.mako.rs, components/style/properties/shorthand/box.mako.rs, components/style/properties/shorthand/border.mako.rs, components/style/properties/data.py, components/style/properties/longhand/background.mako.rs, components/style/properties/shorthand/column.mako.rs, components/style/properties/longhand/column.mako.rs, components/style/properties/shorthand/mask.mako.rs, components/style/properties/shorthand/position.mako.rs, components/style/properties/longhand/ui.mako.rs, components/style/properties/longhand/padding.mako.rs, components/style/properties/longhand/font.mako.rs, components/style/properties/longhand/position.mako.rs, components/style/properties/longhand/border.mako.rs, components/style/properties/longhand/svg.mako.rs, components/style/properties/longhand/box.mako.rs, components/style/properties/properties.mako.rs, components/style/properties/longhand/xul.mako.rs
  • @emilio: components/style/properties/longhand/inherited_text.mako.rs, components/style/properties/longhand/effects.mako.rs, components/style/properties/longhand/margin.mako.rs, components/style/properties/shorthand/box.mako.rs, components/style/properties/shorthand/border.mako.rs, components/style/properties/data.py, components/style/properties/longhand/background.mako.rs, components/style/properties/shorthand/column.mako.rs, components/style/properties/longhand/column.mako.rs, components/style/properties/shorthand/mask.mako.rs, components/style/properties/shorthand/position.mako.rs, components/style/properties/longhand/ui.mako.rs, components/style/properties/longhand/padding.mako.rs, components/style/properties/longhand/font.mako.rs, components/style/properties/longhand/position.mako.rs, components/style/properties/longhand/border.mako.rs, components/style/properties/longhand/svg.mako.rs, components/style/properties/longhand/box.mako.rs, components/style/properties/properties.mako.rs, components/style/properties/longhand/xul.mako.rs
@highfive

This comment has been minimized.

Copy link

commented Jan 10, 2017

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!
@@ -220,12 +230,19 @@ def new_style_struct(self, *args, **kwargs):
def active_style_structs(self):
return [s for s in self.style_structs if s.additional_methods or s.longhands]

def add_prefixed_aliases(self, property_data):
# FIXME Servo's DOM architecture doesn't support vendor-prefixed properties.

This comment has been minimized.

Copy link
@emilio

emilio Jan 10, 2017

Member

What do you mean by this? We should be able to treat them the same way while parsing, right?

This comment has been minimized.

Copy link
@upsuper

upsuper Jan 10, 2017

Author Member

See CSSStyleDeclaration.webidl, there are attributes for properties as their original form. For vendor-prefixed properties, that would be something like attribute DOMString -webkit-foo;. However, name starting with - is invalid in WebIDL.

This comment has been minimized.

Copy link
@upsuper

upsuper Jan 10, 2017

Author Member

FWIW, Gecko prepends _ to property names in WebIDL files. See CSS2Properties.webidl. There is probably some magic in the codegen to handle this.

This comment has been minimized.

Copy link
@emilio

emilio Jan 10, 2017

Member

Oh, ok, I see, I'll file an issue.

We probably need to auto-generate that webidl file eventually anyway.

This comment has been minimized.

Copy link
@emilio

emilio Jan 10, 2017

Member

Can you reference #14941 from that comment?

This comment has been minimized.

Copy link
@upsuper

upsuper Jan 10, 2017

Author Member

OK, will do tomorrow.

This comment has been minimized.

Copy link
@Manishearth

Manishearth Jan 10, 2017

Member

FWIW Gecko has a bug here where CSSSD can be indexed with webkit-foo without the preceding dash.

This comment has been minimized.

Copy link
@Manishearth

Manishearth Jan 10, 2017

Member

Also, for future reference we do not want moz or unspecced webkit prefixes in Servo unless absolutely necessary. Only prefixes from the compat spec (and change the spec if a property seems necessary). We may want to split this attribute into two or use ifs at the usage site when we start thinking about servo.

@@ -135,7 +144,7 @@ def __init__(self, style_struct, name, spec=None, animatable=None, derived_from=

class Shorthand(object):
def __init__(self, name, sub_properties, spec=None, experimental=False, internal=False,
allowed_in_keyframe_block=True, alias=None):
allowed_in_keyframe_block=True, alias=None, prefixes=None):

This comment has been minimized.

Copy link
@emilio

emilio Jan 10, 2017

Member

I'd really like this to be called extra_prefixes instead of just prefixes, but don't worry if it's too much churn.

This comment has been minimized.

Copy link
@upsuper

upsuper Jan 10, 2017

Author Member

extra_prefixes makes sense to me. I can change it to that tomorrow.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2017

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

@emilio

This comment has been minimized.

Copy link
Member

commented Jan 10, 2017

You can pretty much r=me when the comments are addressed and this is rebased.

Copy link
Member

left a comment

r=me with our comments addressed

@@ -220,12 +230,19 @@ def new_style_struct(self, *args, **kwargs):
def active_style_structs(self):
return [s for s in self.style_structs if s.additional_methods or s.longhands]

def add_prefixed_aliases(self, property_data):
# FIXME Servo's DOM architecture doesn't support vendor-prefixed properties.

This comment has been minimized.

Copy link
@Manishearth

Manishearth Jan 10, 2017

Member

FWIW Gecko has a bug here where CSSSD can be indexed with webkit-foo without the preceding dash.

@@ -220,12 +230,19 @@ def new_style_struct(self, *args, **kwargs):
def active_style_structs(self):
return [s for s in self.style_structs if s.additional_methods or s.longhands]

def add_prefixed_aliases(self, property_data):

This comment has been minimized.

Copy link
@Manishearth

Manishearth Jan 10, 2017

Member

nit: Probably just call it property to be consistent

@@ -14,6 +14,14 @@
ALL_SIZES = [(size, False) for size in PHYSICAL_SIZES] + [(size, True) for size in LOGICAL_SIZES]


def maybe_moz_logical_alias(product, side, prop):

This comment has been minimized.

Copy link
@Manishearth

Manishearth Jan 10, 2017

Member

Please leave a comment here that inline-start and -end props can omit the inline.

This comment has been minimized.

Copy link
@upsuper

upsuper Jan 11, 2017

Author Member

The legacy moz-prefixed logical properties are only for inline axis. I guess this can be added to the comment.

@@ -220,12 +230,19 @@ def new_style_struct(self, *args, **kwargs):
def active_style_structs(self):
return [s for s in self.style_structs if s.additional_methods or s.longhands]

def add_prefixed_aliases(self, property_data):
# FIXME Servo's DOM architecture doesn't support vendor-prefixed properties.

This comment has been minimized.

Copy link
@Manishearth

Manishearth Jan 10, 2017

Member

Also, for future reference we do not want moz or unspecced webkit prefixes in Servo unless absolutely necessary. Only prefixes from the compat spec (and change the spec if a property seems necessary). We may want to split this attribute into two or use ifs at the usage site when we start thinking about servo.

@Manishearth

This comment has been minimized.

Copy link
Member

commented Jan 10, 2017

Should these mochitests still be relying on prefixes, btw? Sounds like too much effort to fix right now, but worth filing a bug for.

(A sweep through these might also help us identify tests which don't actually need to be mochitests and can be moved to csswg)

@bholley

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2017

Are they "relying on prefixes", or just testing all the prefixes that we ship? If the latter, the issue is "should we be shipping these prefixes", which is a broader policy question (and generally a complicated one, owned by the web-compat team).

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2017

Right, those tests are just testing all the aliases Gecko currently ships.

@Manishearth

This comment has been minimized.

Copy link
Member

commented Jan 11, 2017

@bors-servo delegate+

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2017

✌️ @upsuper can now approve this pull request

@upsuper upsuper force-pushed the upsuper:add-aliases branch from f533c50 to 8a8ef48 Jan 11, 2017
@upsuper

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2017

@bors-servo r=emilio,Manishearth

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2017

📌 Commit 8a8ef48 has been approved by emilio,Manishearth

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2017

⌛️ Testing commit 8a8ef48 with merge 04a3242...

bors-servo added a commit that referenced this pull request Jan 11, 2017
Add aliases for geckolib for supported properties

<!-- Please describe your changes on the following line: -->
This fixes ~7.5k failures in style system mochitests.

r? @Manishearth

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because it is for geckolib only

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14939)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

@bors-servo bors-servo merged commit 8a8ef48 into servo:master Jan 11, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.