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 uncompute functionality (WIP) #13228

Merged
merged 3 commits into from Sep 23, 2016

Conversation

Projects
None yet
5 participants
@Manishearth
Member

Manishearth commented Sep 12, 2016

As discussed in Taipei we plan to do animations in Stylo on the Rust side. For cascading properly, we need to "uncompute" these,
i.e. convert them into a cascadeable specified value which when computed gets us the same thing again.

This patch starts work on this. Before writing uncompute code for everything, I'd like to check that this is an acceptable amount of mako magic,
and that the general design is okay (to avoid having to rewrite everything once it's done).

Preliminary r? @SimonSapin @birtles


This change is Reviewable

@highfive

This comment has been minimized.

Show comment
Hide comment
@highfive

highfive Sep 12, 2016

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/longhand/background.mako.rs, components/style/properties/helpers.mako.rs, components/style/properties/longhand/box.mako.rs, components/style/values/computed/mod.rs, components/style/properties/longhand/color.mako.rs

highfive commented Sep 12, 2016

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/longhand/background.mako.rs, components/style/properties/helpers.mako.rs, components/style/properties/longhand/box.mako.rs, components/style/values/computed/mod.rs, components/style/properties/longhand/color.mako.rs
@highfive

This comment has been minimized.

Show comment
Hide comment
@highfive

highfive Sep 12, 2016

warning Warning warning

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

highfive commented Sep 12, 2016

warning Warning warning

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

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Sep 13, 2016

Member

r? @heycam since Simon is busy this month

Member

Manishearth commented Sep 13, 2016

r? @heycam since Simon is busy this month

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Sep 14, 2016

Member

For cascading properly, we need to "uncompute"

We talked about this last week, but I think I didn’t really understand it: why do we need to uncompute? It seems to me that that operation shouldn’t be necessary, but I haven’t thought it all through.

Member

SimonSapin commented Sep 14, 2016

For cascading properly, we need to "uncompute"

We talked about this last week, but I think I didn’t really understand it: why do we need to uncompute? It seems to me that that operation shouldn’t be necessary, but I haven’t thought it all through.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Sep 14, 2016

Member

(discussed in IRC -- you need to recascade once you interpolate because of !important, so you need to go back to specified form)

Member

Manishearth commented Sep 14, 2016

(discussed in IRC -- you need to recascade once you interpolate because of !important, so you need to go back to specified form)

@heycam

This comment has been minimized.

Show comment
Hide comment
@heycam

heycam Sep 21, 2016

Member

Sorry for only getting a chance to look at this now. I can understand wanting to eliminate the similar looking code we need to write to to_computed_value and uncompute, but looking at the couple of impl_to_computed_value uses you add in this patch, I find them quite hard to read. To me it obfuscates the how these conversions are performed and how the specified and computed value types relate to each other. I think I would prefer to see these written out by hand.

Another small comment: I never really liked the term "uncompute" much. Can we call the function "to_specified_value" instead? It feels a little odd for that function to be on the trait named ToComputedValue; maybe there's a better, more generic name for that trait we can use.

Why does ToComputedValue need to require Sized?

Member

heycam commented Sep 21, 2016

Sorry for only getting a chance to look at this now. I can understand wanting to eliminate the similar looking code we need to write to to_computed_value and uncompute, but looking at the couple of impl_to_computed_value uses you add in this patch, I find them quite hard to read. To me it obfuscates the how these conversions are performed and how the specified and computed value types relate to each other. I think I would prefer to see these written out by hand.

Another small comment: I never really liked the term "uncompute" much. Can we call the function "to_specified_value" instead? It feels a little odd for that function to be on the trait named ToComputedValue; maybe there's a better, more generic name for that trait we can use.

Why does ToComputedValue need to require Sized?

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Sep 21, 2016

Member

Finished all the remaining properties.

Debating on to_specified_value vs from_computed_value (since the function is implemented on the specified value).

Seems like it doesn't need Sized, not sure why I added it.

r? @heycam @birtles

Member

Manishearth commented Sep 21, 2016

Finished all the remaining properties.

Debating on to_specified_value vs from_computed_value (since the function is implemented on the specified value).

Seems like it doesn't need Sized, not sure why I added it.

r? @heycam @birtles

Show outdated Hide outdated components/style/values/computed/mod.rs
Show outdated Hide outdated components/style/values/computed/mod.rs
Show outdated Hide outdated components/style/values/computed/mod.rs
fn has_viewport_percentage(&self) -> bool {
return self.width.has_viewport_percentage() || self.height.has_viewport_percentage();
}
}
#[derive(Clone, PartialEq, Debug)]
#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
pub struct SpecifiedExplicitSize {
pub struct ExplicitSize {

This comment has been minimized.

@heycam

heycam Sep 22, 2016

Member

(This renaming seems logically separate from the to_specified_value functionality so probably would have been better in a separate commit.)

@heycam

heycam Sep 22, 2016

Member

(This renaming seems logically separate from the to_specified_value functionality so probably would have been better in a separate commit.)

Show outdated Hide outdated components/style/values/computed/mod.rs
computed_value::T(self.0.to_computed_value(context))
}
}
impl ComputedValueAsSpecified for SpecifiedValue {}

