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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[timeless]

this is part of a crusade to get rid of compilation warnings

Comment 1

[timeless]

Attachment: patch

Comment 2

[timeless]

Attachment: using #ifdef DEBUG

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: 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]]

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]]

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]]

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

[(no longer active)]

This patch doesn't apply cleanly any more.

Comment 8

[Jonathan Kew [:jfkthame]]

Rebased and pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32989cd5377b

Comment 9

[Matt Brubeck (:mbrubeck)]

Backed out because of build bustage:
https://hg.mozilla.org/mozilla-central/rev/6028686b01f0

Comment 10

[Jonathan Kew [:jfkthame]]

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]]

Re-landed with the unwanted #ifdef DEBUG removed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe700c7747c

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/fbe700c7747c