Closed Bug 1298722 Opened 3 years ago Closed 3 years ago

Use MOZ_MUST_USE in StyleAnimationValue

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: njn, Assigned: njn)

References

(Blocks 2 open bugs)

Details

(Keywords: coverity, Whiteboard: [CID 1369635])

Attachments

(1 file, 1 obsolete file)

Coverity identified that 1 of the 5 calls to StyleAnimationValue::ComputeDistance() doesn't check the return value. In this case this appears to be legitimate (there's a comment explaining it) but it's a good opportunity to add MOZ_MUST_USE to all the bool return values in that class that indicates success/failure.
birtles, please check the followin in particular:

- the "njn" comment on one case where I wasn't sure what to do;

- the fact that I didn't add MOZ_MUST_USE to UncomputeValue().
Attachment #8785749 - Flags: review?(bbirtles)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Comment on attachment 8785749 [details] [diff] [review]
Use MOZ_MUST_USE in StyleAnimationValue

Review of attachment 8785749 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this. r=me with the following changes.

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +563,5 @@
>                 "Must have same unit");
> +  // njn: what to do here?
> +  Unused <<
> +    StyleAnimationValue::Interpolate(aAnimation.property(), aStart, aEnd,
> +                                     aPortion, interpolatedValue);

I think we're not currently expecting this to fail since we only pass transform and opacity values to the compositor and they should never fail to interpolate.

Perhaps an assertion would be appropriate here so that if this ever does happen we know we need to introduce the 50% flip logic we use on the main thread when interpolation fails.[1]

[1] https://dxr.mozilla.org/mozilla-central/rev/1a5b53a831e5a6c20de1b081c774feb3ff76756c/dom/animation/KeyframeEffect.cpp#406

::: layout/style/StyleAnimationValue.h
@@ +54,5 @@
>     * @return true on success, false on failure.
>     */
> +  static MOZ_MUST_USE bool
> +  Add(nsCSSPropertyID aProperty, StyleAnimationValue& aDest,
> +      const StyleAnimationValue& aValueToAdd, uint32_t aCount) {

Unfortunately, I think our coding style would have us put the return type on the same line as the method name in the method declaration.[1] (Which seems unfortunate because its inconsistent with what we do elsewhere. If you think we can ignore this, I'd be glad to hear it!)

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes

@@ +219,5 @@
>     * @param [out] aSpecifiedValue The resulting specified value.
>     * @return true on success, false on failure.
> +   *
> +   * These functions are not MOZ_MUST_USE because failing to check the return
> +   * value is common and reasonable.

I think we should actually make UncomputeValue MOZ_MUST_USE.

As far as I can tell, the only two cases where this will fail are:

a. If we pass a StyleAnimationValue with Unit null
b. If we pass a StyleAnimationValue with Unit UnparsedString to one
   of the overloads that does NOT serialize to a string (i.e. serializes
   to an nsCSSValue).

Also, it seems like the only places we *don't* check the result are:

* The debug-only AnimValuesStyleRule::List method

  This could probably reasonably ignore the result (particularly since we are
  using the overload that serializes to a string so it's only the null unit case
  where this could fail)

* The debug-only DumpAnimationProperties function

  Likewise this could also reasonably ignore the result (it too serializes
  to a string)

* CreatePropertyValue in KeyframeEffect.cpp

  In this case I think we don't expect this to fail since we shouldn't have
  a null value at this point (and we are serializing to a string so we shouldn't
  fail in the case when the unit is unparsed string).

  If we do have a failure it's likely a bug. e.g.

  - we added a new type to StyleAnimationValue but forgot to make UncomputeValue
    handle it
  - we added some logic to UncomputeValue that now means it can fail for valid
    units -- if so we should work out how to handle that at call sites
  - we changed the logic of CreatePropertyValue or one of its call sites so we
    can now get null values -- that's probably a bug in itself, or at very least
    we should detect and handle null values

  So I think we should just assert that that the result is true here.

* CSSAnimationBuilder::GetComputedValue

  In this case we're not expecting it to fail since we pass the result of
  ExtractComputedValue to UncomputeValue. I think we should just assert that
  the result is true here, since, like the previous case, if this fails its
  probably a bug.
Attachment #8785749 - Flags: review?(bbirtles) → review+
> > +  // njn: what to do here?
> > +  Unused <<
> > +    StyleAnimationValue::Interpolate(aAnimation.property(), aStart, aEnd,
> > +                                     aPortion, interpolatedValue);
> 
> I think we're not currently expecting this to fail since we only pass
> transform and opacity values to the compositor and they should never fail to
> interpolate.
> 
> Perhaps an assertion would be appropriate here so that if this ever does
> happen we know we need to introduce the 50% flip logic we use on the main
> thread when interpolation fails.[1]

Done.


> > +  static MOZ_MUST_USE bool
> > +  Add(nsCSSPropertyID aProperty, StyleAnimationValue& aDest,
> > +      const StyleAnimationValue& aValueToAdd, uint32_t aCount) {
> 
> Unfortunately, I think our coding style would have us put the return type on
> the same line as the method name in the method declaration.[1] (Which seems
> unfortunate because its inconsistent with what we do elsewhere. If you think
> we can ignore this, I'd be glad to hear it!)

I think we can ignore this. Consider this one:

> static MOZ_MUST_USE bool ExtractComputedValue(nsCSSPropertyID aProperty,
>                                               nsStyleContext* aStyleContext,
>                                               StyleAnimationValue& aComputedValue);

That exceeds 80 lines. So we have to break it somehow, and I think this:

> static MOZ_MUST_USE bool 
> ExtractComputedValue(nsCSSPropertyID aProperty,
>                      nsStyleContext* aStyleContext,
>                      StyleAnimationValue& aComputedValue);

is better than this:

> static MOZ_MUST_USE bool ExtractComputedValue(
>   nsCSSPropertyID aProperty, 
>   nsStyleContext* aStyleContext,
>   StyleAnimationValue& aComputedValue);

I've used this breaking style in other places before and people haven't complained.


> I think we should actually make UncomputeValue MOZ_MUST_USE.

Ok, will do.
Updated version, with comments addressed. I will carry over the r+.
Attachment #8785749 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/cfcfff96e4ff
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Hello, I was working with related code recently and I noticed that although the comment saying "These functions are not MOZ_MUST_USE" is there, the functions are in fact marked MOZ_MUST_USE. Is this intentional?

For comment see: https://dxr.mozilla.org/mozilla-central/source/layout/style/StyleAnimationValue.h?q=StyleAnimationValue&redirect_type=direct#267
I just ran into the discrepancy that jyc brought up in comment 7, too (while reviewing a patch that calls this code).

Reading back through this bug, it looks like the comment-vs-code discrepancy exists because:
 - njn *initially* didn't intend to to label these functions (and he admirably included some documentation to explain why they were unlabeled)
 - But, then that changed in comment 2 & comment 3 ("I think we should actually make UncomputeValue MOZ_MUST_USE", "Ok, will do."
 - ...and the original (obsoleted) explanatory code-comment was left in the patch by mistake.

I'll push a trivial followup to clean this up (deleting the comment.)
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/562b1c41685e
followup: remove obsolete documentation about MOZ_MUST_USE annotations on StyleAnimationValue::UncomputeValue(). (no review, comment-only, DONTBUILD)
You need to log in before you can comment on or make changes to this bug.