Last Comment Bug 577911 - don't ignore return value from SetGlyphsFromRun (was: mark DEBUG only variables as ifdef DEBUG in gfx)
: don't ignore return value from SetGlyphsFromRun (was: mark DEBUG only variabl...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
Depends on: 577899
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-11 05:20 PDT by timeless
Modified: 2011-10-09 07:34 PDT (History)
4 users (show)
Ms2ger: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (832 bytes, patch)
2010-07-11 05:41 PDT, timeless
no flags Details | Diff | Splinter Review
using #ifdef DEBUG (894 bytes, patch)
2010-08-06 05:07 PDT, timeless
no flags Details | Diff | Splinter Review
patch v2 - don't ignore the return value from SetGlyphsFromRun (834 bytes, patch)
2010-08-16 09:49 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
bugzilla: approval2.0-
Details | Diff | Splinter Review

Description timeless 2010-07-11 05:20:42 PDT
this is part of a crusade to get rid of compilation warnings
Comment 1 timeless 2010-07-11 05:41:22 PDT
Created attachment 456738 [details] [diff] [review]
patch
Comment 2 timeless 2010-08-06 05:07:42 PDT
Created attachment 463498 [details] [diff] [review]
using #ifdef DEBUG
Comment 3 Jonathan Kew (:jfkthame) 2010-08-16 09:49:47 PDT
Created attachment 466340 [details] [diff] [review]
patch v2 - don't ignore the return value from SetGlyphsFromRun

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.)
Comment 4 Jonathan Kew (:jfkthame) 2010-08-17 07:16:34 PDT
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.
Comment 5 Jonathan Kew (:jfkthame) 2011-02-21 09:32:45 PST
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.
Comment 6 Johnathan Nightingale [:johnath] 2011-02-22 12:10:57 PST
Comment on attachment 466340 [details] [diff] [review]
patch v2 - don't ignore the return value from SetGlyphsFromRun

Clean up can wait until after 4
Comment 7 :Ehsan Akhgari 2011-03-25 16:37:12 PDT
This patch doesn't apply cleanly any more.
Comment 8 Jonathan Kew (:jfkthame) 2011-10-07 09:28:10 PDT
Rebased and pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32989cd5377b
Comment 9 Matt Brubeck (:mbrubeck) 2011-10-07 12:36:15 PDT
Backed out because of build bustage:
https://hg.mozilla.org/mozilla-central/rev/6028686b01f0
Comment 10 Jonathan Kew (:jfkthame) 2011-10-08 11:35:04 PDT
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.
Comment 11 Jonathan Kew (:jfkthame) 2011-10-08 11:38:38 PDT
Re-landed with the unwanted #ifdef DEBUG removed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe700c7747c
Comment 12 :Ms2ger (⌚ UTC+1/+2) 2011-10-09 07:34:03 PDT
https://hg.mozilla.org/mozilla-central/rev/fbe700c7747c

Note You need to log in before you can comment on or make changes to this bug.