Closed Bug 1267524 Opened 8 years ago Closed 8 years ago

Use class member initializer list for style structs

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

Details

Attachments

(20 files)

MozReview Request: Bug 1267524 Part 1 - Use member initializer list for nsStyleOutline.
58 bytes, text/x-review-board-request
heycam
: review+
Details
MozReview Request: Bug 1267524 Part 2 - Use member initializer list for nsStyleXUL.
58 bytes, text/x-review-board-request
heycam
: review+
Details
MozReview Request: Bug 1267524 Part 3 - Use member initializer list for nsStyleColumn.
58 bytes, text/x-review-board-request
heycam
: review+
Details
MozReview Request: Bug 1267524 Part 4.2 - Use member initializer list for nsStyleSVG.
58 bytes, text/x-review-board-request
heycam
: review+
Details
MozReview Request: Bug 1267524 Part 5 - Use member initializer list for nsStyleSVGReset.
58 bytes, text/x-review-board-request
heycam
: review+
Details
MozReview Request: Bug 1267524 Part 6 - Use member initializer list for nsStylePosition.
58 bytes, text/x-review-board-request
heycam
: review+
Details
MozReview Request: Bug 1267524 Part 7 - Use member initializer list for nsStyleTable.
58 bytes, text/x-review-board-request
heycam
: review+
Details
MozReview Request: Bug 1267524 Part 8 - Use member initializer list for nsStyleTableBorder.
58 bytes, text/x-review-board-request
heycam
: review+
Details
MozReview Request: Bug 1267524 Part 9 - Use member initializer list for nsStyleColor.
58 bytes, text/x-review-board-request
heycam
: review+
Details
MozReview Request: Bug 1267524 Part 10 - Use member initializer list for nsStyleDisplay.
58 bytes, text/x-review-board-request
heycam
: review+
Details
MozReview Request: Bug 1267524 Part 11 - Use member initializer list for nsStyleVisibility.
58 bytes, text/x-review-board-request
heycam
: review+
Details
MozReview Request: Bug 1267524 Part 12 - Use member initializer list for nsStyleContent.
58 bytes, text/x-review-board-request
heycam
: review+
Details
MozReview Request: Bug 1267524 Part 13 - Use member initializer list for nsStyleTextReset.
58 bytes, text/x-review-board-request
heycam
: review+
Details
MozReview Request: Bug 1267524 Part 14 - Use member initializer list for nsStyleText.
58 bytes, text/x-review-board-request
heycam
: review+
Details
MozReview Request: Bug 1267524 Part 15 - Use member initializer list for nsStyleUserInterface.
58 bytes, text/x-review-board-request
heycam
: review+
Details
MozReview Request: Bug 1267524 Part 16 - Use member initializer list for nsStyleUIReset.
58 bytes, text/x-review-board-request
heycam
: review+
Details
MozReview Request: Bug 1267524 Part 17 - Use member initializer list for nsStyleVariables.
58 bytes, text/x-review-board-request
heycam
: review+
Details
MozReview Request: Bug 1267524 Part 4.1 - Add Reset() and rewrite methods for nsStyleSVGPaint.
58 bytes, text/x-review-board-request
heycam
: review+
Details
MozReview Request: Bug 1267524 Part 18 - Remove "void" from zero argument functions.
58 bytes, text/x-review-board-request
heycam
: review+
Details
MozReview Request: Bug 1267524 Part 19 - Move nsStyleCoord members to initializer list.
58 bytes, text/x-review-board-request
heycam
: review+
Details 
Many style structs like nsStyleDisplay initialize their members in the body of the constructor [1]. nsStyleDisplay has mixed usage [2].

We should make those members initialize in the member initializer list. It's more C++-ish, and could be faster. Compilers also output warnings when the initialization of the struct members is out of order.

[1] https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/layout/style/nsStyleStruct.cpp#3567
[2] https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/layout/style/nsStyleStruct.cpp#2838
mCachedOutlineWidth was not initialized, I set it to 0.

Review commit: https://reviewboard.mozilla.org/r/48903/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48903/
Attachment #8745203 - Flags: review?(cam)
Attachment #8745204 - Flags: review?(cam)
Attachment #8745205 - Flags: review?(cam)
Attachment #8745206 - Flags: review?(cam)
Attachment #8745207 - Flags: review?(cam)
Attachment #8745208 - Flags: review?(cam)
Attachment #8745209 - Flags: review?(cam)
Attachment #8745210 - Flags: review?(cam)
Attachment #8745211 - Flags: review?(cam)
Attachment #8745212 - Flags: review?(cam)
Attachment #8745213 - Flags: review?(cam)
Attachment #8745214 - Flags: review?(cam)
Attachment #8745215 - Flags: review?(cam)
Attachment #8745216 - Flags: review?(cam)
Attachment #8745217 - Flags: review?(cam)
Attachment #8745218 - Flags: review?(cam)
Attachment #8745219 - Flags: review?(cam)
In the initializer list of the copy constructor, mStrokeDasharray and
mStrokeDasharrayLength are both initialized to nullptr and 0
respectively. If aSource.mStrokeDasharray has value, they're further be
set to proper values later in the function body. In this way, the two
'else' clauses can be removed.

Review commit: https://reviewboard.mozilla.org/r/48909/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48909/
David, am I right in thinking that initializers weren't traditionally used on style structs because we used to rely on the zero-initialization that pres arena allocation did, and that using initializers would maybe obscure that fact?  And that now we explicitly initialize everything, we should indeed just go ahead and use initializers?
Flags: needinfo?(dbaron)
https://reviewboard.mozilla.org/r/48929/#review45787

::: layout/style/nsStyleStruct.cpp:3587
(Diff revision 1)
> -  mTextEmphasisColor = aContext.DefaultColor();
> -  mWebkitTextFillColor = aContext.DefaultColor();
> -  mWebkitTextStrokeColor = aContext.DefaultColor();
> -  mControlCharacterVisibility = nsCSSParser::ControlCharVisibilityDefault();
> -
>    mWordSpacing.SetCoordValue(0);

One general notice is that I leave nsStyleCoord untouched because we lack a constructor to construct a nsStyleCoord with SetCoordValue(0). However we can construct nsStyleCoord with SetNormalValue() by nsStyleCoord(eStyleUnit_Normal).

If we feel constructing nsStyleCoord in two pass is awkard and it's a better style to construct by something like nsStyleCoord(eStyleUnit_Normal) in the initializer list, I can make it happen in next patch set.

Perhaps we could modify [1] to let it accept eStyleUnit_Coord. I don't see the reason behind the comment.

[1] https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/layout/style/nsStyleCoord.cpp#25-26
FWIW, Some of the style structs like nsStyleFont already use initializer list [1].

[1] https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/layout/style/nsStyleStruct.cpp#118
(In reply to Cameron McCormack (:heycam) from comment #18)
> David, am I right in thinking that initializers weren't traditionally used
> on style structs because we used to rely on the zero-initialization that
> pres arena allocation did, and that using initializers would maybe obscure
> that fact?  And that now we explicitly initialize everything, we should
> indeed just go ahead and use initializers?

I'm not really sure of the history, but using member initializers makes sense.


(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #0)
> Compilers also output warnings when the
> initialization of the struct members is out of order.

One note here:  the members are initialized in order even when the declarations are out of order.  So the compiler warns because having them listed out of order is confusing code, because things happen in a different order from the order they're written.
Flags: needinfo?(dbaron)
Comment on attachment 8745203 [details]
MozReview Request: Bug 1267524 Part 1 - Use member initializer list for nsStyleOutline.

https://reviewboard.mozilla.org/r/48903/#review46579
Attachment #8745203 - Flags: review?(cam) → review+
Attachment #8745204 - Flags: review?(cam) → review+
Comment on attachment 8745204 [details]
MozReview Request: Bug 1267524 Part 2 - Use member initializer list for nsStyleXUL.

https://reviewboard.mozilla.org/r/48905/#review46581
Comment on attachment 8745205 [details]
MozReview Request: Bug 1267524 Part 3 - Use member initializer list for nsStyleColumn.

https://reviewboard.mozilla.org/r/48907/#review46583

::: layout/style/nsStyleStruct.cpp:814
(Diff revision 1)
> +  : mColumnCount(NS_STYLE_COLUMN_COUNT_AUTO)
> +  , mColumnRuleColor(NS_RGB(0, 0, 0))
> +  , mColumnRuleStyle(NS_STYLE_BORDER_STYLE_NONE)
> +  , mColumnFill(NS_STYLE_COLUMN_FILL_BALANCE)
> +  , mColumnRuleColorIsForeground(true)
> +  , mColumnRuleWidth((StaticPresData::Get()->GetBorderWidthTable())[NS_STYLE_BORDER_WIDTH_MEDIUM])

Let's wrap this to fit 80 columns:

  , mColumnRuleWidth(StaticPresData::Get()->
                       GetBorderWidthTable()[NS_STYLE_BORDER_WIDTH_MEDIUM])
Attachment #8745205 - Flags: review?(cam) → review+
Attachment #8745206 - Flags: review?(cam)
Comment on attachment 8745206 [details]
MozReview Request: Bug 1267524 Part 4.2 - Use member initializer list for nsStyleSVG.

https://reviewboard.mozilla.org/r/48909/#review46585

::: layout/style/nsStyleStruct.cpp:876
(Diff revision 1)
>  
>  // --------------------
>  // nsStyleSVG
>  //
>  nsStyleSVG::nsStyleSVG(StyleStructContext aContext)
> +  : mFill(eStyleSVGPaintType_Color)

Since it might not be obvious that it is initialized, add a comment at the end of this line saying that it sets it to NS_RGB(0, 0, 0).

::: layout/style/nsStyleStruct.cpp
(Diff revision 1)
>  
> -  mMarkerEnd = aSource.mMarkerEnd;
> -  mMarkerMid = aSource.mMarkerMid;
> -  mMarkerStart = aSource.mMarkerStart;
> -
> -  mStrokeDasharrayLength = aSource.mStrokeDasharrayLength;

Can you add an assertion in here to verify that if aSource.mStrokeDasharray is null, then aSource.mStrokeDasharrayLength is 0?

In fact I think it would be better just to use an nsTArray to store the dasharray, then we don't need to worry about manual memory management for the array.  Please file a followup to do that.

::: layout/style/nsStyleStruct.cpp
(Diff revision 1)
>  {
>    if (mType == eStyleSVGPaintType_Server) {
>      this->~nsStyleSVGPaint();
> -    new (this) nsStyleSVGPaint();
> +    new (this) nsStyleSVGPaint(aType);
>    }
> -  mType = aType;

Why don't we need to set mType any more?  If the old type is not eStyleSVGPaintType_Server, don't we still need to set the new type?

I don't really like the explicit destructor / placement new calls.  What do you think about:

* making the constructor just initialize mType to nsStyleSVGPaint(0) and then immediately call SetType(aType)

* adding a Reset method that does the work that the constructor currently does, and then sets mType to nsStyleSVGPaint(0)

* making SetType first call Reset() and then do the work that is currently in the constructor

This is more like what nsCSSValue does.
Attachment #8745207 - Flags: review?(cam) → review+
Comment on attachment 8745207 [details]
MozReview Request: Bug 1267524 Part 5 - Use member initializer list for nsStyleSVGReset.

https://reviewboard.mozilla.org/r/48911/#review46587
Comment on attachment 8745208 [details]
MozReview Request: Bug 1267524 Part 6 - Use member initializer list for nsStylePosition.

https://reviewboard.mozilla.org/r/48913/#review46589
Attachment #8745208 - Flags: review?(cam) → review+
Comment on attachment 8745209 [details]
MozReview Request: Bug 1267524 Part 7 - Use member initializer list for nsStyleTable.

https://reviewboard.mozilla.org/r/48915/#review46591
Attachment #8745209 - Flags: review?(cam) → review+
Comment on attachment 8745210 [details]
MozReview Request: Bug 1267524 Part 8 - Use member initializer list for nsStyleTableBorder.

https://reviewboard.mozilla.org/r/48917/#review46593

::: layout/style/nsStyleStruct.cpp:1805
(Diff revision 1)
> -  mCaptionSide = NS_STYLE_CAPTION_SIDE_TOP;
> -  mBorderSpacingCol = 0;
> -  mBorderSpacingRow = 0;
>  }
>  
> -nsStyleTableBorder::~nsStyleTableBorder(void) 
> +nsStyleTableBorder::~nsStyleTableBorder(void)

While you're touching that line, you can remove the "void".
Attachment #8745210 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #29)
> While you're touching that line, you can remove the "void".

That applies to some of the previous patches, too.
Comment on attachment 8745211 [details]
MozReview Request: Bug 1267524 Part 9 - Use member initializer list for nsStyleColor.

https://reviewboard.mozilla.org/r/48919/#review46595
Attachment #8745211 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #29)
> While you're touching that line, you can remove the "void".

A cleanup like that could also be an extra patch on top.

(In C, () means "we don't specify what the arguments are" and (void) means "no arguments", but in C++ they both mean "no arguments".  We consider using (void) poor style in C++.)
Comment on attachment 8745212 [details]
MozReview Request: Bug 1267524 Part 10 - Use member initializer list for nsStyleDisplay.

https://reviewboard.mozilla.org/r/48921/#review46597

::: layout/style/nsStyleStruct.cpp:2833
(Diff revision 1)
> +  , mOriginalDisplay(mDisplay)
> +  , mContain(NS_STYLE_CONTAIN_NONE)

Regarding the initialization order issue that David mentioned, our compiler warnings (turned into errors on some platforms) would catch this, if mDisplay were initialized after mOriginalDisplay (resulting in the assignment to mOriginalDisplay using an uninitialized value).  But I don't think there's much to gain from initializing mOriginalDisplay (and mOriginalFloats) in this way, so let's just use NS_STYLE_DISPLAY_INLINE (and NS_STYLE_FLOAT_NONE).
Attachment #8745212 - Flags: review?(cam) → review+
Comment on attachment 8745213 [details]
MozReview Request: Bug 1267524 Part 11 - Use member initializer list for nsStyleVisibility.

https://reviewboard.mozilla.org/r/48923/#review46599
Attachment #8745213 - Flags: review?(cam) → review+
Comment on attachment 8745214 [details]
MozReview Request: Bug 1267524 Part 12 - Use member initializer list for nsStyleContent.

https://reviewboard.mozilla.org/r/48925/#review46601

::: layout/style/nsStyleStruct.cpp:3332
(Diff revision 1)
>  //-----------------------
>  // nsStyleContent
>  //
>  
>  nsStyleContent::nsStyleContent(StyleStructContext aContext)
> -  : mMarkerOffset(),
> +  : mMarkerOffset()

mMarkerOffset will be default constructed without being listed here, so we can remove this.
Attachment #8745214 - Flags: review?(cam) → review+
Comment on attachment 8745215 [details]
MozReview Request: Bug 1267524 Part 13 - Use member initializer list for nsStyleTextReset.

https://reviewboard.mozilla.org/r/48927/#review46603
Attachment #8745215 - Flags: review?(cam) → review+
Attachment #8745216 - Flags: review?(cam) → review+
Comment on attachment 8745216 [details]
MozReview Request: Bug 1267524 Part 14 - Use member initializer list for nsStyleText.

https://reviewboard.mozilla.org/r/48929/#review46605
https://reviewboard.mozilla.org/r/48929/#review45787

> One general notice is that I leave nsStyleCoord untouched because we lack a constructor to construct a nsStyleCoord with SetCoordValue(0). However we can construct nsStyleCoord with SetNormalValue() by nsStyleCoord(eStyleUnit_Normal).
> 
> If we feel constructing nsStyleCoord in two pass is awkard and it's a better style to construct by something like nsStyleCoord(eStyleUnit_Normal) in the initializer list, I can make it happen in next patch set.
> 
> Perhaps we could modify [1] to let it accept eStyleUnit_Coord. I don't see the reason behind the comment.
> 
> [1] https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/layout/style/nsStyleCoord.cpp#25-26

mSomeValue(eStyleUnit_Normal) is better than a separate SetNormalValue() call, I think.

For coord values, that comment might be because we have some support in nsCoord.h for nscoord being float rather than int32_t, although I am sure nobody has tested that configuration for a long time and it's unlikely we'll ever use it.  If we just added a second argument to the nsStyleCoord(nscoord) constructor, it would be ambiguous with the one taking an int32_t.

We could add a static method to nsStyleCoord:

  static nsStyleCoord CoordValue(nscoord aValue) { nsStyleCoord v; v.SetCoordValue(aValue); return v; }
  
that we could then use in initializer lists.
Comment on attachment 8745217 [details]
MozReview Request: Bug 1267524 Part 15 - Use member initializer list for nsStyleUserInterface.

https://reviewboard.mozilla.org/r/48931/#review46609

::: layout/style/nsStyleStruct.cpp:3783
(Diff revision 1)
>  nsStyleUserInterface::nsStyleUserInterface(StyleStructContext aContext)
> -{ 
> +  : mUserInput(NS_STYLE_USER_INPUT_AUTO)
> +  , mUserModify(NS_STYLE_USER_MODIFY_READ_ONLY)
> +  , mUserFocus(NS_STYLE_USER_FOCUS_NONE)
> +  , mPointerEvents(NS_STYLE_POINTER_EVENTS_AUTO)
> +  , mCursor(NS_STYLE_CURSOR_AUTO) // fix for bugzilla bug 51113

This old comment isn't necessary, in my opinion.  (There's nothing special about setting the property to its initial value, here.)  So please remove it.
Attachment #8745217 - Flags: review?(cam) → review+
Comment on attachment 8745218 [details]
MozReview Request: Bug 1267524 Part 16 - Use member initializer list for nsStyleUIReset.

https://reviewboard.mozilla.org/r/48933/#review46611
Attachment #8745218 - Flags: review?(cam) → review+
Comment on attachment 8745219 [details]
MozReview Request: Bug 1267524 Part 17 - Use member initializer list for nsStyleVariables.

https://reviewboard.mozilla.org/r/48935/#review46613
Attachment #8745219 - Flags: review?(cam) → review+
https://reviewboard.mozilla.org/r/48917/#review46593

> While you're touching that line, you can remove the "void".

I'll remove all "void" from zero-argument functions in a separate patch.
Blocks: 1269175
https://reviewboard.mozilla.org/r/48909/#review46585

> Since it might not be obvious that it is initialized, add a comment at the end of this line saying that it sets it to NS_RGB(0, 0, 0).

Will do.

> Can you add an assertion in here to verify that if aSource.mStrokeDasharray is null, then aSource.mStrokeDasharrayLength is 0?
> 
> In fact I think it would be better just to use an nsTArray to store the dasharray, then we don't need to worry about manual memory management for the array.  Please file a followup to do that.

OK. Will add assertion to guantee the correctness of aSource.mStrokeDasharray and aSource.mStrokeDasharrayLength.

File bug 1269175 for using nsTArray for nsStyleSVG::mStrokeDasharray.

> Why don't we need to set mType any more?  If the old type is not eStyleSVGPaintType_Server, don't we still need to set the new type?
> 
> I don't really like the explicit destructor / placement new calls.  What do you think about:
> 
> * making the constructor just initialize mType to nsStyleSVGPaint(0) and then immediately call SetType(aType)
> 
> * adding a Reset method that does the work that the constructor currently does, and then sets mType to nsStyleSVGPaint(0)
> 
> * making SetType first call Reset() and then do the work that is currently in the constructor
> 
> This is more like what nsCSSValue does.

My bad. We need to set mType. I agree the explicit destructor / placement new calls is not appealing. See my Part 4.1 for the rewrite of nsStyleSVGPaint.
https://reviewboard.mozilla.org/r/48929/#review45787

> mSomeValue(eStyleUnit_Normal) is better than a separate SetNormalValue() call, I think.
> 
> For coord values, that comment might be because we have some support in nsCoord.h for nscoord being float rather than int32_t, although I am sure nobody has tested that configuration for a long time and it's unlikely we'll ever use it.  If we just added a second argument to the nsStyleCoord(nscoord) constructor, it would be ambiguous with the one taking an int32_t.
> 
> We could add a static method to nsStyleCoord:
> 
>   static nsStyleCoord CoordValue(nscoord aValue) { nsStyleCoord v; v.SetCoordValue(aValue); return v; }
>   
> that we could then use in initializer lists.

I just found that we do have a inline nsStyleCoord constructor in the header [1] for setting the type to eStyleUnit_Coord and value. Since most of the parts in this bug have been reviewed, I'll move the rest of the nsStyleCoord to initilization list in a separate patch.

[1] https://dxr.mozilla.org/mozilla-central/rev/1461a4071341c282afcf7b72e33036412d2251d4/layout/style/nsStyleCoord.h#321
Add Reset() to eliminate the explicit destructor / placement new calls
in SetType().

Review commit: https://reviewboard.mozilla.org/r/49935/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49935/
Attachment #8745206 - Attachment description: MozReview Request: Bug 1267524 Part 4 - Use member initializer list for nsStyleSVG. → MozReview Request: Bug 1267524 Part 4.2 - Use member initializer list for nsStyleSVG.
Attachment #8745206 - Flags: review?(cam)
Comment on attachment 8745203 [details]
MozReview Request: Bug 1267524 Part 1 - Use member initializer list for nsStyleOutline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48903/diff/1-2/
Comment on attachment 8745204 [details]
MozReview Request: Bug 1267524 Part 2 - Use member initializer list for nsStyleXUL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48905/diff/1-2/
Comment on attachment 8745205 [details]
MozReview Request: Bug 1267524 Part 3 - Use member initializer list for nsStyleColumn.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48907/diff/1-2/
Comment on attachment 8745206 [details]
MozReview Request: Bug 1267524 Part 4.2 - Use member initializer list for nsStyleSVG.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48909/diff/1-2/
Comment on attachment 8745207 [details]
MozReview Request: Bug 1267524 Part 5 - Use member initializer list for nsStyleSVGReset.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48911/diff/1-2/
Comment on attachment 8745208 [details]
MozReview Request: Bug 1267524 Part 6 - Use member initializer list for nsStylePosition.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48913/diff/1-2/
Comment on attachment 8745209 [details]
MozReview Request: Bug 1267524 Part 7 - Use member initializer list for nsStyleTable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48915/diff/1-2/
Comment on attachment 8745210 [details]
MozReview Request: Bug 1267524 Part 8 - Use member initializer list for nsStyleTableBorder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48917/diff/1-2/
Comment on attachment 8745211 [details]
MozReview Request: Bug 1267524 Part 9 - Use member initializer list for nsStyleColor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48919/diff/1-2/
Comment on attachment 8745212 [details]
MozReview Request: Bug 1267524 Part 10 - Use member initializer list for nsStyleDisplay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48921/diff/1-2/
Comment on attachment 8745213 [details]
MozReview Request: Bug 1267524 Part 11 - Use member initializer list for nsStyleVisibility.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48923/diff/1-2/
Comment on attachment 8745214 [details]
MozReview Request: Bug 1267524 Part 12 - Use member initializer list for nsStyleContent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48925/diff/1-2/
Comment on attachment 8745215 [details]
MozReview Request: Bug 1267524 Part 13 - Use member initializer list for nsStyleTextReset.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48927/diff/1-2/
Comment on attachment 8745216 [details]
MozReview Request: Bug 1267524 Part 14 - Use member initializer list for nsStyleText.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48929/diff/1-2/
Comment on attachment 8745217 [details]
MozReview Request: Bug 1267524 Part 15 - Use member initializer list for nsStyleUserInterface.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48931/diff/1-2/
Comment on attachment 8745218 [details]
MozReview Request: Bug 1267524 Part 16 - Use member initializer list for nsStyleUIReset.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48933/diff/1-2/
Comment on attachment 8745219 [details]
MozReview Request: Bug 1267524 Part 17 - Use member initializer list for nsStyleVariables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48935/diff/1-2/
Attachment #8747572 - Flags: review?(cam)
Attachment #8747573 - Flags: review?(cam)
Attachment #8747574 - Flags: review?(cam)
I got try fails. I guess nsStyleSVGPaint needs a copy constructor to get the mPaint.mPaintServer reference count correct.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9be669ce446d
Comment on attachment 8747572 [details]
MozReview Request: Bug 1267524 Part 4.1 - Add Reset() and rewrite methods for nsStyleSVGPaint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49935/diff/1-2/
Comment on attachment 8745206 [details]
MozReview Request: Bug 1267524 Part 4.2 - Use member initializer list for nsStyleSVG.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48909/diff/2-3/
Comment on attachment 8745207 [details]
MozReview Request: Bug 1267524 Part 5 - Use member initializer list for nsStyleSVGReset.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48911/diff/2-3/
Comment on attachment 8745208 [details]
MozReview Request: Bug 1267524 Part 6 - Use member initializer list for nsStylePosition.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48913/diff/2-3/
Comment on attachment 8745209 [details]
MozReview Request: Bug 1267524 Part 7 - Use member initializer list for nsStyleTable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48915/diff/2-3/
Comment on attachment 8745210 [details]
MozReview Request: Bug 1267524 Part 8 - Use member initializer list for nsStyleTableBorder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48917/diff/2-3/
Comment on attachment 8745211 [details]
MozReview Request: Bug 1267524 Part 9 - Use member initializer list for nsStyleColor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48919/diff/2-3/
Comment on attachment 8745212 [details]
MozReview Request: Bug 1267524 Part 10 - Use member initializer list for nsStyleDisplay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48921/diff/2-3/
Comment on attachment 8745213 [details]
MozReview Request: Bug 1267524 Part 11 - Use member initializer list for nsStyleVisibility.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48923/diff/2-3/
Comment on attachment 8745214 [details]
MozReview Request: Bug 1267524 Part 12 - Use member initializer list for nsStyleContent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48925/diff/2-3/
Comment on attachment 8745215 [details]
MozReview Request: Bug 1267524 Part 13 - Use member initializer list for nsStyleTextReset.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48927/diff/2-3/
Comment on attachment 8745216 [details]
MozReview Request: Bug 1267524 Part 14 - Use member initializer list for nsStyleText.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48929/diff/2-3/
Comment on attachment 8745217 [details]
MozReview Request: Bug 1267524 Part 15 - Use member initializer list for nsStyleUserInterface.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48931/diff/2-3/
Comment on attachment 8745218 [details]
MozReview Request: Bug 1267524 Part 16 - Use member initializer list for nsStyleUIReset.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48933/diff/2-3/
Comment on attachment 8745219 [details]
MozReview Request: Bug 1267524 Part 17 - Use member initializer list for nsStyleVariables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48935/diff/2-3/
Comment on attachment 8747573 [details]
MozReview Request: Bug 1267524 Part 18 - Remove "void" from zero argument functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49937/diff/1-2/
Comment on attachment 8747574 [details]
MozReview Request: Bug 1267524 Part 19 - Move nsStyleCoord members to initializer list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49939/diff/1-2/
Attachment #8745206 - Flags: review?(cam)
Comment on attachment 8745206 [details]
MozReview Request: Bug 1267524 Part 4.2 - Use member initializer list for nsStyleSVG.

https://reviewboard.mozilla.org/r/48909/#review46919

::: layout/style/nsStyleStruct.cpp:942
(Diff revisions 1 - 3)
>    , mStrokeWidthFromObject(aSource.mStrokeWidthFromObject)
>  {
>    MOZ_COUNT_CTOR(nsStyleSVG);
>  
> +  MOZ_ASSERT(bool(aSource.mStrokeDasharray) ==
> +             bool(aSource.mStrokeDasharrayLength));

Please add a message to the assertion.

::: layout/style/nsStyleStruct.cpp:2702
(Diff revisions 1 - 3)
> -bool nsStyleBackground::HasFixedBackground() const
> +bool nsStyleBackground::HasFixedBackground(nsIFrame* aFrame) const
>  {
>    NS_FOR_VISIBLE_IMAGE_LAYERS_BACK_TO_FRONT(i, mImage) {
>      const nsStyleImageLayers::Layer &layer = mImage.mLayers[i];
>      if (layer.mAttachment == NS_STYLE_IMAGELAYER_ATTACHMENT_FIXED &&
> -        !layer.mImage.IsEmpty()) {
> +        !layer.mImage.IsEmpty() &&
> +        !nsLayoutUtils::IsTransformed(aFrame)) {

Are these changes meant to be here?
Comment on attachment 8747572 [details]
MozReview Request: Bug 1267524 Part 4.1 - Add Reset() and rewrite methods for nsStyleSVGPaint.

https://reviewboard.mozilla.org/r/49935/#review46921
Attachment #8747572 - Flags: review?(cam) → review+
Comment on attachment 8747573 [details]
MozReview Request: Bug 1267524 Part 18 - Remove "void" from zero argument functions.

https://reviewboard.mozilla.org/r/49937/#review46923
Attachment #8747573 - Flags: review?(cam) → review+
Comment on attachment 8747574 [details]
MozReview Request: Bug 1267524 Part 19 - Move nsStyleCoord members to initializer list.

https://reviewboard.mozilla.org/r/49939/#review46925

::: layout/style/nsStyleStruct.h:1755
(Diff revision 2)
> +  // These members get their default constructors which initialize them to
> +  // representations of their respective initial value.
> +  // mGridTemplate{Rows,Columns}: false and empty arrays for 'none'
> +  // mGridTemplateAreas: nullptr for 'none'
> +  // mGrid{Column,Row}{Start,End}: false/0/empty values for 'auto'

I think this comment would be best left in the body of the constructor.  (Feels like it clutters the header file here a bit.)

::: layout/style/nsStyleStruct.cpp:2939
(Diff revision 2)
>    , mTransformStyle(aSource.mTransformStyle)
>    , mTransformBox(aSource.mTransformBox)
>    , mSpecifiedTransform(aSource.mSpecifiedTransform)
> +  , mTransformOrigin{ aSource.mTransformOrigin[0],
> +                      aSource.mTransformOrigin[1],
> +                       aSource.mTransformOrigin[2] }

Nit: this line is not aligned.
Attachment #8747574 - Flags: review?(cam) → review+
https://reviewboard.mozilla.org/r/48909/#review46919

> Please add a message to the assertion.

Will do.

> Are these changes meant to be here?

Yes. Mozreview will show everything changed including those from rebasing. If the diff is between origin and revision 3, they won't show in my part 4.2 :)
Comment on attachment 8745206 [details]
MozReview Request: Bug 1267524 Part 4.2 - Use member initializer list for nsStyleSVG.

https://reviewboard.mozilla.org/r/48909/#review46933
Attachment #8745206 - Flags: review+
https://reviewboard.mozilla.org/r/49939/#review46925

> I think this comment would be best left in the body of the constructor.  (Feels like it clutters the header file here a bit.)

OK. Let's not move it.

> Nit: this line is not aligned.

Nice catch!
Comment on attachment 8745203 [details]
MozReview Request: Bug 1267524 Part 1 - Use member initializer list for nsStyleOutline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48903/diff/2-3/
Comment on attachment 8745204 [details]
MozReview Request: Bug 1267524 Part 2 - Use member initializer list for nsStyleXUL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48905/diff/2-3/
Comment on attachment 8745205 [details]
MozReview Request: Bug 1267524 Part 3 - Use member initializer list for nsStyleColumn.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48907/diff/2-3/
Comment on attachment 8747572 [details]
MozReview Request: Bug 1267524 Part 4.1 - Add Reset() and rewrite methods for nsStyleSVGPaint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49935/diff/2-3/
Comment on attachment 8745206 [details]
MozReview Request: Bug 1267524 Part 4.2 - Use member initializer list for nsStyleSVG.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48909/diff/3-4/
Comment on attachment 8745207 [details]
MozReview Request: Bug 1267524 Part 5 - Use member initializer list for nsStyleSVGReset.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48911/diff/3-4/
Comment on attachment 8745208 [details]
MozReview Request: Bug 1267524 Part 6 - Use member initializer list for nsStylePosition.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48913/diff/3-4/
Comment on attachment 8745209 [details]
MozReview Request: Bug 1267524 Part 7 - Use member initializer list for nsStyleTable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48915/diff/3-4/
Comment on attachment 8745210 [details]
MozReview Request: Bug 1267524 Part 8 - Use member initializer list for nsStyleTableBorder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48917/diff/3-4/
Comment on attachment 8745211 [details]
MozReview Request: Bug 1267524 Part 9 - Use member initializer list for nsStyleColor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48919/diff/3-4/
Comment on attachment 8745212 [details]
MozReview Request: Bug 1267524 Part 10 - Use member initializer list for nsStyleDisplay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48921/diff/3-4/
Comment on attachment 8745213 [details]
MozReview Request: Bug 1267524 Part 11 - Use member initializer list for nsStyleVisibility.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48923/diff/3-4/
Comment on attachment 8745214 [details]
MozReview Request: Bug 1267524 Part 12 - Use member initializer list for nsStyleContent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48925/diff/3-4/
Comment on attachment 8745215 [details]
MozReview Request: Bug 1267524 Part 13 - Use member initializer list for nsStyleTextReset.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48927/diff/3-4/
Comment on attachment 8745216 [details]
MozReview Request: Bug 1267524 Part 14 - Use member initializer list for nsStyleText.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48929/diff/3-4/
Comment on attachment 8745217 [details]
MozReview Request: Bug 1267524 Part 15 - Use member initializer list for nsStyleUserInterface.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48931/diff/3-4/
Comment on attachment 8745218 [details]
MozReview Request: Bug 1267524 Part 16 - Use member initializer list for nsStyleUIReset.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48933/diff/3-4/
Comment on attachment 8745219 [details]
MozReview Request: Bug 1267524 Part 17 - Use member initializer list for nsStyleVariables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48935/diff/3-4/
Comment on attachment 8747573 [details]
MozReview Request: Bug 1267524 Part 18 - Remove "void" from zero argument functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49937/diff/2-3/
Comment on attachment 8747574 [details]
MozReview Request: Bug 1267524 Part 19 - Move nsStyleCoord members to initializer list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49939/diff/2-3/
Comment on attachment 8745210 [details]
MozReview Request: Bug 1267524 Part 8 - Use member initializer list for nsStyleTableBorder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48917/diff/4-5/
Comment on attachment 8745211 [details]
MozReview Request: Bug 1267524 Part 9 - Use member initializer list for nsStyleColor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48919/diff/4-5/
Comment on attachment 8745212 [details]
MozReview Request: Bug 1267524 Part 10 - Use member initializer list for nsStyleDisplay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48921/diff/4-5/
Comment on attachment 8745213 [details]
MozReview Request: Bug 1267524 Part 11 - Use member initializer list for nsStyleVisibility.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48923/diff/4-5/
Comment on attachment 8745214 [details]
MozReview Request: Bug 1267524 Part 12 - Use member initializer list for nsStyleContent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48925/diff/4-5/
Comment on attachment 8745215 [details]
MozReview Request: Bug 1267524 Part 13 - Use member initializer list for nsStyleTextReset.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48927/diff/4-5/
Comment on attachment 8745216 [details]
MozReview Request: Bug 1267524 Part 14 - Use member initializer list for nsStyleText.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48929/diff/4-5/
Comment on attachment 8745217 [details]
MozReview Request: Bug 1267524 Part 15 - Use member initializer list for nsStyleUserInterface.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48931/diff/4-5/
Comment on attachment 8745218 [details]
MozReview Request: Bug 1267524 Part 16 - Use member initializer list for nsStyleUIReset.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48933/diff/4-5/
Comment on attachment 8745219 [details]
MozReview Request: Bug 1267524 Part 17 - Use member initializer list for nsStyleVariables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48935/diff/4-5/
Comment on attachment 8747573 [details]
MozReview Request: Bug 1267524 Part 18 - Remove "void" from zero argument functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49937/diff/3-4/
Comment on attachment 8747574 [details]
MozReview Request: Bug 1267524 Part 19 - Move nsStyleCoord members to initializer list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49939/diff/3-4/
Class member initializer lists are not supported by MSVC 2013, which we still support. This should be backed out.
Mike, many structs in nsStyleStruct.cpp already use initializer list in the constructor like [1]. This is the C++98 syntax. The only C++11 syntax I used is the list-initialization for an array in the constructor [2]. However, "Initializer lists" is supported by visual studio 2013 according to [3]. Could you elaborate a bit more? Thanks.

[1] https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/layout/style/nsStyleStruct.cpp#2838
[2] https://hg.mozilla.org/integration/mozilla-inbound/rev/bae0edbcd259#l1.164
[3] https://msdn.microsoft.com/en-US/library/hh567368.aspx
Flags: needinfo?(mh+mozilla)
If the list-initialization for an array in Part 19 is causing trouble for msvc 2013, I could do a follow-up to remove the syntax from Part 19.
I got this error building with 2013: https://msdn.microsoft.com/en-us/library/9f53ks1w.aspx
Flags: needinfo?(mh+mozilla)
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #125)
> If the list-initialization for an array in Part 19 is causing trouble for
> msvc 2013, I could do a follow-up to remove the syntax from Part 19.

Yes, please, because it did break it, and I imagine I am not the only person currently blocked with a non-functioning build.

Is there a follow up bug for this?  This is a large patch, it would be difficult to back out, so please get a fix in another bug.  Since we knew about this problem, we shouldn't have landed it in the first place, but it is too late for that.
Flags: needinfo?(tlin)
Flags: needinfo?(cam)
The error in question (there may be more once these are cleared):

 0:33.57 c:/Users/msreckovic/Repos/mozilla-central/layout/style/nsStyleStruct.cpp(2909) : error C2536: 'nsStyleDisplay::nsStyleDisplay::mTransformOrigin' : cannot specify explicit initializer for arrays
 0:33.57 c:\users\msreckovic\repos\mozilla-central\layout\style\nsStyleStruct.h(2578) : see declaration of 'nsStyleDisplay::mTransformOrigin'
 0:33.57 c:/Users/msreckovic/Repos/mozilla-central/layout/style/nsStyleStruct.cpp(2909) : error C2536: 'nsStyleDisplay::nsStyleDisplay::mPerspectiveOrigin' : cannot specify explicit initializer for arrays
 0:33.57         c:\users\msreckovic\repos\mozilla-central\layout\style\nsStyleStruct.h(2580) : see declaration of 'nsStyleDisplay::mPerspectiveOrigin'
 0:33.57 c:/Users/msreckovic/Repos/mozilla-central/layout/style/nsStyleStruct.cpp(2974) : error C2536: 'nsStyleDisplay::nsStyleDisplay::mTransformOrigin' : cannot specify explicit initializer for arrays
 0:33.57 c:\users\msreckovic\repos\mozilla-central\layout\style\nsStyleStruct.h(2578) : see declaration of 'nsStyleDisplay::mTransformOrigin'
 0:33.57 c:/Users/msreckovic/Repos/mozilla-central/layout/style/nsStyleStruct.cpp(2974) : error C2536: 'nsStyleDisplay::nsStyleDisplay::mPerspectiveOrigin' : cannot specify explicit initializer for arrays
 0:33.57 c:\users\msreckovic\repos\mozilla-central\layout\style\nsStyleStruct.h(2580) : see declaration of 'nsStyleDisplay::mPerspectiveOrigin'
(In reply to Milan Sreckovic [:milan] from comment #129)
> (In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #125)
> > If the list-initialization for an array in Part 19 is causing trouble for
> > msvc 2013, I could do a follow-up to remove the syntax from Part 19.
> 
> Yes, please, because it did break it, and I imagine I am not the only person
> currently blocked with a non-functioning build.
> 
> Is there a follow up bug for this?  This is a large patch, it would be
> difficult to back out, so please get a fix in another bug.  Since we knew
> about this problem, we shouldn't have landed it in the first place, but it
> is too late for that.

Looks like a fix was already pushed to inbound (see comment 127).
Could you rebase to inbound and check again?
Flags: needinfo?(tlin)
Flags: needinfo?(milan)
Flags: needinfo?(cam)
Sorry for breaking msvc 2013. The fix is landed on m-c now.
Randomly, I still can't build, but it's probably a different bug.  Don't have time to chase it down, but it's in nsStyleStruct.h.  Now that we're moving back to VC 2013 for the official builds, I imagine this will get sorted out quickly :)
Flags: needinfo?(milan)
Blocks: 1309813
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: