Closed Bug 577911 Opened 10 years ago Closed 9 years ago

don't ignore return value from SetGlyphsFromRun (was: mark DEBUG only variables as ifdef DEBUG in gfx)

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: timeless, Assigned: jfkthame)

References

Details

Attachments

(1 file, 2 obsolete files)

this is part of a crusade to get rid of compilation warnings
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #456738 - Flags: review?(jfkthame)
Attached patch using #ifdef DEBUG (obsolete) — Splinter Review
Attachment #456738 - Attachment is obsolete: true
Attachment #463498 - Flags: review?(jfkthame)
Attachment #456738 - Flags: review?(jfkthame)
Summary: use NS_DEBUG_ASSIGN for gfx → mark DEBUG only variables as ifdef DEBUG in gfx
Rather than #ifdef'ing the declaration of rv here, I think we should check the value and return the proper success or failure status to the caller. Ignoring failure at this point is a legacy of older code where failure of InitTextRun() was not really handled, but since bug 553963 has landed we have a more useful way of dealing with this.

(I think failure in SetGlyphsFromRun is highly unlikely except in an out-of-memory scenario, in which case the browser is probably doomed anyway. But given that InitTextRun is now supposed to return whether it succeeded, we might as well do it properly here.)
Attachment #466340 - Flags: review?(jdaggett)
Attachment #466340 - Flags: review?(jdaggett) → review+
Comment on attachment 463498 [details] [diff] [review]
using #ifdef DEBUG

This is obsoleted by attachment 466340 [details] [diff] [review], as the variable will no longer be unused in release builds.
Attachment #463498 - Attachment is obsolete: true
Attachment #463498 - Flags: review?(jfkthame)
Comment on attachment 466340 [details] [diff] [review]
patch v2 - don't ignore the return value from SetGlyphsFromRun

This is minor tidying-up, but it would be nice to go ahead and land it for correctness' sake.
Attachment #466340 - Flags: approval2.0?
Comment on attachment 466340 [details] [diff] [review]
patch v2 - don't ignore the return value from SetGlyphsFromRun

Clean up can wait until after 4
Attachment #466340 - Flags: approval2.0? → approval2.0-
This patch doesn't apply cleanly any more.
Whiteboard: not-ready
Rebased and pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32989cd5377b
Assignee: timeless → jfkthame
Whiteboard: not-ready
Target Milestone: --- → mozilla10
Backed out because of build bustage:
https://hg.mozilla.org/mozilla-central/rev/6028686b01f0
Target Milestone: mozilla10 → ---
Updating the summary to reflect what the patch here actually does. The change in the obsoleted patch here (attachment 463498 [details] [diff] [review]) ended up getting done by bug 605179, but we don't really want that change - the rv variable isn't supposed to be DEBUG-only, it's a return code that we ought to check. I will undo that when re-pushing the patch here, so that it doesn't break things any more.
Summary: mark DEBUG only variables as ifdef DEBUG in gfx → don't ignore return value from SetGlyphsFromRun (was: mark DEBUG only variables as ifdef DEBUG in gfx)
Re-landed with the unwanted #ifdef DEBUG removed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe700c7747c
https://hg.mozilla.org/mozilla-central/rev/fbe700c7747c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.