Skip to content

/ servo

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

Introduce and derive ComputeSquaredDistance #18058

Merged
merged 3 commits into from Aug 13, 2017
Merged

Introduce and derive ComputeSquaredDistance #18058

merged 3 commits into from Aug 13, 2017

Conversation

@nox
Copy link
Member

nox commented Aug 12, 2017 •
edited by SimonSapin

This new trait merges the former Animatable methods compute_distance and compute_squared_distance.

This change is Reviewable
nox added 2 commits Aug 12, 2017
@nox
Remove impl_animatable_for_option_tuple
b14e68f
@nox
Introduce ComputeSquaredDistance
51b7400 
This allows us to merge the former Animatable methods compute_distance and
compute_squared_distance, reducing code size.
@highfive
Copy link

highfive commented Aug 12, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/values/generics/text.rs, components/style/values/generics/rect.rs, components/style/values/computed/mod.rs, components/style/values/generics/position.rs, components/style/values/generics/border.rs and 18 more
  • @canaltinova: components/style/values/generics/text.rs, components/style/values/generics/rect.rs, components/style/values/computed/mod.rs, components/style/values/generics/position.rs, components/style/values/generics/border.rs and 18 more
  • @emilio: components/style/values/generics/text.rs, ports/geckolib/glue.rs, components/style/values/generics/rect.rs, components/style/values/computed/mod.rs, components/style/values/generics/position.rs and 19 more
@highfive
Copy link

highfive commented Aug 12, 2017

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!
@nox
Copy link
Member Author

nox commented Aug 12, 2017

r? @emilio
@bors-servo try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e30ab60218fa83f083308c498b05f12ac65e993c
@highfive highfive assigned emilio and unassigned mbrubeck Aug 12, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Aug 12, 2017

⌛ Trying commit f3d589e with merge 2a4eb7f...
bors-servo added a commit that referenced this pull request Aug 12, 2017
@bors-servo
Auto merge of #18058 - servo:compute-squared-distance, r=<try>
Loading status checks…
2a4eb7f 
Introduce and derive ComputeSquaredDistance

This new trait merges the former `Animatable` methods `compute_distance` and `compute_squared_distance`.
@bors-servo
Copy link
Contributor

bors-servo commented Aug 12, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
State: approved= try=True
@nox
Copy link
Member Author

nox commented Aug 13, 2017

It's not clear to me why we need to optimise the case where we only have one distance and we don't want to square it, I think SquaredDistance could just be a wrapper of f64.
@nox
Derive ComputeSquaredDistance
Loading status checks…
277351d
@nox nox force-pushed the compute-squared-distance branch from f3d589e to 277351d Aug 13, 2017
@emilio
emilio approved these changes Aug 13, 2017
View changes
Copy link
Member

emilio left a comment

Looks quite fancy, r=me :)
components/style/values/distance.rs
Some(first) => first,
None => return SquaredDistance::Value(0.),
};
iter.fold(first, Add::add)

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Aug 13, 2017

Member

Can't you do iter.fold(SquaredDistance::Value(0.), Add::add), and remove the match above?

This comment has been minimized.

Sign in to view
Copy link
@nox

nox Aug 13, 2017

Author Member

I'm not doing that to avoid pointlessly transforming a SquaredDistance::Sqrt into SquaredDistance::Value if there was only one distance in the iterator. As I said in a comment of the PR though, I'm not sure we should even bother to try to avoid the final f64::sqrt call, I just did it that way to preserve the functionality but I wouldn't mind just storing the squared one always.
components/style/values/generics/mod.rs
#[cfg_attr(feature = "servo", derive(HeapSizeOf, Deserialize, Serialize))]
#[cfg_attr(feature = "servo", derive(Deserialize, HeapSizeOf, Serialize))]
#[derive(Clone, ComputeSquaredDistance, Copy, Debug, HasViewportPercentage)]
#[derive(PartialEq, PartialOrd, ToComputedValue, ToCss)]

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Aug 13, 2017

Member

NOT ENOUGH DERIVE
components/style/values/computed/length.rs
},
_ => {
// FIXME(nox): Should this return `Ok(SquaredDistance::Value(1.))`
// when `self` and `other` are the same extremum value?

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Aug 13, 2017

Member

I think so, if only for consistency.
components/style/properties/helpers/animated_properties.mako.rs
},
_ => {
// FIXME(nox): Should this return `Ok(SquaredDistance::Value(0.))`
// if `self` and `other` are the same keyword value?

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Aug 13, 2017

Member

Probably, if only for consistency.
components/style/values/computed/length.rs
#[inline]
fn compute_squared_distance(&self, other: &Self) -> Result<SquaredDistance, ()> {
// FIXME(nox): This looks incorrect to me, to add a distance between lengths
// with a distance between percentages.

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Aug 13, 2017

Member

Maybe check with @BorisChiou or @hiikezoe? It does seem slightly fishy...
@nox
Copy link
Member Author

nox commented Aug 13, 2017

@bors-servo r=emilio
@bors-servo
Copy link
Contributor

bors-servo commented Aug 13, 2017

📌 Commit 277351d has been approved by emilio
@bors-servo
Copy link
Contributor

bors-servo commented Aug 13, 2017

⌛ Testing commit 277351d with merge 60c44b0...
bors-servo added a commit that referenced this pull request Aug 13, 2017
@bors-servo
Auto merge of #18058 - servo:compute-squared-distance, r=emilio
Loading status checks…
60c44b0 
Introduce and derive ComputeSquaredDistance

This new trait merges the former `Animatable` methods `compute_distance` and `compute_squared_distance`.

<!-- 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/18058)
<!-- Reviewable:end -->
@nox nox mentioned this pull request Aug 13, 2017
Open
@bors-servo
Copy link
Contributor

bors-servo commented Aug 13, 2017

💔 Test failed - linux-rel-wpt
@nox
Copy link
Member Author

nox commented Aug 13, 2017

@bors-servo retry #6734

wat
@bors-servo
Copy link
Contributor

bors-servo commented Aug 13, 2017

⚡ Previous build results for android, arm32, arm64, linux-dev, windows-msvc-dev are reusable. Rebuilding only linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4...
@bors-servo
Copy link
Contributor

bors-servo commented Aug 13, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: emilio
Pushing 60c44b0 to master...
@bors-servo bors-servo merged commit 277351d into master Aug 13, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@nox nox deleted the compute-squared-distance branch Aug 14, 2017
@hiikezoe hiikezoe mentioned this pull request Mar 3, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilio emilio

Assignees

@emilio emilio

Labels
S-tests-failed
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@nox @highfive @bors-servo @emilio @mbrubeck
You can’t perform that action at this time.