Closed Bug 1388601 Opened 7 years ago Closed 7 years ago

stylo: Implement test cases for ComputeSquaredDistance on BasicShape and FilterList

Categories

(Core :: CSS Parsing and Computation, enhancement, P4)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: boris, Assigned: boris)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1388601 - Part 1: Add tests of distance for Basic Shape.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1388601 - Part 2: Add tests of distance for Filter lists.
59 bytes, text/x-review-board-request
hiro
: review+
Details 
This bug will implement Animatable::compute_distance [1] and compute_squared_distance for BasicShape.

[1] https://github.com/w3c/csswg-drafts/issues/662
Assignee: nobody → boris.chiou
AFAIK the properties that currently use BasicShape aren't even currently animatable.
Yes, I believe Bug 1376495 will make the properties using BasicShape animatable first.
It seems nox implemented this in Servo already. In this bug, I will add some tests of BasicShape in Gecko to make sure we have the same result for Gecko and Servo.
Priority: P2 → P3
downgrade to P3 because we only need to add some test cases.
Priority: P3 → P4
Summary: stylo: Implement compute_distance for BasicShape → stylo: Implement test cases for ComputeSquaredDistance on BasicShape
Status: NEW → ASSIGNED
Summary: stylo: Implement test cases for ComputeSquaredDistance on BasicShape → stylo: Implement test cases for ComputeSquaredDistance on BasicShape and FilterList
There is a bug of filter on Servo, so... before sending review requests, I have to fix it first (in another bug).
Comment on attachment 8907999 [details]
Bug 1388601 - Part 1: Add tests of distance for Basic Shape.

https://reviewboard.mozilla.org/r/179678/#review185298

Looks awesome!
I don't still understand why '* 2' for inset distance, but other parts look pretty great!  Holding r+ until Boris adds a comment for the inset distance.

::: dom/animation/test/mozilla/test_distance_of_basic_shape.html:19
(Diff revision 2)
> +  assert_equals(dist, 0, 'distance of basic shape');
> +}, 'Test distance of none and none');

I guess we don't need 'Test distance of' because we will know this test file is test_distance_of_basic_shape.html if we see failures in this test file.
Wherea, we might want to know which assertion fails if we see failures, so it would be nice to have 'distance between none and none' in assert_equals().

::: dom/animation/test/mozilla/test_distance_of_basic_shape.html:25
(Diff revision 2)
> +}, 'Test distance of none and none');
> +
> +test(function(t) {
> +  var target = addDiv(t);
> +  var dist = getDistance(target, 'clip-path', 'circle(10px)', 'circle(20px)');
> +  assert_equals(dist, 10, 'distance of basic shape');

Likewise, should we use 'circle(10px) and circle(20px)', or 'circle(10px) -> circle(20px)', I am not sure how we should represent distance between two points though. Now I think 'distance of' is redandunt. Rest of assert_equals() should be modified as well.

::: dom/animation/test/testcommon.js:352
(Diff revision 2)
>    target.appendChild(element);
>    return element;
>  }
> +
> +/*
> + * Get Animation distance from two specified values for a specific property.

Nit: 'distance between two specified values for..'?
Comment on attachment 8908000 [details]
Bug 1388601 - Part 2: Add tests of distance for Filter lists.

https://reviewboard.mozilla.org/r/179680/#review185310

This one should also modify test name and assert descrition as well as the part 1, but anyway looks great!
Attachment #8908000 - Flags: review?(hikezoe) → review+
Comment on attachment 8907999 [details]
Bug 1388601 - Part 1: Add tests of distance for Basic Shape.

https://reviewboard.mozilla.org/r/179678/#review185298

> I don't still understand why '* 2' for inset distance, but other parts look pretty great!  Holding r+ until Boris adds a comment for the inset distance.

Just add some comment in the file and update those descriptions in these two test files.
Thanks for the suggestion.
Comment on attachment 8907999 [details]
Bug 1388601 - Part 1: Add tests of distance for Basic Shape.

https://reviewboard.mozilla.org/r/179678/#review185322

Thanks for clearing explanation!
Attachment #8907999 - Flags: review?(hikezoe) → review+
Besides, I will land these patches after Bug 1399799 is landed. Thanks, Hiro-san!
Do'h, 'clear explanation'.
Depends on: 1399799
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/499c7061d5b6
Part 1: Add tests of distance for Basic Shape. r=hiro
https://hg.mozilla.org/integration/autoland/rev/7c12af6fd620
Part 2: Add tests of distance for Filter lists. r=hiro
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: