1244049 - Use scoped enum for nsCSSPseudoElement::Type

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Chiou [:boris]]

According to Bug 1174575 Comment 31 & Bug 1174575 Comment 32, many functions use "enum nsCSSPseudoElements::Type"[1] as an argument, and it's better to change its type to "scoped enum" and rename it, so we can get the benefits from scope enum (e.g. forward declaration this type).

[1] https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSPseudoElements.h#56

Comment 1

[Boris Chiou [:boris]]

I think we could sill put this in nsCSSPseudoElements:
For example:

class nsCSSPseudoElement {
  ...
  enum class Type : uint8_t {
#define CSS_PSEUDO_ELEMENT(_name, _value_, _flags) \
    _name,
#include "nsCSSPseudoElementList.h"
#undef CSS_PSEUDO_ELEMENT
    Count,
    AnonBox = Count,
    XULTree,
    NotPseudo,
    MAX
  };
  ...
}

Just remove the prefix "ePseudo" and some redundant words, so the usage could be
1. nsCSSPseudoElements::Type
2. nsCSSPseudoElements::Type::Count     // the count of pseudo type
3. nsCSSPseudoElements::Type::NotPseudo // not a pseudo element
4. nsCSSPseudoElements::Type::before    // the :before pseudo

Another idea is to move this enum into CSSPseudoElement class (Bug 1174575), and might try to merge CSSPseudoElement and nsCSSPseudoElement (i.e. Move these static functions and #defines into CSSPseudoElement), so the coexistence of CSSPsuedoElement and nsCSSPseudoElemnts would not confuse other developers. However, I'm not sure whether it is a good idea or not.

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

AFAIK, you cannot forward declare a scoped enum if it's inside a class C without including the header which defines C. I might define "enum class CSSPseudoElementType : uint8_t" under mozilla::dom namespace, and remove the prefix ePseudo_ like comment #1.

Comment 3

[Boris Chiou [:boris]]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #2)
> AFAIK, you cannot forward declare a scoped enum if it's inside a class C
> without including the header which defines C. I might define "enum class
> CSSPseudoElementType : uint8_t" under mozilla::dom namespace, and remove the
> prefix ePseudo_ like comment #1.

Thanks for addressing this point, TY. I should move it out of the class.

For example:
namespace mozilla {
namespace dom{
enum class CSSPseudoElementType : uint8_t {
  ...
}
}
}

I'd like to remove the "ns" prefix of this enum, as you mentioned.

Comment 4

[Boris Chiou [:boris]]

Attachment: Part 1: Define scoped enum for CSSPseudoElement type

Comment 5

[Boris Chiou [:boris]]

Attachment: Part 2: Replace nsCSSPseudoElements::Type with CSSPseudoElementType

Also, try to use forward declaraions for CSSPseudoElementType;

Comment 6

[Boris Chiou [:boris]]

Comment on attachment 8719710 [details] [diff] [review]
Part 1: Define scoped enum for CSSPseudoElement type

Review of attachment 8719710 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSPseudoElements.h
@@ +38,5 @@
> +namespace mozilla {
> +
> +// The total count of CSSPseudoElement is less than 256,
> +// so use uint8_t as its underlying type.
> +enum class CSSPseudoElementType : uint8_t {

The definition of mozilla::CSSPseudoElementType.

Comment 7

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=366afff27a60

Comment 8

[Boris Chiou [:boris]]

Attachment: Part 2: Replace nsCSSPseudoElements::Type with CSSPseudoElementType (v2)

Also, try to use forward declaraions for CSSPseudoElementType;

Fix errors on debug build.

Comment 9

[Boris Chiou [:boris]]

Attachment: Part 1: Define scoped enum for CSSPseudoElement type (v2)

Fix errors on debug build.

Comment 10

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e8cf03ccb71

Comment 11

[Boris Chiou [:boris]]

Comment on attachment 8719729 [details] [diff] [review]
Part 1: Define scoped enum for CSSPseudoElement type (v2)

Review of attachment 8719729 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSPseudoElements.h
@@ +38,5 @@
> +namespace mozilla {
> +
> +// The total count of CSSPseudoElement is less than 256,
> +// so use uint8_t as its underlying type.
> +enum class CSSPseudoElementType : uint8_t {

Hi David,
This is the definition of mozilla::CSSPseudoElementType.

Comment 12

[Boris Chiou [:boris]]

Attachment: Part 1: Define scoped enum for CSSPseudoElement type (v3)

Comment 13

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=051f946d8737

Comment 14

[Boris Chiou [:boris]]

Sorry. There are some build errors on debug mode, so I should update the patches soon.

Comment 15

[Boris Chiou [:boris]]

Attachment: Part 1: Define scoped enum for CSSPseudoElement type (v4)

Updated: fix build errors on debug mode.

Comment 16

[Boris Chiou [:boris]]

Attachment: Part 2: Replace nsCSSPseudoElements::Type with CSSPseudoElementType (v3)

Also, try to use forward declaraions for CSSPseudoElementType;

Rebased.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8720149 [details] [diff] [review]
Part 1: Define scoped enum for CSSPseudoElement type (v4)

Given the number of places that you have to write uint8_t out, I think
it would be better to:

typedef uint8_t CSSPseudoElementTypeBase;

right above the definition of CSSPseudoElementType, and then use
CSSPseudoElementTypeBase everywhere you currently write uint8_t
*or* (in StyleRule) int16_t.

r=dbaron with that


You should also fix (in a followup patch) nsCSSSelector::mPseudoType
to be this new type instead of int16_t.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8720150 [details] [diff] [review]
Part 2: Replace nsCSSPseudoElements::Type with CSSPseudoElementType (v3)

I'm curious why you're preferring using over typedef; existing usage
prefers typedef, and I'd rather keep things consistent unless there's
a good reason.  (This applies to the previous patch as well, actually.)

And in general, I'd have expected you to propose changes of that form
in a separate patch rather than stuck within a patch to do something 
else.  (Also, in general, the bigger the patch, the more wary you
should be of mixing other things with it.)

Please undo all of those changes, and use typedef for the new things 
too.  (Or is there a good reason not to?)

>+enum class CSSPseudoElementType: uint8_t;

In general, you should have a space before the :.  (Every time!)

I guess I'm ok with this pattern, since the compiler should give
an error if this doesn't match the definition of the type, and you
don't need to apply the Base advice from the previous patch to this,
but you should still do that for the previous patch.

nsCSSPseudoElements.h:

>-  static bool PseudoElementContainsElements(const Type aType) {
>+  static bool PseudoElementContainsElements(const Type aType)
>+  {

These should stay as they are.  I believe the general rule is that we
put the opening brace on its own line if, when on its own line, it
would not have any indentation before it.

(If that's wrong, we can deal with it when we clang-format the whole
tree.)

r=dbaron with that

Comment 19

[Boris Chiou [:boris]]

(In reply to David Baron [:dbaron] ⌚️UTC-8 from comment #18)
> Please undo all of those changes, and use typedef for the new things 
> too.  (Or is there a good reason not to?)
No good reason. It's just my preference to use "using". I will revert them in another patch. Thanks.

> 
> >+enum class CSSPseudoElementType: uint8_t;
> 
> In general, you should have a space before the :.  (Every time!)
> 
> I guess I'm ok with this pattern, since the compiler should give
> an error if this doesn't match the definition of the type, and you
> don't need to apply the Base advice from the previous patch to this,
> but you should still do that for the previous patch.

OK. I think use CSSPseudoElementTypeBase is OK to me because it's much easier to change the underlying type if we have more pseudo types in the feature, so I will still follow the previous comment to use "typedef CSSPseudoElementTypeBase". Thanks.

Comment 20

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris]  from comment #19)
> OK. I think use CSSPseudoElementTypeBase is OK to me because it's much
> easier to change the underlying type if we have more pseudo types in the
> feature, so I will still follow the previous comment to use "typedef
> CSSPseudoElementTypeBase". Thanks.

Sorry. If I use "typedef CSSPseudoElementType", I have to include nsCSSPseudoElements.h in each header file, so cannot do forward declarations for CSSPseudoElementType. Therefore, I think using uint8_t directly has this benefit, and I'd like to keep using uint8_t but add a space before :.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Boris Chiou [:boris]  from comment #19)
> (In reply to David Baron [:dbaron] ⌚️UTC-8 from comment #18)
> > Please undo all of those changes, and use typedef for the new things 
> > too.  (Or is there a good reason not to?)
> No good reason. It's just my preference to use "using". I will revert them
> in another patch. Thanks.

No, that's not acceptable, since it breaks "hg blame", etc.  You should not make the changes in this patch.

(In reply to Boris Chiou [:boris]  from comment #20)
> Sorry. If I use "typedef CSSPseudoElementType", I have to include
> nsCSSPseudoElements.h in each header file, so cannot do forward declarations
> for CSSPseudoElementType. Therefore, I think using uint8_t directly has this
> benefit, and I'd like to keep using uint8_t but add a space before :.

Agreed, the reduction in #include dependencies was why I suggested that using uint8_t directly in the forward declarations was acceptable.

Comment 22

[Boris Chiou [:boris]]

Attachment: Part 2: Replace nsCSSPseudoElements::Type with CSSPseudoElementType. (v4, carry dbaron's r+)

Also, try to use forward declaraions for CSSPseudoElementType;

Updated:
1. Use typedef to keep things consistent
2. Add a space before : when doing forward declarations to CSSPseudoElementType.

Comment 23

[Boris Chiou [:boris]]

Attachment: Part 3: Replace the type of nsCSSSelector::mPseudoType

Comment 24

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc06cebf2a35

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a020e2e4ace
https://hg.mozilla.org/integration/mozilla-inbound/rev/248497c0c18d
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f67b24cbb75

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, I still wanted you to do the CSSPseudoElementTypeBase thing in patch 1 rather than scatter uint8_t in various places all over the tree.  (It's possible we might need to change it to uint16_t at some point...)

Comment 27

[Boris Chiou [:boris]]

(In reply to David Baron [:dbaron] ⌚️UTC-8 from comment #26)
> Er, I still wanted you to do the CSSPseudoElementTypeBase thing in patch 1
> rather than scatter uint8_t in various places all over the tree.  (It's
> possible we might need to change it to uint16_t at some point...)

Comment 28

[Boris Chiou [:boris]]

(In reply to David Baron [:dbaron] ⌚️UTC-8 from comment #26)
> Er, I still wanted you to do the CSSPseudoElementTypeBase thing in patch 1
> rather than scatter uint8_t in various places all over the tree.  (It's
> possible we might need to change it to uint16_t at some point...)

Sorry, from comment 21, I thought it's acceptable to use uint8_t directly. Could I upload a new patch to use CSSPseudoElementTypeBase, instead of using uint8_t directly? Or I will ask Sheriff to back out these patches and re-review them.

Comment 29

[Boris Chiou [:boris]]

Or you mean I still have to define CSSPseudoElementTypeBase for static_cast<...> in some cpps for patch 1?

Comment 30

[Boris Chiou [:boris]]

(In reply to David Baron [:dbaron] ⌚️UTC-8 from comment #26)
> Er, I still wanted you to do the CSSPseudoElementTypeBase thing in patch 1
> rather than scatter uint8_t in various places all over the tree.  (It's
> possible we might need to change it to uint16_t at some point...)

Sorry, I misunderstood this with forward declarations. These patches are already in mozilla-inbound, and if it's ok to upload a new patch to fix this, I will upload it soon, or we can back out these patches.

Comment 31

[Boris Chiou [:boris]]

Attachment: Part 4: Define CSSPseudoElementTypeBase

In order to convert CSSPseudoElementType into its underlying type easier,
we define CSSPseudoElementTypeBase. However, keep using uint8_t directly for
forward declarations.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8721134 [details] [diff] [review]
Part 4: Define CSSPseudoElementTypeBase

Thanks.  (No need to back out the earlier patches.)

Comment 33

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=28e11bde50df

Comment 34

[Boris Chiou [:boris]]

Hi Sheriff,

Could you please check in 'part 4'? I believe 'part 1 ~ 3' are already in mozilla-inbound. Thank you.

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24706cc31630

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a020e2e4ace
https://hg.mozilla.org/mozilla-central/rev/248497c0c18d
https://hg.mozilla.org/mozilla-central/rev/1f67b24cbb75

Comment 37

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/24706cc31630