Closed
Bug 1209780
Opened 9 years ago
Closed 8 years ago
Make DrawResult MOZ_MUST_USE
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: seth, Assigned: seth)
References
Details
Attachments
(6 files)
780 bytes,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
6.89 KB,
patch
|
seth
:
review+
|
Details | Diff | Splinter Review |
4.52 KB,
patch
|
seth
:
review+
|
Details | Diff | Splinter Review |
1.30 KB,
patch
|
seth
:
review+
|
Details | Diff | Splinter Review |
1.71 KB,
patch
|
seth
:
review+
|
Details | Diff | Splinter Review |
1.25 KB,
patch
|
seth
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8667597 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 2•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d66f856bbc7a
Assignee | ||
Comment 3•9 years ago
|
||
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
Assignee | ||
Comment 4•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7bba7433f26
Comment 5•8 years ago
|
||
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.
Comment 6•8 years ago
|
||
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.
Comment 7•8 years ago
|
||
Attachment #8727982 -
Flags: review?(seth)
Comment 8•8 years ago
|
||
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.
Comment 9•8 years ago
|
||
Not sure exactly what we want to/should do here. This seems reasonable.
Attachment #8727988 -
Flags: review?(seth)
Comment 10•8 years ago
|
||
Not sure if the first hunk is needed or not, but better to keep it consistent I think.
Attachment #8727989 -
Flags: review?(seth)
Assignee | ||
Comment 11•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Attachment #8727988 -
Flags: review?(seth) → review+
Assignee | ||
Updated•8 years ago
|
Attachment #8727989 -
Flags: review?(seth) → review+
Assignee | ||
Comment 12•8 years ago
|
||
(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.
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/82073af0177d https://hg.mozilla.org/integration/mozilla-inbound/rev/7596f7088006
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/82073af0177d https://hg.mozilla.org/mozilla-central/rev/7596f7088006
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0028f61d4526
Comment 17•8 years ago
|
||
Oops, forgot to use leave-open.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 18•8 years ago
|
||
(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.
Assignee | ||
Comment 19•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=66c726fa5347
Comment 20•8 years ago
|
||
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)
Comment 21•8 years ago
|
||
(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
Assignee | ||
Comment 22•8 years ago
|
||
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+
Comment 23•8 years ago
|
||
Followup bug for the svg cases bug 1258510.
Comment 24•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/25ce690ffebc https://hg.mozilla.org/integration/mozilla-inbound/rev/bc854d1c6ab7
Comment 25•8 years ago
|
||
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
Updated•8 years ago
|
Keywords: leave-open
Comment 26•8 years ago
|
||
Attachment #8733088 -
Flags: review?(seth)
Comment 29•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/25ce690ffebc
Assignee | ||
Comment 30•8 years ago
|
||
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+
Comment 31•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ce8b97e0e69 https://hg.mozilla.org/integration/mozilla-inbound/rev/f292de432804
Updated•8 years ago
|
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Keywords: leave-open
Resolution: --- → FIXED
Updated•8 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 32•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0ce8b97e0e69 https://hg.mozilla.org/mozilla-central/rev/f292de432804
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•