Closed Bug 832440 Opened 7 years ago Closed 7 years ago

gfxFont.cpp|h is inconsistent on whether appUnitsPerDevPixel is signed or unsigned

Categories

(Core :: Graphics: Text, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Build warning:
{
gfx/thebes/gfxFont.cpp:2438:46: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
}
for this line of code:
> 2438         sw->GetAppUnitsPerDevUnit() != aKey->mAppUnitsPerDevUnit ||
https://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.cpp#2438

gfxFont.[h|cpp] used to have all of its appUnitsPerDevUnit variables being signed, but this cset for bug 825871 converted one of them to unsigned:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39b72947ad79#l8.502

I think the others want to be converted, too.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Pretty sure this converts all the remaining signed appUnitsPerDevUnit values there.
Attachment #704029 - Flags: review?(jfkthame)
Here's the other option: convert everything to be signed.  (That's more consistent with e.g. nsDeviceContext::AppUnitsPerCSSPixel() and related functions, and it's safer in case we end up doing some math involving subtraction with one of these variables / methods somewhere.)
Attachment #704126 - Flags: review?(jfkthame)
Attachment #704029 - Attachment description: fix v1 → fix v1: convert everything to be unsigned
Attachment #704029 - Attachment description: fix v1: convert everything to be unsigned → option 1: convert everything to be unsigned
Attachment #704126 - Attachment description: fix v2: convert everything to be signed → option 2: convert everything to be signed
(In reply to Daniel Holbert [:dholbert] from comment #2)
> Here's the other option: convert everything to be signed.  (That's more
> consistent with e.g. nsDeviceContext::AppUnitsPerCSSPixel()

Actually, this is _not_ consistent with nsDeviceContext::AppUnitsPerDevPixel()  [1] or nsPresContext::AppUnitsPerDevPixel() [2]

As a result, this triggered build warnings (treated as errors)[3] for downgrading a uint32 into an int32, for a couple of callers into gfxFont APIs that pass a uint32.

This updated patch addresses that in one spot by adding a int32_t cast, and another spot by changing a numeric-constant-returning function to return int32_t instead of uint32_t.

I'm leaning towards 'option 1', though, for consistency across our various appUnitsPerDevPixel APIs/variables...

[1] https://mxr.mozilla.org/mozilla-central/source/gfx/src/nsDeviceContext.h#56
[2] https://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.h#548
[3] (because layout/generic and layout/svg are both FAIL_ON_WARNINGS)
Attachment #704126 - Attachment is obsolete: true
Attachment #704126 - Flags: review?(jfkthame)
Attachment #704138 - Flags: review?(jfkthame)
(oops, that last version had an unnecessary chunk, for nsSVGGlyphFrame.cpp -- there's no need to cast the result of GetTextRunUnitsFactor() because I'm already changing its return type in nsSVGGlyphFrame.h.)
Attachment #704138 - Attachment is obsolete: true
Attachment #704138 - Flags: review?(jfkthame)
Attachment #704146 - Flags: review?(jfkthame)
I'd rather convert everything (including nsDeviceContext::AppUnitsPerCSSPixel()) to be float as I said in bug 833037.
Even if we end up fixing bug 833037, I think there's still value in converting these (few, targeted) variables to have a consistent type before we make that broader float-switchover, so that if we e.g. end up backing out bug 833037, we're left in a consistent state.

Also, incidentally, I believe this bug's warning is the last remaining warning that's keeping /gfx/thebes from being marked as FAIL_ON_WARNINGS (woot!) for Linux/Android, on m-i. (see bug 832545 where that will happen)
Comment on attachment 704146 [details] [diff] [review]
option 2 v3: convert everything to be signed

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

I'm happier going to consistently signed than unsigned here. Although appUnitsPerDev itself can never be negative, we may use it in arithmetic involving potentially-negative coordinates, which means this factor could "impose" its unsignedness on the entire expression in such cases. Chances are that as long as everything happens in int-sized variables, twos-complement overflow etc would end up giving us the "expected" result anyway, but it seems wrong to be relying on that.

(Also note that if/when we make this a floating-point value, it will definitely lose its unsignedness. So if that's a problem for any reason - which I doubt - we may as well find out now...)

r=me, but please fix the assertion (see below).

::: gfx/thebes/gfxFont.cpp
@@ +4703,5 @@
>      : gfxShapedText(aLength, aFlags, aParams->mAppUnitsPerDevUnit)
>      , mUserData(aParams->mUserData)
>      , mFontGroup(aFontGroup)
>  {
> +    NS_ASSERTION(mAppUnitsPerDevUnit >= 0, "Invalid app unit scale");

This should be asserting >, not >=. I suspect bad things could happen if appUnitsPerDev were ever zero.

::: layout/generic/nsTextFrameThebes.cpp
@@ +2031,5 @@
>    gfxTextRunFactory::Parameters params =
>        { mContext, finalUserData, &skipChars,
> +        textBreakPointsAfterTransform.Elements(),
> +        textBreakPointsAfterTransform.Length(),
> +        int32_t(firstFrame->PresContext()->AppUnitsPerDevPixel())};

I guess this is fine for now, though I'd be somewhat tempted to extend this change deeper and make these values signed in PresContext (etc) as well. Let's not try to go there at the moment, though; that sounds like it could be a pretty deep rabbit-hole.

::: layout/svg/nsSVGGlyphFrame.h
@@ +216,1 @@
>    

How about losing the unwanted whitespace while you're here?
Attachment #704146 - Flags: review?(jfkthame) → review+
Attachment #704029 - Flags: review?(jfkthame)
Yeah, I agreed that signed arithmetic is probably safer here.

I fixed the assertion and the whitespace that you pointed out, & landed "option 2":

(I also reverted the attached patch's change to nsSVGGlyphFrame.cpp, because that was just a whitespace-only indentation change.  That change was in the attached patch because originally, while writing the patch, I had made a temporary tweak to that file, and I left behind a straggling indent-change.)
Attachment #704029 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d26d1c77e47e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.