Closed Bug 1286151 Opened 8 years ago Closed 8 years ago

Support paced spacing for filter property

Categories

(Core :: DOM: Animation, defect, P3)

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: boris, Assigned: boris)

References

(
URL
)

Details

(Keywords: devrel-needed)

Attachments

(7 files)

Bug 1286151 - Part 1: Implement filter distance for blur.
58 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1286151 - Part 2: Implement filter distance for drop-shadow.
58 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1286151 - Part 3: Implement filter distance for the rest.
58 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1286151 - Part 4: Factor out ComputeSingleShadowSquareDistance.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1286151 - Part 5: Test.
58 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1286151 - Part 6: Move tests of spacing on transform into wpt.
58 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1286151 - Part 7: Make ComputeShapeDistance return the status.
58 bytes, text/x-review-board-request
hiro
: review+
Details 
Bug 1244590 implemented paced spacing, and we also need to implement StyleAnimationValue::ComputeDistance() for filter [1]. According to Bug 1272549 comment 1 and Bug 1272549 comment 2, the current spec [2] doesn't give us any formula for calculating the distance between two filter functions. so we can follow the same rule for interpolation and could use the square root of the sum of the squares of distances of each components between two interpolatable filters.

[1] https://dxr.mozilla.org/mozilla-central/rev/3461f3cae78495f100a0f7d3d2e0b89292d3ec02/layout/style/StyleAnimationValue.cpp#868-869
[2] https://drafts.fxtf.org/filters/#animation-of-filters
Assignee: nobody → boris.chiou
Depends on: 523196
See Also: → 726550
See Also: 726550 →
Status: NEW → ASSIGNED
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%)
Comment on attachment 8810766 [details]
Bug 1286151 - Part 4: Factor out ComputeSingleShadowSquareDistance.

https://reviewboard.mozilla.org/r/93074/#review93398

