Closed
Bug 1298722
Opened 8 years ago
Closed 8 years ago
Use MOZ_MUST_USE in StyleAnimationValue
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
(Blocks 1 open bug)
Details
(Keywords: coverity, Whiteboard: [CID 1369635])
Attachments
(1 file, 1 obsolete file)
19.39 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Comment 2•8 years ago
|
||
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+
Assignee | ||
Comment 3•8 years ago
|
||
> > + // 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.
Assignee | ||
Comment 4•8 years ago
|
||
Updated version, with comments addressed. I will carry over the r+.
Assignee | ||
Updated•8 years ago
|
Attachment #8785749 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8786225 -
Flags: review+
Assignee | ||
Comment 5•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfcfff96e4ffd245631e59cacb187410ef20362c
Bug 1298722 - Use MOZ_MUST_USE in StyleAnimationValue. r=birtles.
Comment 6•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment 7•8 years ago
|
||
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
Comment 8•8 years ago
|
||
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)
Comment 10•8 years ago
|
||
bugherder |
Updated•6 years ago
|
Blocks: coverity-analysis
You need to log in
before you can comment on or make changes to this bug.
Description
•