Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upSkip adding/accumulating ClipRect values which corresponding rect offset is auto. #18210
Conversation
|
This patche have been reviewed by @hiikezoe , @Manishearth , @birtles |
|
@bors-servo delegate+ |
|
|
|
@bors-servo r=hiro, manishearth, birtles p=10 |
|
|
Skip adding/accumulating ClipRect values which corresponding rect offset is auto. <!-- Please describe your changes on the following line: --> This is a PR for https://bugzilla.mozilla.org/show_bug.cgi?id=1390352 This patch will skip adding/accumulating the values which corresponding rect offset is auto, and make Servo_AnimationValues_ComputeDistance return negative value instead of 0.0 when the function fails to distinguish its failure. --- <!-- 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 <!-- Either: --> There are already these tests in dom/smil/tests of gecko, this PR will enable these tests. For detail, see https://bugzilla.mozilla.org/show_bug.cgi?id=1390352. <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- 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/17288) <!-- Reviewable:end -->
|
|
@jdm |
| @@ -384,7 +384,7 @@ pub extern "C" fn Servo_AnimationValues_ComputeDistance(from: RawServoAnimationV | |||
| -> f64 { | |||
| let from_value = AnimationValue::as_arc(&from); | |||
| let to_value = AnimationValue::as_arc(&to); | |||
| from_value.compute_squared_distance(to_value).map(|d| d.sqrt()).unwrap_or(0.0) | |||
| from_value.compute_squared_distance(to_value).map(|d| d.sqrt()).unwrap_or(-1.0) | |||
This comment has been minimized.
This comment has been minimized.
nox
Aug 24, 2017
Member
You explained the change in the commit, but a comment right here would be useful to convey that a negative distance means an error occurred.
| fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> { | ||
| if !self.can_animate_with(other, procedure) { | ||
| return Err(()); | ||
| } |
This comment has been minimized.
This comment has been minimized.
nox
Aug 24, 2017
Member
There is a better way to do that:
let animate_component = |this, other| {
match (this.animate(other, procedure)?, procedure) {
(None, Procedure::Interpolate { .. }) => Ok(None),
(None, _) => Err(()),
(result, _) => Ok(result),
}
}This way you don't need code to check whether the two values should be allowed to interpolate at all beforehand.
You could even wrap the Option<T> of the ClipRect fields in its own new type and implement Animate by hand for it, keeping the derived code for ClipRect.
This comment has been minimized.
This comment has been minimized.
|
Thanks @jdm ! |
…s auto. This patch will skip add_weighted() when self_portion + other_portion not equal to zero. (i.e. adding or accumulating). Gecko does same behavior since we can't add two auto value.
…ead of 0.0 when the function fails to distinguish its failure. We need to check whether the function fails or not in order to check whether we support the specified paced animation values. Current servo returns 0.0 when failed computing distance, so servo doesn't distinguish its failure. This patch makes Servo_AnimationValue_ComputeDistance return a negative value when the function fails.
|
r? @nox |
|
I think you can land this PR with previous review from Bugzilla as far as @nox doesn't complain about the updated patch. |
|
Would have been nice to have a better spec link on the |
|
@bors-servo r+ |
|
|
Skip adding/accumulating ClipRect values which corresponding rect offset is auto. <!-- Please describe your changes on the following line: --> This is a PR for https://bugzilla.mozilla.org/show_bug.cgi?id=1390352 This patch will skip adding/accumulating the values which corresponding rect offset is auto, and make Servo_AnimationValues_ComputeDistance return negative value instead of 0.0 when the function fails to distinguish its failure. --- <!-- 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 <!-- Either: --> There are already these tests in dom/smil/tests of gecko, this PR will enable these tests. For detail, see https://bugzilla.mozilla.org/show_bug.cgi?id=1390352. <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- 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/17288) <!-- Reviewable:end -->
|
That failure was #13480, so you can retry. |
|
@bors-servo retry |
Skip adding/accumulating ClipRect values which corresponding rect offset is auto. <!-- Please describe your changes on the following line: --> This is a PR for https://bugzilla.mozilla.org/show_bug.cgi?id=1390352 This patch will skip adding/accumulating the values which corresponding rect offset is auto, and make Servo_AnimationValues_ComputeDistance return negative value instead of 0.0 when the function fails to distinguish its failure. --- <!-- 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 <!-- Either: --> There are already these tests in dom/smil/tests of gecko, this PR will enable these tests. For detail, see https://bugzilla.mozilla.org/show_bug.cgi?id=1390352. <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- 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/17288) <!-- Reviewable:end -->
|
|
Not sure what to make of this error, but it's not related to these changes; probably need to investigate the build machine. |
|
In any case, go ahead and retry. |
|
@bors-servo retry |
|
|
|
|
|
I think test fail of mac-rel-wpt1 is related to #18228 |
|
@bors-servo retry |
|
|
|
|
|
@bors-servo retry |
Skip adding/accumulating ClipRect values which corresponding rect offset is auto. <!-- Please describe your changes on the following line: --> This is a PR for https://bugzilla.mozilla.org/show_bug.cgi?id=1390352 This patch will skip adding/accumulating the values which corresponding rect offset is auto, and make Servo_AnimationValues_ComputeDistance return negative value instead of 0.0 when the function fails to distinguish its failure. --- <!-- 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 <!-- Either: --> There are already these tests in dom/smil/tests of gecko, this PR will enable these tests. For detail, see https://bugzilla.mozilla.org/show_bug.cgi?id=1390352. <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- 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/17288) <!-- Reviewable:end -->
|
|
|
Thanks @upsuper ! |
mantaroh commentedAug 24, 2017
This is a PR for https://bugzilla.mozilla.org/show_bug.cgi?id=1390352
This patch will skip adding/accumulating the values which corresponding rect offset is auto, and make Servo_AnimationValues_ComputeDistance return negative value instead of 0.0 when the function fails to distinguish its failure.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThere are already these tests in dom/smil/tests of gecko, this PR will enable these tests.
For detail, see https://bugzilla.mozilla.org/show_bug.cgi?id=1390352.
This change is