1287728 - convert RestyleResult to enum class

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jeremy Chen ([e-mail])]

Using enum classes gets us a bit more safety, since the compiler actually does type checking for us.

Comment 1

[Jeremy Chen ([e-mail])]

Attachment: Bug 1287728 - part1: explicitly define default value for RestyleResult.

Review commit: https://reviewboard.mozilla.org/r/65190/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65190/

Comment 2

[Jeremy Chen ([e-mail])]

Attachment: Bug 1287728 - part2: convert RestyleResult to enum class.

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

[Cameron McCormack (:heycam)]

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.

Comment 4

[Cameron McCormack (:heycam)]

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.

Comment 5

[Jeremy Chen ([e-mail])]

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.

Comment 6

[Jeremy Chen ([e-mail])]

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 7

[Jeremy Chen ([e-mail])]

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 8

[Jeremy Chen ([e-mail])]

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/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fe8f7126d821
https://hg.mozilla.org/mozilla-central/rev/5001c86e2f20