Closed Bug 1287728 Opened 6 years ago Closed 6 years ago

convert RestyleResult to enum class

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

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.
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 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 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+
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.
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. :)
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/
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
https://hg.mozilla.org/mozilla-central/rev/fe8f7126d821
https://hg.mozilla.org/mozilla-central/rev/5001c86e2f20
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.