Use scoped enum for nsCSSPseudoElement::Type

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: boris, Assigned: boris)

Tracking

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: dom-triaged)

Attachments

(4 attachments, 6 obsolete attachments)

122.09 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
165.61 KB, patch
boris
: review+
Details | Diff | Splinter Review
3.86 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
8.54 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
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
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.
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.
(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.
Whiteboard: dom-triaged
Assignee: nobody → boris.chiou
Blocks: 1234403
Status: NEW → ASSIGNED
Attachment #8719711 - Flags: review?(dbaron)
Attachment #8719711 - Attachment is obsolete: true
Attachment #8719711 - Flags: review?(dbaron)
Attachment #8719710 - Attachment is obsolete: true
Attachment #8719710 - Flags: review?(dbaron)
Attachment #8719729 - Attachment is obsolete: true
Attachment #8719729 - Flags: review?(dbaron)
Sorry. There are some build errors on debug mode, so I should update the patches soon.
Updated: fix build errors on debug mode.
Attachment #8720149 - Flags: review?(dbaron)
Attachment #8720087 - Attachment is obsolete: true
Attachment #8720087 - Flags: review?(dbaron)
Also, try to use forward declaraions for CSSPseudoElementType;

Rebased.
Attachment #8720150 - Flags: review?(dbaron)
Attachment #8719727 - Attachment is obsolete: true
Attachment #8719727 - Flags: review?(dbaron)
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.
Attachment #8720149 - Flags: review?(dbaron) → review+
Component: DOM → CSS Parsing and Computation
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
Attachment #8720150 - Flags: review?(dbaron) → review+
(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.
(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 :.
(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.
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.
Attachment #8720649 - Flags: review+
Attachment #8720150 - Attachment is obsolete: true
Attachment #8720654 - Flags: review?(dbaron)
Attachment #8720654 - Flags: review?(dbaron) → review+
Keywords: checkin-needed
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...)
Flags: needinfo?(boris.chiou)
(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.
In order to convert CSSPseudoElementType into its underlying type easier,
we define CSSPseudoElementTypeBase. However, keep using uint8_t directly for
forward declarations.
Attachment #8721134 - Flags: review?(dbaron)
Comment on attachment 8721134 [details] [diff] [review]
Part 4: Define CSSPseudoElementTypeBase

Thanks.  (No need to back out the earlier patches.)
Attachment #8721134 - Flags: review?(dbaron) → review+
Hi Sheriff,

Could you please check in 'part 4'? I believe 'part 1 ~ 3' are already in mozilla-inbound. Thank you.
https://hg.mozilla.org/mozilla-central/rev/4a020e2e4ace
https://hg.mozilla.org/mozilla-central/rev/248497c0c18d
https://hg.mozilla.org/mozilla-central/rev/1f67b24cbb75
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
See Also: → 1250820
You need to log in before you can comment on or make changes to this bug.