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

Make serialization match Gecko in a few corner cases #18352

Merged
merged 2 commits into from Sep 13, 2017

Conversation

Projects
None yet
5 participants
@jdm
Copy link
Member

commented Sep 1, 2017

This addresses the testcases from https://bugzilla.mozilla.org/show_bug.cgi?id=1345218. Gecko differs from the specification by doing the following:

  • reusing longhands that have previously been serialized in order to serialize shorthands
  • immediately breaking out of the shorthand loop for the current property as soon as a shorthand is successfully serialized

w3c/csswg-drafts#1774 is filed to track ways that the standard could be made more clear on these points.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix bug 1345218.
  • There are tests for these changes

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Sep 1, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/declaration_block.rs, components/style/properties/properties.mako.rs
  • @canaltinova: components/style/properties/declaration_block.rs, components/style/properties/properties.mako.rs
  • @emilio: components/style/properties/declaration_block.rs, components/style/properties/properties.mako.rs
@jdm

This comment has been minimized.

@emilio
Copy link
Member

left a comment

Note that this loop happens to be quite hot for Servo (#17315), so maybe it's worth to spend a bit more time to avoid that vector? Longhands aren't that uncommon.

// If the set of previously serialized shorthands contains any of the shorthands
// that are associated with the current longhand, we can skip it since its value
// has already been included in the serialization of the related shorthand.
if shorthands.iter().any(|s| shorthands_used.iter().any(|used| used == s)) {

This comment has been minimized.

Copy link
@emilio

emilio Sep 1, 2017

Member

Isn't this quadratic? :(

This comment has been minimized.

Copy link
@emilio

emilio Sep 1, 2017

Member

Can we instead use something like PropertyDeclarationIdSet, but for shorthands instead?

@@ -751,6 +755,7 @@ impl ToCss for PropertyDeclarationBlock {
importance,
&mut is_first_serialization)?;
} else {
shorthands_used.push(shorthand);

This comment has been minimized.

Copy link
@emilio

emilio Sep 1, 2017

Member

I believe it would be enough to add all the longhands in shorthand.longhands() to already_serialized, wouldn't it?

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2017

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

@jdm jdm force-pushed the jdm:serialize-fun branch from cf806bd to d67e2bd Sep 12, 2017

@jdm

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2017

It turns out that the shorthand check was completely unnecessary (and broke system font serialization). The other commits are unchanged and all tests pass: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6da6d7ff309b88f7646c8114f921c36ba8057ef3

@jdm jdm assigned emilio and unassigned KiChjang Sep 12, 2017

@emilio

emilio approved these changes Sep 13, 2017

@@ -673,10 +673,6 @@ impl ToCss for PropertyDeclarationBlock {
}
} else {
for (longhand, importance) in self.declaration_importance_iter() {
if already_serialized.contains(longhand.id()) {

This comment has been minimized.

Copy link
@emilio

emilio Sep 13, 2017

Member

Do we need to remove the same check in the system font loop?

(I don't remember why we have the special path for the system font in the first place...)

This comment has been minimized.

Copy link
@jdm

jdm Sep 13, 2017

Author Member

It is redundant, yes.

@jdm jdm force-pushed the jdm:serialize-fun branch from d67e2bd to 5b83bea Sep 13, 2017

@jdm

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2017

@bors-servo: r=emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

📌 Commit 5b83bea has been approved by emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

⌛️ Testing commit 5b83bea with merge e50341d...

bors-servo added a commit that referenced this pull request Sep 13, 2017

Auto merge of #18352 - jdm:serialize-fun, r=emilio
Make serialization match Gecko in a few corner cases

This addresses the testcases from https://bugzilla.mozilla.org/show_bug.cgi?id=1345218. Gecko differs from the specification by doing the following:
* reusing longhands that have previously been serialized in order to serialize shorthands
* immediately breaking out of the shorthand loop for the current property as soon as a shorthand is successfully serialized

w3c/csswg-drafts#1774 is filed to track ways that the standard could be made more clear on these points.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix [bug 1345218](https://bugzilla.mozilla.org/show_bug.cgi?id=1345218).
- [X] There are tests for these changes

<!-- 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/18352)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

@bors-servo bors-servo merged commit 5b83bea into servo:master Sep 13, 2017

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
You can’t perform that action at this time.