729781 - FrameLayerBuilder.cpp:1529:91: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

layout/base/FrameLayerBuilder.cpp: In member function ‘void mozilla::{anonymous}::ContainerState::InvalidateForLayerChange(nsDisplayItem*, mozilla::layers::Layer*)’:
layout/base/FrameLayerBuilder.cpp:1529:91: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

The code in question:
> 1529     NS_ASSERTION(appUnitsPerDevPixel ==
> 1530                    mContainerFrame->PresContext()->AppUnitsPerDevPixel(),
> 1531                  "app units per dev pixel should be constant in a container");

* appUnitsPerDevPixel is a signed value (PRInt32) (because it's potentially obtained via nsDisplayZoom::GetParentAppUnitsPerDevPixel() which returns a PRInt32)
* PresContext()->AppUnitsPerDevPixel() returns an unsigned value (PRUint32)

Simplest solution is to just cast one of the values to the other one's type.

(I think this was introduced by
  http://hg.mozilla.org/mozilla-central/rev/8a2432d7f8f3
which changed PresContext::AppUnitsPerDevPixel() to return PRUint32)

Comment 1

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

Attachment: fix

Comment 2

[Timothy Nikkel (:tnikkel)]

Instead of just papering over this warning can we make the types consistent? Ie make AppUnitsPerDevPixel in FrameLayerBuilder.cpp return PRUint32, which means display zoom items should store their values as PRUint32's, which means we have to change nsSubDocumentFrame::BuildDisplayList, which means changing ConvertAppUnitsRoundOut to take PRUint32's. This is just continuing what 8a2432d7f8f3 started by converting appunits per dev pixel into an unsigned int.

Comment 3

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

That works too.  I think I looked ~2 layers into Comment 2 and saw signed integers, and stopped looking since it was just for code inside of an assertion anyway. :)  But yeah, taking 8a2432d7f8f3 to its logical conclusion sounds reasonable.

Comment 4

[:Cykesiopka]

From what I can see:
  - The assertion was removed in Bug 733607
  - nsDisplayZoom::GetParentAppUnitsPerDevPixel() returns a int32_t
  - PresContext()->AppUnitsPerDevPixel() returns a nscoord now
  - In nsCoord.h, nscoord can equal int32_t or a float, but since #define NS_COORD_IS_FLOAT is commented out, it's a int32_t
  - ConvertAppUnitsRoundOut() all take int_32t parameters now

So I'm guessing this bug can be closed now...?

Comment 5

[:Cykesiopka]

(In reply to Cykesiopka from comment #4)
>   - ConvertAppUnitsRoundOut() all take int_32t parameters now

int32_t, sorry.

Comment 6

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

(In reply to Cykesiopka from comment #4)
> From what I can see:
>   - The assertion was removed in Bug 733607
> So I'm guessing this bug can be closed now...?

Cool -- if the assertion's been removed, then that trivially fixes this. :)