This comment has been minimized.

@heycam

heycam Sep 22, 2016

Member

(This and the other changes to use ComputedValueAsSpecified where appropriate too seem like logically separate changes that would have been worth doing in a separate commit, underneath this one that adds to_specified_value.)

@heycam

heycam Sep 22, 2016

Member

(This and the other changes to use ComputedValueAsSpecified where appropriate too seem like logically separate changes that would have been worth doing in a separate commit, underneath this one that adds to_specified_value.)

Show outdated Hide outdated components/style/properties/longhand/effects.mako.rs
Show outdated Hide outdated components/style/properties/longhand/effects.mako.rs
Show outdated Hide outdated components/style/properties/longhand/inherited_text.mako.rs
Show outdated Hide outdated components/style/values/specified/basic_shape.rs
@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Sep 22, 2016

Member

Addressed. Split up the commits as well.

Member

Manishearth commented Sep 22, 2016

Addressed. Split up the commits as well.

@heycam

This comment has been minimized.

Show comment
Hide comment
@heycam

heycam Sep 23, 2016

Member

@bors-servo delegate+

(In case you want to wait for review from @birtles.)

Member

heycam commented Sep 23, 2016

@bors-servo delegate+

(In case you want to wait for review from @birtles.)

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 23, 2016

Contributor

✌️ @Manishearth can now approve this pull request

Contributor

bors-servo commented Sep 23, 2016

✌️ @Manishearth can now approve this pull request

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth
Member

Manishearth commented Sep 23, 2016

@bors-servo r=heycam

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 23, 2016

Contributor

📌 Commit 0ec3845 has been approved by heycam

Contributor

bors-servo commented Sep 23, 2016

📌 Commit 0ec3845 has been approved by heycam

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 23, 2016

Contributor

⌛️ Testing commit 0ec3845 with merge 121c8ca...

Contributor

bors-servo commented Sep 23, 2016

⌛️ Testing commit 0ec3845 with merge 121c8ca...

bors-servo added a commit that referenced this pull request Sep 23, 2016

Auto merge of #13228 - Manishearth:uncompute, r=heycam
Add uncompute functionality (WIP)

As discussed in Taipei we plan to do animations in Stylo on the Rust side. For cascading properly, we need to "uncompute" these,
i.e. convert them into a cascadeable specified value which when computed gets us the same thing again.

This patch starts work on this. Before writing uncompute code for everything, I'd like to check that this is an acceptable amount of mako magic,
and that the general design is okay (to avoid having to rewrite everything once it's done).

Preliminary r? @SimonSapin @birtles

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

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 23, 2016

Contributor

💔 Test failed - linux-dev

Contributor

bors-servo commented Sep 23, 2016

💔 Test failed - linux-dev

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth
Member

Manishearth commented Sep 23, 2016

@bors-servo r=heycam

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 23, 2016

Contributor

📌 Commit d81c6af has been approved by heycam

Contributor

bors-servo commented Sep 23, 2016

📌 Commit d81c6af has been approved by heycam

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 23, 2016

Contributor

⌛️ Testing commit d81c6af with merge fb52bb7...

Contributor

bors-servo commented Sep 23, 2016

⌛️ Testing commit d81c6af with merge fb52bb7...

bors-servo added a commit that referenced this pull request Sep 23, 2016

Auto merge of #13228 - Manishearth:uncompute, r=heycam
Add uncompute functionality (WIP)

As discussed in Taipei we plan to do animations in Stylo on the Rust side. For cascading properly, we need to "uncompute" these,
i.e. convert them into a cascadeable specified value which when computed gets us the same thing again.

This patch starts work on this. Before writing uncompute code for everything, I'd like to check that this is an acceptable amount of mako magic,
and that the general design is okay (to avoid having to rewrite everything once it's done).

Preliminary r? @SimonSapin @birtles

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

This comment has been minimized.

Show comment
Hide comment
@bors-servo
Contributor

bors-servo commented Sep 23, 2016

@bors-servo bors-servo merged commit d81c6af into servo:master Sep 23, 2016

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment