799407 - Fix build warnings in layout

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 669420 [details] [diff] [review]
Patch

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

Cool!

::: layout/base/nsCSSRendering.cpp
@@ +1899,5 @@
>                                     appUnitsPerPixel));
>    }
>  
>    // Compute gradient shape: the x and y radii of an ellipse.
> +  double radiusX = 0, radiusY = 0;

How about initializing radiusX and radiusY in the default case of the switch statement so if we forget to initialize in some case, we still get a warning?

::: layout/generic/nsBlockFrame.cpp
@@ +1559,3 @@
>              NS_STYLE_DIRECTION_LTR == aDirection) ||
>             (NS_STYLE_TEXT_ALIGN_END == aAlignment &&
>              NS_STYLE_DIRECTION_RTL == aDirection)) &&

Fix indent

::: layout/generic/nsViewportFrame.cpp
@@ +236,5 @@
>    // to reflect the available space for the fixed items
>    nsHTMLReflowState reflowState(aReflowState);
> +#ifdef DEBUG
> +  nsPoint offset =
> +#endif

DebugOnly<> doesn't work here?

Comment 2

[David Zbarsky (:dzbarsky)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #1)
> 
> ::: layout/generic/nsViewportFrame.cpp
> @@ +236,5 @@
> >    // to reflect the available space for the fixed items
> >    nsHTMLReflowState reflowState(aReflowState);
> > +#ifdef DEBUG
> > +  nsPoint offset =
> > +#endif
> 
> DebugOnly<> doesn't work here?

No, probably because DebugOnly doesn't define |operator.|

Comment 3

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Fixed the nits.

Comment 4

[David Zbarsky (:dzbarsky)]

Leaving open for gcc warnings.
https://hg.mozilla.org/integration/mozilla-inbound/rev/53edf545ee94

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/53edf545ee94

Comment 6

[David Zbarsky (:dzbarsky)]

Attachment: Fix warnings in layout/generic

Comment 7

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3884ab03451d

Comment 8

[David Zbarsky (:dzbarsky)]

Backed out because more warnings have been added.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce995fa8292c

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 670575 [details] [diff] [review]
Fix warnings in layout/generic

>diff --git a/layout/generic/nsTextFrameThebes.cpp b/layout/generic/nsTextFrameThebes.cpp
>--- a/layout/generic/nsTextFrameThebes.cpp
>+++ b/layout/generic/nsTextFrameThebes.cpp
>-  nscolor overrideColor;
>+  nscolor overrideColor = nscolor(0);

Drive-by nit: this probably wants to be NS_RGBA(0,0,0,0), not nscolor(0).  (They evaluate to the same thing, but the RGBA form is more human-readable and makes fewer assumptions about the underlying representation of nscolor).

Note that we don't have any instances of "nscolor(0)" in the codebase right now:
 https://mxr.mozilla.org/mozilla-central/search?string=nscolor%280%29
but we do have ~50 each of NS_RGB(0,0,0) and NS_RGBA(0,0,0,0)
 https://mxr.mozilla.org/mozilla-central/search?string=NS_RGB%280%2C
 https://mxr.mozilla.org/mozilla-central/search?string=NS_RGBA%280%2C

Comment 10

[David Zbarsky (:dzbarsky)]

I pushed the warning fixes but didn't enable FAIL_ON_WARNINGS.
https://hg.mozilla.org/integration/mozilla-inbound/rev/78022080a795

Comment 11

[David Zbarsky (:dzbarsky)]

Attachment: Turn on fail_on_warnings for layout/generic

Comment 12

[David Zbarsky (:dzbarsky)]

Pushed the layout/generic patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97bda7166b3a

Comment 13

[David Zbarsky (:dzbarsky)]

Attachment: Enable fail-on-warnings everywhere except layout/style

Comment 14

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/784bb7c5ea9c

Comment 15

[David Zbarsky (:dzbarsky)]

Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d85b73ade155

Comment 16

[David Zbarsky (:dzbarsky)]

and relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a96deba7de41

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/78022080a795
https://hg.mozilla.org/mozilla-central/rev/97bda7166b3a
https://hg.mozilla.org/mozilla-central/rev/a96deba7de41

Comment 19

[:Cykesiopka]

Can this bug be closed now? It seems all the patches have landed, and that this bug isn't being used as meta bug.

Comment 20

[David Zbarsky (:dzbarsky)]

Yep.