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

RESOLVED FIXED in mozilla10

Status

()

Core
Graphics
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: timeless, Assigned: jfkthame)

Tracking

Trunk
mozilla10
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
this is part of a crusade to get rid of compilation warnings
(Reporter)

Comment 1

7 years ago
Created attachment 456738 [details] [diff] [review]
patch
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #456738 - Flags: review?(jfkthame)
(Reporter)

Comment 2

7 years ago
Created attachment 463498 [details] [diff] [review]
using #ifdef DEBUG
Attachment #456738 - Attachment is obsolete: true
Attachment #463498 - Flags: review?(jfkthame)
Attachment #456738 - Flags: review?(jfkthame)
(Reporter)

Updated

7 years ago
Summary: use NS_DEBUG_ASSIGN for gfx → mark DEBUG only variables as ifdef DEBUG in gfx
(Assignee)

Comment 3

7 years ago
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.)
Attachment #466340 - Flags: review?(jdaggett)

Updated

7 years ago
Attachment #466340 - Flags: review?(jdaggett) → review+
(Assignee)

Comment 4

7 years ago
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)
(Assignee)

Comment 5

7 years ago
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
(Assignee)

Comment 8

6 years ago
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 → ---
(Assignee)

Comment 10

6 years ago
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)
(Assignee)

Comment 11

6 years ago
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
Last Resolved: 6 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.