Closed
Bug 1287728
Opened 7 years ago
Closed 7 years ago
convert RestyleResult to enum class
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: chenpighead, Assigned: chenpighead)
Details
Attachments
(2 files)
Using enum classes gets us a bit more safety, since the compiler actually does type checking for us.
Assignee | ||
Comment 1•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/65190/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/65190/
Attachment #8772356 -
Flags: review?(cam)
Attachment #8772357 -
Flags: review?(cam)
Assignee | ||
Comment 2•7 years ago
|
||
Since a higher RestyleResult value represent a superset of the work done by a lower value, explicitly assign values to all members of RestyleResult. Then, convert RestyleResult to enum class and Fix all the uses accordingly. Review commit: https://reviewboard.mozilla.org/r/65192/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/65192/
Comment 3•7 years ago
|
||
Comment on attachment 8772356 [details] Bug 1287728 - part1: explicitly define default value for RestyleResult. https://reviewboard.mozilla.org/r/65190/#review62210 r=me with these addressed. ::: layout/base/RestyleManager.h:675 (Diff revision 1) > // > // These values must be ordered so that later values imply that all > // the work of the earlier values is also done. > enum RestyleResult { > + // default initial value > + eRestyleResult_None = 0, Now that we have values starting from 0, we can drop the explicity numbering. (There's nothing that relies on the specific numbers, just that they are ordered in a particular way.) ::: layout/base/RestyleManager.cpp:5197 (Diff revision 1) > break; > case eRestyleResult_ContinueAndForceDescendants: > result.AssignLiteral("eRestyleResult_ContinueAndForceDescendants"); > break; > default: > - result.AppendPrintf("RestyleResult(%d)", aRestyleResult); > + result.AssignLiteral("eRestyleResult_None"); Let's assert in here that the value is indeed eRestyleResult_None.
Attachment #8772356 -
Flags: review?(cam) → review+
Comment 4•7 years ago
|
||
Comment on attachment 8772357 [details] Bug 1287728 - part2: convert RestyleResult to enum class. https://reviewboard.mozilla.org/r/65192/#review62206 ::: layout/base/RestyleManager.h:675 (Diff revision 1) > // > // These values must be ordered so that later values imply that all > // the work of the earlier values is also done. > - enum RestyleResult { > + enum class RestyleResult : uint8_t { > // default initial value > - eRestyleResult_None = 0, > + eNone = 0, I think we don't need the explicit numbering.
Attachment #8772357 -
Flags: review?(cam) → review+
Assignee | ||
Comment 5•7 years ago
|
||
https://reviewboard.mozilla.org/r/65190/#review62210 > Now that we have values starting from 0, we can drop the explicity numbering. (There's nothing that relies on the specific numbers, just that they are ordered in a particular way.) You're right. Will do. > Let's assert in here that the value is indeed eRestyleResult_None. Good idea. Will do.
Assignee | ||
Comment 6•7 years ago
|
||
https://reviewboard.mozilla.org/r/65192/#review62206 > I think we don't need the explicit numbering. Will drop the explicit numbering. Thank you for the review. :)
Assignee | ||
Comment 7•7 years ago
|
||
Comment on attachment 8772356 [details] Bug 1287728 - part1: explicitly define default value for RestyleResult. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65190/diff/1-2/
Assignee | ||
Comment 8•7 years ago
|
||
Comment on attachment 8772357 [details] Bug 1287728 - part2: convert RestyleResult to enum class. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65192/diff/1-2/
Pushed by jichen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fe8f7126d821 part1: explicitly define default value for RestyleResult. r=heycam https://hg.mozilla.org/integration/autoland/rev/5001c86e2f20 part2: convert RestyleResult to enum class. r=heycam
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fe8f7126d821 https://hg.mozilla.org/mozilla-central/rev/5001c86e2f20
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•