Created attachment 8810766[details]Bug 1286151 - Part 4: Factor out ComputeSingleShadowSquareDistance.
Both eUnit_Shadow and the drop-shadow of eUnit_Filter needs to compute the
distance of two shadows, so we can factor out the implementation.
However, there is a little bit different in these two places.
If one shadow have color, another one doesn't have color value:
1. In eUnit_Shadow, we skip the current shadow pair, and then continue
to compute the distance of the next pair.
2. In eUnit_Filter, we return zero distance of the current filter list pair
directly.
Review commit: https://reviewboard.mozilla.org/r/93074/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93074/

My proposed way to compute the distance of a filter function list pair: (These rules are similar to interpolation. [1])
1. If both filters have a <filter-function-list> of same length without <url> and for each <filter-function> for which there is a corresponding item in each list,
* Compute the square distance of each <filter-function> pair and get the square root of their summation.
2. If both filters have a <filter-function-list> of different length without <url> and for each <filter-function> for which there is a corresponding item in each list,
* Append the missing equivalent <filter-function>s from the longer list to the end of the shorter list.
The new added <filter-function>s must be initialized to their default values.
* Compute the square distance of each <filter-function> pair and get the square root of their summation.
3. If one filter is none and the other is a <filter-function-list> without <url>
* Replace none with the corresponding <filter-function-list> of the other filter.
The new <filter-function>s must be initialized to their default values.
* Compute the square distance of each <filter-function> pair and get the square root of their summation.
4. Otherwise
* Cannot compute the distance, so return 0.0.
How to compute the "square distance" of each <filter-function>?
<blur()>
Get the absolute difference and then square it.
<brightness()>
<contrast()>
<grayscale()>
<invert()>
<opacity()>
<saturate()>
<sepia()>
Convert percentage values to numbers with 0% being relative to 0 and 100% relative to 1. Get the absolute difference and then square it
<hue-rotate()>
Get the absolute difference (unit: "Radian"), and the square it.
<drop-shadow()>
We only support one shadow item, and the format of drop-shadow is:
* drop-shadow( <length>{2,3} <color>? )
* Calculate its square distance just like what we do for {box|text}-shadow.
* Get to square distance of each <length>, and the square distance of <color>, and then sum them.
* We cannot do interpolation between color and no-color [2], so I think we cannot compute its distance in this case,
so return 0.0. However, Google chrome and Safari can do interpolation in this case.
There is a conflict while computing the distance of {box|text}-shadow [3]. We go through all the shadow items even if
one of them cannot be interpolated.
Actually, I think we should treat no-color as "transparent", so we can do interpolation and get its distance.
This may be related to Bug 726550.
[1] https://drafts.fxtf.org/filters/#interpolation-of-filters
[2] http://searchfox.org/mozilla-central/rev/886d5186f0598ab2f8a9953eb5a4dab9750ef834/layout/style/StyleAnimationValue.cpp#1771-1780
[3] http://searchfox.org/mozilla-central/rev/886d5186f0598ab2f8a9953eb5a4dab9750ef834/layout/style/StyleAnimationValue.cpp#1456-1478