::: layout/style/StyleAnimationValue.cpp:770
(Diff revision 2)
> +  // Color, Inset
> +  const nsCSSValue &color1 = array1->Item(4);
> +  const nsCSSValue &color2 = array2->Item(4);
> +  const nsCSSValue &inset1 = array1->Item(5);
> +  const nsCSSValue &inset2 = array2->Item(5);
> +  if ((color1.GetUnit() != color2.GetUnit() &&
> +       (!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.
> +    // Note: There are only two possible states of the inset value:
> +    //  (1) GetUnit() == eCSSUnit_Null
> +    //  (2) GetUnit() == eCSSUnit_Enumerated &&
> +    //      GetIntValue() == NS_STYLE_BOX_SHADOW_INSET
> +    return false;
> +  }

Hi Daniel,

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.
Blocks: 1317914
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?
Attachment #8810768 - Flags: review?(hiikezoe) → review+
(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"
(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 8810767 [details]
Bug 1286151 - Part 5: Test.

https://reviewboard.mozilla.org/r/93076/#review93406

It would be better to add a test case of mismatched filter function lists because of reverse order.

e.g.
{ filter: 'grayscale(100%) opacity(50%)' },
{ filter: 'opacity(100%) grayscale(50%)' },

I think the last test case can be changed to the case.
Attachment #8810767 - Flags: review?(hiikezoe) → review+
Comment on attachment 8810767 [details]
Bug 1286151 - Part 5: Test.

https://reviewboard.mozilla.org/r/93076/#review93406

OK. I will change it to this case.
Comment on attachment 8810763 [details]
Bug 1286151 - Part 1: Implement filter distance for blur.

https://reviewboard.mozilla.org/r/91942/#review93402

::: layout/style/StyleAnimationValue.cpp:748
(Diff revision 2)
> +static bool
> +ComputeFilterDistance(const nsCSSValueList *aList1,
> +                      const nsCSSValueList *aList2,
> +                      double& aSquareDistance)
> +{

I am wondering why this function returns boolean instead of double.

::: layout/style/StyleAnimationValue.cpp:826
(Diff revision 2)
> +      UniquePtr<nsCSSValueList> none =
> +        AddWeightedFilterFunction(0, aList2, 0, aList2,

I am wondering we can create 'none' filter for a given filter function without calling AddWeightedFilterFunction()?  This is a bit tricky.
Comment on attachment 8810765 [details]
Bug 1286151 - Part 3: Implement filter distance for the rest.

https://reviewboard.mozilla.org/r/91946/#review93410
Attachment #8810765 - Flags: review?(hiikezoe) → review+
Comment on attachment 8810764 [details]
Bug 1286151 - Part 2: Implement filter distance for drop-shadow.

https://reviewboard.mozilla.org/r/91944/#review93414
Attachment #8810764 - Flags: review?(hiikezoe) → review+
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

::: layout/style/StyleAnimationValue.cpp:748
(Diff revision 2)
> +static bool
> +ComputeFilterDistance(const nsCSSValueList *aList1,
> +                      const nsCSSValueList *aList2,
> +                      double& aSquareDistance)
> +{

Thanks.  It makes sense.  I guessed we can return negative value in error cases since distance value will never be negative.

::: layout/style/StyleAnimationValue.cpp:826
(Diff revision 2)
> +      UniquePtr<nsCSSValueList> none =
> +        AddWeightedFilterFunction(0, aList2, 0, aList2,

I did not know that Brian suggested to re-use AddTransformLists().   Hmm, it seems to me that the name AddTransformLists (or AddWeightedFilterFunction) does not guarantee that it returns the same list with default values when passing 0.0 as coefficient value.  I think we should fix some day somehow.
Attachment #8810763 - Flags: review?(hiikezoe) → review+
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?)
Attachment #8810766 - Flags: review?(dholbert) → review-
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 8811591 [details]
Bug 1286151 - Part 7: Make ComputeShapeDistance return the status.

https://reviewboard.mozilla.org/r/93644/#review93720

ComputeFilterListDistance() should return boolean in the first place (in Part 1)?
Attachment #8811591 - Flags: review?(hiikezoe) → review+
File a spec issue for this:
https://github.com/w3c/fxtf-drafts/issues/91
Comment on attachment 8811591 [details]
Bug 1286151 - Part 7: Make ComputeShapeDistance return the status.

https://reviewboard.mozilla.org/r/93644/#review93720

I think yes. It would be better to let both ComputeFilterListDistance() and ComputeFilterDistance() return boolean. Thanks for your suggestion. I will move the change for ComputeFilterListDistance() into part 1, so this part only revise the function signture of ComputeShapeDistance.
Comment on attachment 8810763 [details]
Bug 1286151 - Part 1: Implement filter distance for blur.

https://reviewboard.mozilla.org/r/91942/#review93884

::: layout/style/StyleAnimationValue.cpp:746
(Diff revision 4)
> +// Return false if we cannot compute the distance between these filter
> +// functions.
> +static bool
> +ComputeFilterDistance(const nsCSSValueList* aList1,
> +                      const nsCSSValueList* aList2,
> +                      double& aSquareDistance)
> +{

Could you please add "Square" to this function name? (so, calling it "ComputeFilterSquareDistance")

It does not return a distance -- it returns a square-distance -- so its name should reflect that. (And the other preexisting functions in this file that return square-distance have "Square" in their name.)
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?");
Attachment #8810766 - Flags: review?(dholbert) → review-
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.
Attachment #8810766 - Flags: review?(dholbert) → review+
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.
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/46dbee3c3988
Part 1: Implement filter distance for blur. r=hiro
https://hg.mozilla.org/integration/autoland/rev/38140192cbc9
Part 2: Implement filter distance for drop-shadow. r=hiro
https://hg.mozilla.org/integration/autoland/rev/74f17142693e
Part 3: Implement filter distance for the rest. r=hiro
https://hg.mozilla.org/integration/autoland/rev/f03eb570c499
Part 4: Factor out ComputeSingleShadowSquareDistance. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/b42b42b76d0f
Part 5: Test. r=hiro
https://hg.mozilla.org/integration/autoland/rev/c177db19fc7a
Part 6: Move tests of spacing on transform into wpt. r=hiro
https://hg.mozilla.org/integration/autoland/rev/c2249c615a33
Part 7: Make ComputeShapeDistance return the status. r=hiro
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: