Fix build warnings in layout

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

(Blocks 1 bug)

unspecified
mozilla19
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Assignee

Description

7 years ago
Posted patch Patch (obsolete) — Splinter Review
No description provided.
Attachment #669420 - Flags: review?(roc)
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?
Assignee

Comment 2

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

Comment 3

7 years ago
Posted patch PatchSplinter Review
Fixed the nits.
Attachment #669420 - Attachment is obsolete: true
Attachment #669420 - Flags: review?(roc)
Attachment #669856 - Flags: review?(roc)
Assignee

Comment 4

7 years ago
Leaving open for gcc warnings.
https://hg.mozilla.org/integration/mozilla-inbound/rev/53edf545ee94
Whiteboard: [leave open]
Assignee

Updated

7 years ago
Attachment #669856 - Flags: checkin+
Assignee

Comment 6

7 years ago
Attachment #670575 - Flags: review?(roc)
Assignee

Updated

7 years ago
Attachment #670575 - Flags: checkin+
Assignee

Comment 8

7 years ago
Backed out because more warnings have been added.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce995fa8292c
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
Assignee

Comment 10

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

Updated

7 years ago
Attachment #671284 - Flags: checkin+
Assignee

Updated

7 years ago
Attachment #671302 - Flags: checkin+
Depends on: 801566

Updated

6 years ago
Duplicate of this bug: 729759

Comment 19

6 years ago
Can this bug be closed now? It seems all the patches have landed, and that this bug isn't being used as meta bug.
Flags: needinfo?(dzbarsky)
Assignee

Comment 20

6 years ago
Yep.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(dzbarsky)
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.