(In reply to Boris Chiou [:boris] from comment #7)
> My proposed way to compute the distance of a filter function list pair:
> (These rules are similar to interpolation. [1])
>
> 1. If both filters have a <filter-function-list> of same length without
> <url> and for each <filter-function> for which there is a corresponding item
> in each list,
> * Compute the square distance of each <filter-function> pair and get the
> square root of their summation.
>
> 2. If both filters have a <filter-function-list> of different length without
> <url> and for each <filter-function> for which there is a corresponding item
> in each list,
> * Append the missing equivalent <filter-function>s from the longer list to
> the end of the shorter list.
> The new added <filter-function>s must be initialized to their default
> values.
> * Compute the square distance of each <filter-function> pair and get the
> square root of their summation.
>
> 3. If one filter is none and the other is a <filter-function-list> without
> <url>
> * Replace none with the corresponding <filter-function-list> of the other
> filter.
> The new <filter-function>s must be initialized to their default values.
> * Compute the square distance of each <filter-function> pair and get the
> square root of their summation.
>
> 4. Otherwise
> * Cannot compute the distance, so return 0.0.
Example:
var anim =
createDiv(t).animate([ { filter: 'grayscale(50%) opacity(100%) blur(5px) drop-shadow(10px 10px 1px blue)' },
{ filter: 'grayscale(75%) opacity(50%)' } ],
The distance is: sqrt( (0.75 - 0.5)^2 + (1 - 0.5)^2 + (5 - 0)^2 + (10^2 + 10^2 + 1^2 + (1^2 + 1^2) ) )
^^^^^^^^^^^^ ^^^^^^^^^ ^^^^^^^ ^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^
grayscale opacity blur drop-shadow first 3 blue v.s transparent (percentage RGBA)
Note: blue is (0, 0, 100%, 100%), transparent is (0%, 0%, 0%, 0%)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #17)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #16)
> > Comment on attachment 8810768[details]
> > Bug 1286151 - Part 6: Move tests of spacing on transform into wpt.
> >
> > https://reviewboard.mozilla.org/r/93078/#review93404
> >
> > Why did MozReview track the difference between the old one and the new one?
>
> "didn't"
Maybe I convert git patches into hg patches by git-cinnabar, so the "rename" operator is treated as delete/new file. My |git status| says "it is rename" and my |git show| can see the difference between the old one and the new one. :(

Comment on attachment 8810763[details]Bug 1286151 - Part 1: Implement filter distance for blur.
https://reviewboard.mozilla.org/r/91942/#review93402> I am wondering why this function returns boolean instead of double.
For example:
animate([ { filter: 'grayscale(50%) opacity(100%) blur(5px) drop-shadow(10px 10px 1px blue)' },
{ filter: 'grayscale(75%) opacity(50%) contrast(50%) drop-shadow(20px 20px 5px blue)' } ],
We call ComputeFilterListDistance() to traverse both filter function lists. If one of the filter pair is failed, the total distance should be 0.0 because we cannot do interpolation.
In this case, |blur| and |constrast| are not interpolated, so neither are the two filter function lists. I use the boolean value let ComputeFilterListDistance() know not only the computation is failed, but also the distance is 0.0. I have to keep both information to distinguish this case from something like "|blur(1px)| and |blur(1px)|". |blur(1px)| and |blur(1px)| are interpolated and their distance is also 0.0, so we have to continue to check next filter function pair.
> I am wondering we can create 'none' filter for a given filter function without calling AddWeightedFilterFunction()? This is a bit tricky.
Another way is to write an simple helper function to create an equivalent filter function with default values. (I suppose ComputeFilterDistance() should have non-null arguments)
However, I reuse AddWeightedFilterFunction beause we also do the similar thing to create a transform none function list [1]. I know this might need some redundant computation, but we can reuse the code.
[1] http://searchfox.org/mozilla-central/rev/8562d3859b89ac89f46690b9ed2c473e0728d6c0/layout/style/StyleAnimationValue.cpp#1516-1521

Comment on attachment 8810763[details]Bug 1286151 - Part 1: Implement filter distance for blur.
https://reviewboard.mozilla.org/r/91942/#review93424> Thanks. It makes sense. I guessed we can return negative value in error cases since distance value will never be negative.
Negative value is a good idea. The another reason of returning a boolean value is that ComputeDistance() uses the similar function signature. i.e. return a boolean value, and the output distance is an argument. Thanks for your suggestion.

(In reply to Boris Chiou [:boris] from comment #15)
> I sent you a review request because you reviewed David's patch in Bug
> 523196. I would like to factor out the computation of shadow distance. There
> is a minor difference: In the origin implementation, we continue to
> calculate the distance of the next shadow item if we meet a "color v.s.
> no-color" case. However, in my patch, I would like to return false to
> interrupt the computation because I think we don't do interpolation in this
> case, so we should return 0.0 for distance. The spec doesn't talk much about
> the computaion of shadow distance. What do you think about this? Thanks.
I think it should be fine to bail out on failure.
In the place in the code where you're having to be "failure-tolerant" right now (case eUnit_Shadow), we've actually already called AddWeighted to produce equal-length lists -- and AddWeighted will already fail if it happens to hit a "color vs. no-color" case. SO, we should never actually be depending on the current failure-tolerant behavior -- and as a result, we can use the stricter option, for simplicity / better code sharing.

Comment on attachment 8810766[details]Bug 1286151 - Part 4: Factor out ComputeSingleShadowSquareDistance.
https://reviewboard.mozilla.org/r/93074/#review93622
This is nearly there, but per review nit below, I think we need to fold a bit more into this function in order for its name to make sense.
::: layout/style/StyleAnimationValue.cpp:781
(Diff revision 2)
> + (!color1.IsNumericColorUnit() ||
> + !color2.IsNumericColorUnit())) ||
> + inset1 != inset2) {
> + // According to AddWeightedShadowItems, we don't know how to animate
> + // between color and no-color, or between inset and not-inset,
> + // so we cannot compute the distance neither.
Grammar nit: s/neither/either/
(English frowns on double-negatives -- so "cannot compute the distance either" is the correct formulation here.)
::: layout/style/StyleAnimationValue.cpp:1648
(Diff revision 2)
> - nsCSSValue::Array *array1 = shadow1->mValue.GetArrayValue();
> - nsCSSValue::Array *array2 = shadow2->mValue.GetArrayValue();
> - for (size_t i = 0; i < 4; ++i) {
> - MOZ_ASSERT(array1->Item(i).GetUnit() == eCSSUnit_Pixel,
> + double currentSquareDistance = 0.0;
> + // If we cannot calculate the distance between the current shadow pair,
> + // we skip it and continue to the next one.
> + if (ComputeShadowDistance(shadow1, shadow2, currentSquareDistance)) {
I think we should push the while-loop as well as the sqrt(...) computation *inside* the ComputeShadowDistance function, so that its behavior is a bit more what you would expect. (So: callers shouldn't have to bother caring about "currentSquareDistance" -- they should just get an actual-distance back from ComputeShadowDistance.)
Then it'll be more intuitive, in two ways:
(1) it'll process the whole list that was passed in (as callers would rightly expect)
(2) it'll actually return a distance (as its name suggests it should) instead of a square-distance.
I suspect you've got it split out like you do in order to have the differing failure behavior at each callsite -- but per my previous comment, I think you can make both callsites bail on failure.
And actually, at this callsite, we *should* be able to non-fatally assert (with NS_ERROR(...)) in the failure case -- because in any failure condition, I believe we can expect that AddWeighted should've already failed & made us bail out earlier. Could you add a NS_ERROR along those lines at this callsite?)

Comment on attachment 8810766[details]Bug 1286151 - Part 4: Factor out ComputeSingleShadowSquareDistance.
https://reviewboard.mozilla.org/r/93074/#review93622> I think we should push the while-loop as well as the sqrt(...) computation *inside* the ComputeShadowDistance function, so that its behavior is a bit more what you would expect. (So: callers shouldn't have to bother caring about "currentSquareDistance" -- they should just get an actual-distance back from ComputeShadowDistance.)
>
> Then it'll be more intuitive, in two ways:
> (1) it'll process the whole list that was passed in (as callers would rightly expect)
> (2) it'll actually return a distance (as its name suggests it should) instead of a square-distance.
>
> I suspect you've got it split out like you do in order to have the differing failure behavior at each callsite -- but per my previous comment, I think you can make both callsites bail on failure.
>
> And actually, at this callsite, we *should* be able to non-fatally assert (with NS_ERROR(...)) in the failure case -- because in any failure condition, I believe we can expect that AddWeighted should've already failed & made us bail out earlier. Could you add a NS_ERROR along those lines at this callsite?)
Thanks for explanation. I updated a new patch to include the while-loop and let ComputeShadowDistance returns the final distance we want. Thanks.

Comment on attachment 8810766[details]Bug 1286151 - Part 4: Factor out ComputeSingleShadowSquareDistance.
https://reviewboard.mozilla.org/r/93074/#review93882
::: layout/style/StyleAnimationValue.cpp:881
(Diff revision 4)
> - // between inset and not-inset.
> - // NOTE: In case when both colors' units are eCSSUnit_Null, that means
> - // neither color value was specified, so we can interpolate.
> return false;
> }
> -
> + aSquareDistance = distance * distance;
Ah, sorry -- I hadn't noticed that this callsite *does want* the squared-distance. So it's a bit awkward that we're doing a sqrt() (inside of ComputeShadowDistance) followed by a squaring operation here.
I'd like to avoid that.
Could we factor out the internals of the main loop here into a function named "ComputeSingleShadowSquareDistance()"?
(This will look kind of like ComputeShadowDistance in your previous version of this patch, except now with a name that makes it clearer that it only processes a single list-entry and that it returns a square-distance instead of a distance.)
With that change, maybe we don't actually need a ComputeShadowDistance wrapper function after all, since I think it'd only have one callsite. (I'd be fine either way.)
::: layout/style/StyleAnimationValue.cpp:1661
(Diff revision 4)
> }
>
> const nsCSSValueList *shadow1 = normValue1.GetCSSValueListValue();
> const nsCSSValueList *shadow2 = normValue2.GetCSSValueListValue();
> -
> - double squareDistance = 0.0;
> + if (!ComputeShadowDistance(shadow1, shadow2, aDistance)) {
> + NS_ERROR("Can not compute shadow distance!");
The string here needs to be clearer about why we're caring to (and justified in) spamming an assertion.
How about something like:
NS_ERROR("Unexpected ComputeShadowDistance failure; why didn't we "
"fail earlier, in AddWeighted calls above?");

Comment on attachment 8810766[details]Bug 1286151 - Part 4: Factor out ComputeSingleShadowSquareDistance.
https://reviewboard.mozilla.org/r/93074/#review93882> Ah, sorry -- I hadn't noticed that this callsite *does want* the squared-distance. So it's a bit awkward that we're doing a sqrt() (inside of ComputeShadowDistance) followed by a squaring operation here.
>
> I'd like to avoid that.
>
> Could we factor out the internals of the main loop here into a function named "ComputeSingleShadowSquareDistance()"?
>
> (This will look kind of like ComputeShadowDistance in your previous version of this patch, except now with a name that makes it clearer that it only processes a single list-entry and that it returns a square-distance instead of a distance.)
>
> With that change, maybe we don't actually need a ComputeShadowDistance wrapper function after all, since I think it'd only have one callsite. (I'd be fine either way.)
Thanks, Daniel. I would keep this wrapper because the spec says drop-shadow follows the rules of <shadow-list>s, so reuse the code from <shadow-list>s might be closer to the spec. Therefore, I will follow your suggestion to rename the functions.
> The string here needs to be clearer about why we're caring to (and justified in) spamming an assertion.
>
> How about something like:
> NS_ERROR("Unexpected ComputeShadowDistance failure; why didn't we "
> "fail earlier, in AddWeighted calls above?");
Thanks! This is much clearer!

Comment on attachment 8810766[details]Bug 1286151 - Part 4: Factor out ComputeSingleShadowSquareDistance.
https://reviewboard.mozilla.org/r/93074/#review94246
r=me, with the following nits addressed:
::: layout/style/StyleAnimationValue.cpp:756
(Diff revision 5)
> + const nsCSSValue::Array *array1 = aShadow1->mValue.GetArrayValue();
> + const nsCSSValue::Array *array2 = aShadow2->mValue.GetArrayValue();
Please shift the "*" to the left of the space here (to hug the type rather than the variable name), per Mozilla coding style.
(Comparing the two original code-chunks that you're merging here: one of them had this style correct (type-hugging), and one had them incorrect (variable-name-hugging). Let's make this single shared impl have them correct, rather than incorrect.)
::: layout/style/StyleAnimationValue.cpp:772
(Diff revision 5)
> + const nsCSSValue &color1 = array1->Item(4);
> + const nsCSSValue &color2 = array2->Item(4);
> + const nsCSSValue &inset1 = array1->Item(5);
> + const nsCSSValue &inset2 = array2->Item(5);
As above, please shift the "&" to the left on these 4 lines (to hug the type).
::: layout/style/StyleAnimationValue.cpp:798
(Diff revision 5)
> + // If the computation is failed, aSquareDistance should keep its
> + // original value, so we assign it after computing distance successfully.
> + aSquareDistance = squareDistance;
> + return true;
This comment is a bit confusing, because it sounds like it's suggesting that computation might've failed at this point in the code. (But really, if we get here, we know the computation *did not fail*. Hence, it's confusing to have a comment about "If the computation is failed" here.)
Let's just remove this comment -- or if you'd really like to keep it, please move it up to be alongside the declaration of the "squareDistance" local variable, and replace "is failed" with "fails".
::: layout/style/StyleAnimationValue.cpp:1664
(Diff revision 5)
> - if (color1.GetUnit() != eCSSUnit_Null) {
> - double colorDistance = ComputeColorDistance(color1.GetColorValue(),
> - color2.GetColorValue());
> - squareDistance += colorDistance * colorDistance;
> }
> + aDistance += squareDistance;
This usage of aDistance as a square-distance (temporarily) is a bit confusing. Please add a comment to the right of this line, like...
aDistance += squareDistance; // cumulative distance^2; sqrt()'d below.

Comment on attachment 8810766[details]Bug 1286151 - Part 4: Factor out ComputeSingleShadowSquareDistance.
https://reviewboard.mozilla.org/r/93074/#review94246> This comment is a bit confusing, because it sounds like it's suggesting that computation might've failed at this point in the code. (But really, if we get here, we know the computation *did not fail*. Hence, it's confusing to have a comment about "If the computation is failed" here.)
>
> Let's just remove this comment -- or if you'd really like to keep it, please move it up to be alongside the declaration of the "squareDistance" local variable, and replace "is failed" with "fails".
Thanks. I would like to remove this comment.