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

style: More serialization tweaks. #20081

Merged
merged 5 commits into from Feb 21, 2018

Conversation

@emilio
Copy link
Member

commented Feb 20, 2018

This still doesn't fix everything. In particular, we need to check whether the
subproperty will be enabled in Longhands and LonghandsToSerialize too.

I haven't decided yet on what's the best way to do that.


This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Feb 20, 2018

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/gecko.mako.rs, ports/geckolib/glue.rs, components/style/properties/properties.mako.rs, components/style/properties/helpers.mako.rs, components/style/properties/declaration_block.rs and 4 more
  • @canaltinova: components/style/properties/gecko.mako.rs, components/style/properties/properties.mako.rs, components/style/properties/helpers.mako.rs, components/style/properties/declaration_block.rs, components/style/dom.rs and 3 more
@highfive

This comment has been minimized.

Copy link

commented Feb 20, 2018

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify style code, but no tests are modified. Please consider adding a test!
@emilio

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2018

r? @nox (last three commits, since the rest is #20078)

@highfive highfive assigned nox and unassigned mbrubeck Feb 20, 2018

@emilio emilio force-pushed the emilio:more-longhand-stuff branch from 2c2f40b to bf8f324 Feb 20, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2018

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

emilio added 3 commits Feb 20, 2018
style: There's no need to check the serialized custom properties.
Given they're not duplicated, and can't be part of a shorthand.
style: Don't try to serialize the same shorthand over and over.
I looked at what were we doing in that loop, and we're doing tons of dumb stuff.

In particular, we try to serialize the "all" shorthand all the time. This patch
prevents us from trying to serialize shorthands that we've already tried to
serialize.

@emilio emilio force-pushed the emilio:more-longhand-stuff branch from e28ca18 to 820e940 Feb 20, 2018

@nox
nox approved these changes Feb 20, 2018
@@ -3811,8 +3811,7 @@ impl<'a> PrioritizedPropertyIter<'a> {
let mut sorted_property_indices: Vec<PropertyAndIndex> =
properties.iter().enumerate().map(|(index, pair)| {
PropertyAndIndex {
property: PropertyId::from_nscsspropertyid(pair.mProperty)
.unwrap_or(all.clone()),
property: PropertyId::from_nscsspropertyid(pair.mProperty).unwrap_or(all.clone()),

This comment has been minimized.

Copy link
@nox

nox Feb 20, 2018

Member

It's not actually important because that clone is a no op, but maybe we should define all in the loop and avoid the clone, because it looked expensive to me before I checked.

@jdm jdm removed the S-awaiting-review label Feb 20, 2018

emilio added 2 commits Feb 20, 2018
style: Remove unneeded whitespace.
I bet it was added when this code was part of a mako file.

@emilio emilio changed the title style: Only expose longhands and shorthands to rust via iterators. style: More serialization tweaks. Feb 20, 2018

@emilio

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2018

So this is red because we have internal properties that with this patch we don't set properly... So I'm going to remove the iterator stuff for now and figure out what the right thing to do is...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=64633d48ff1414ae76f8dc59f1f67a94a385b80c

@emilio emilio force-pushed the emilio:more-longhand-stuff branch from 4d6c24d to a4c7728 Feb 20, 2018

@emilio

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2018

@bors-servo r=nox

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2018

📌 Commit a4c7728 has been approved by nox

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2018

⌛️ Testing commit a4c7728 with merge a478f6a...

bors-servo added a commit that referenced this pull request Feb 20, 2018
Auto merge of #20081 - emilio:more-longhand-stuff, r=nox
style: More serialization tweaks.

This still doesn't fix everything. In particular, we need to check whether the
subproperty will be enabled in Longhands and LonghandsToSerialize too.

I haven't decided yet on what's the best way to do that.

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

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2018

💔 Test failed - windows-msvc-dev

@Manishearth

This comment has been minimized.

Copy link
Member

commented Feb 20, 2018

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2018

⌛️ Testing commit a4c7728 with merge 2c060eb...

bors-servo added a commit that referenced this pull request Feb 21, 2018
Auto merge of #20081 - emilio:more-longhand-stuff, r=nox
style: More serialization tweaks.

This still doesn't fix everything. In particular, we need to check whether the
subproperty will be enabled in Longhands and LonghandsToSerialize too.

I haven't decided yet on what's the best way to do that.

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

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2018

@bors-servo bors-servo merged commit a4c7728 into servo:master Feb 21, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
bors-servo added a commit that referenced this pull request Feb 27, 2018
Auto merge of #20134 - emilio:split-stuff, r=nox
style: Split out NonCustomPropertyId::enabled_for_all_content from allowed_in.

This is part of a patch that was reviewed by nox in #20081, but which I reverted because that approach didn't quite work.

I think I have something that works now, but I'm waiting for a Geckotry. Landing this should be worth it in the meantime though.

<!-- 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/20134)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.