Closed Bug 1209780 Opened 4 years ago Closed 4 years ago

Make DrawResult MOZ_MUST_USE

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(6 files)

It will be pretty easy to break sync decoding in the future by ignoring the return value of functions that return a DrawResult. Let's make that harder by marking DrawResult MOZ_MUST_USE.

Making this change is likely to reveal some existing issues; we'll fix those here too.
This patch implements the actual change. We'll need a try job to see where we're
currently ignoring DrawResults. I'm guessing we *are* doing that in some places,
so I've gone ahead and marked this "part 1".
Attachment #8667597 - Flags: review?(tnikkel)
Attachment #8667597 - Flags: review?(tnikkel) → review+
Turns out that we're actually hitting a bug in MOZ_MUST_USE itself - see bug 1214037. Once that's fixed, we can continue to move forward on this.
Depends on: 1214037
Revived this
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cdd2aac1a91
I get unused value warnings in the table painter still, but they don't seem to make any sense, the value is used.
Depends on: 1254247
Okay, after working around bug 1254247 in the static analysis, I think I have the complete list of places where we ignored a DrawResult. Bug 1203417 is one. I'll post patches here for the rest.
Attachment #8727982 - Flags: review?(seth)
Some of these could propagate their results, but display items are harder to come by. For now I'm just including this so we know these need to be fixed. Not asking for review yet. If we decide to punt on these we can file a followup bug.
Not sure exactly what we want to/should do here. This seems reasonable.
Attachment #8727988 - Flags: review?(seth)
Not sure if the first hunk is needed or not, but better to keep it consistent I think.
Attachment #8727989 - Flags: review?(seth)
Comment on attachment 8727982 [details] [diff] [review]
Fix nsTreeBodyFrame.

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

Looks good! Glad to get moving on this; I think it'll greatly reduce the number of mistakes we make with DrawResult.

::: layout/xul/tree/nsTreeBodyFrame.h
@@ +258,5 @@
>                          nscoord&             aRemainingWidth,
>                          nscoord&             aCurrX);
>  
>    // This method paints the text string inside a particular cell of the tree.
> +  DrawResult PaintText(int32_t             aRowIndex, 

You didn't introduce it, but it'd be good to remove the whitespace at the end of this line.
Attachment #8727982 - Flags: review?(seth) → review+
Attachment #8727988 - Flags: review?(seth) → review+
Attachment #8727989 - Flags: review?(seth) → review+
(In reply to Timothy Nikkel (:tnikkel) from comment #10)
> Created attachment 8727989 [details] [diff] [review]
> Fix a few DrawResult's in nsTablePainter
> 
> Not sure if the first hunk is needed or not, but better to keep it
> consistent I think.

Agreed; better to keep it consistent.
https://hg.mozilla.org/mozilla-central/rev/82073af0177d
https://hg.mozilla.org/mozilla-central/rev/7596f7088006
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Oops, forgot to use leave-open.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Timothy Nikkel (:tnikkel) from comment #8)
> Created attachment 8727983 [details] [diff] [review]
> Mark some DrawResult as unused in layout/svg
> 
> Some of these could propagate their results, but display items are harder to
> come by. For now I'm just including this so we know these need to be fixed.
> Not asking for review yet. If we decide to punt on these we can file a
> followup bug.

Yeah. I think we want to fix these, but it may be better just to file a followup, because it's important that we stop more issues from being introduced.
Comment on attachment 8727983 [details] [diff] [review]
Mark some DrawResult as unused in layout/svg

I'll file a followup and insert FIXME comments with the bug number into the source.
Attachment #8727983 - Flags: review?(seth)
(In reply to Seth Fowler [:seth] [:s2h] from comment #19)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=66c726fa5347

That has the svg failures, as expected. Try push with svg patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a854bd056c2
Comment on attachment 8727983 [details] [diff] [review]
Mark some DrawResult as unused in layout/svg

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

Looks good. Please don't forget to file the followup, though.
Attachment #8727983 - Flags: review?(seth) → review+
Followup bug for the svg cases bug 1258510.
We have static analysis on OS X, I did not know this. It catches another DrawResult not being used. So I backed out the patch making DrawResult a must use.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a250decd09b6
Comment on attachment 8733088 [details] [diff] [review]
Fix a call in cocoa utils

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

Looks good.

::: widget/cocoa/nsCocoaUtils.mm
@@ +491,5 @@
>        NS_ERROR("Failed to create gfxContext");
>        return NS_ERROR_FAILURE;
>      }
>  
> +    mozilla::image::DrawResult res = 

There's whitespace at the end of this line.
Attachment #8733088 - Flags: review?(seth) → review+
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
You need to log in before you can comment on or make changes to this bug.