Closed Bug 842514 Opened 7 years ago Closed 7 years ago

consistently use signed types for appUnitsPerDevPixel variables

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: jfkthame, Assigned: jfkthame)

Details

Attachments

(4 files)

Bug 832440 addressed this in gfxFont.* and a couple of associated files, but we also have similar inconsistencies elsewhere in gfx and layout code.

Although appUnits per device pixel can obviously never be negative, I think it makes most sense to standardized on signed integers for these fields/variables. I don't expect any behavior change from this - in most cases, we're assigning the value to a (signed) nscoord or floating-point variable anyway - but it will help us avoid stumbling over the signed-vs-unsigned comparison warning, as happened in bug 841470 recently, or potential overflow/data-loss warnings when assigning between signed and unsigned variables of the same size.

(If we decide to switch these values entirely to floating-point, as suggested in bug 833037, the issue becomes moot, but that's a separate discussion and potentially a more disruptive change.)
Assignee: nobody → jfkthame
Attachment #715448 - Flags: review?(roc)
Tryserver run to confirm these patches don't introduce new build failures:
https://tbpl.mozilla.org/?tree=Try&rev=16b697e84d58
Comment on attachment 715451 [details] [diff] [review]
part 4 - make appUnitsPerDev signed in canvas

Review of attachment 715451 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/CanvasRenderingContext2D.h
@@ +485,5 @@
> +
> +    CMG_STYLE_PATTERN = 1,
> +
> +    CMG_STYLE_GRADIENT = 2
> +

Remove this hunk, you're just adding unnecessary lines
Attachment #715451 - Flags: review?(roc) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> Comment on attachment 715451 [details] [diff] [review]
> part 4 - make appUnitsPerDev signed in canvas
> 
> Review of attachment 715451 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/CanvasRenderingContext2D.h
> @@ +485,5 @@
> > +
> > +    CMG_STYLE_PATTERN = 1,
> > +
> > +    CMG_STYLE_GRADIENT = 2
> > +
> 
> Remove this hunk, you're just adding unnecessary lines

Hmm... actually that was caused by a chunk with windows line-endings, which my editor interpreted as doubled newlines. So I'll fix that.
Folded the 4 patches together, and cleaned-up those line endings:

https://hg.mozilla.org/integration/mozilla-inbound/rev/27652a4eddb4

(I won't be surprised if there are still some cases that I've missed, but this should at least reduce the opportunities for signedness-mismatches with these variables.)
Target Milestone: --- → mozilla22
https://hg.mozilla.org/mozilla-central/rev/27652a4eddb4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.