Closed Bug 1477533 Opened 5 years ago Closed 5 years ago

Convert NS_STYLE_COLUMN_FILL_* and NS_STYLE_COLUMN_SPAN_* to enum classes

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

Details

Attachments

(3 files)

Bug 1477533 - Use static const class variable to represent column-count: auto.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1477533 - Convert NS_STYLE_COLUMN_FILL_* to an enum class.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1477533 - Convert NS_STYLE_COLUMN_SPAN_* to an enum class.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
Also, convert NS_STYLE_COLUMN_COUNT_AUTO to a static const class variable.
Comment on attachment 8993989 [details]
Bug 1477533 - Use static const class variable to represent column-count: auto.

https://reviewboard.mozilla.org/r/258596/#review265578

r=me although I wonder whether we should avoid making individual style struct members be initialized in their declaration rather than the constructor, since if we are not consistent about where we do this, then it makes it a little harder to understand what the constructor is doing.  But it's not a big deal.
Attachment #8993989 - Flags: review?(cam) → review+
The style guide https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices says "For constructors, initialize member variables with : nsFoo aFoo(bFoo) syntax." so I think that means we should avoid initializing in the declaration.
Comment on attachment 8993990 [details]
Bug 1477533 - Convert NS_STYLE_COLUMN_FILL_* to an enum class.

https://reviewboard.mozilla.org/r/258598/#review265580
Attachment #8993990 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #6)
> The style guide
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Coding_Style#CC_practices says "For constructors, initialize member
> variables with : nsFoo aFoo(bFoo) syntax." so I think that means we should
> avoid initializing in the declaration.

I'm moderately sure that predates the ability to initialize in the declaration, and that it's done to avoid constructors like:


Foo::Foo()
{
  mBar = 3;
  mBaz = 5;
}

Maybe worth asking for a clarification to the list or something?
Comment on attachment 8993991 [details]
Bug 1477533 - Convert NS_STYLE_COLUMN_SPAN_* to an enum class.

https://reviewboard.mozilla.org/r/258600/#review265582
Attachment #8993991 - Flags: review?(cam) → review+
The rule mentioned in comment 6 / comment 8 was added relatively recently (a couple years ago) in:

  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style$revision/1073782
  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style$compare?locale=en-US&to=1073782&from=1060866

Not sure whether that accounted for initialization in the declaration, I don't see any related thread anywhere.
We should be able to use all the features in C++11 per https://developer.mozilla.org/en-US/docs/Mozilla/Using_CXX_in_Mozilla_code.  In this case, it should be the "member initializers" feature.

Though initializing in the declaration is not very popular, we do have a few usages in nsStyleStruct.h such as  https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/layout/style/nsStyleStruct.h#2047-2048

IMHO, I like this feature because if a class has multiple constructors, we don't need to initialize a member in all the constructors' initializer list unless some constructor wants to assign different initial value.
I think https://developer.mozilla.org/en-US/docs/Mozilla/Using_CXX_in_Mozilla_code is really about whether the compilers and platforms we build on support various C++ features, and still we must use judgement as to the clarity of the code.  (For example, "auto" is allowed per that page, but most of the time it's clearer to write out the type.)

Though Emilio is probably right about the rule I quoted being about avoiding `mFoo = blah`, rather than avoiding initializers on the declarations.

I do think it looks a bit funny if one or two member variables are initialized like this, while the rest aren't, but I don't feel strongly about it so feel free to land the patches as is.
Right, "auto" is a good example which should only be used under certain circumstances. And thanks for the clarification about your preference. I'm going to land the patch as is.
Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/dea3f75dbe56
Use static const class variable to represent column-count: auto. r=heycam
https://hg.mozilla.org/integration/autoland/rev/799227e35bc5
Convert NS_STYLE_COLUMN_FILL_* to an enum class. r=heycam
https://hg.mozilla.org/integration/autoland/rev/428ca334e24f
Convert NS_STYLE_COLUMN_SPAN_* to an enum class. r=heycam
https://hg.mozilla.org/mozilla-central/rev/dea3f75dbe56
https://hg.mozilla.org/mozilla-central/rev/799227e35bc5
https://hg.mozilla.org/mozilla-central/rev/428ca334e24f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.