Closed Bug 1297306 Opened 8 years ago Closed 8 years ago

Convert NS_STYLE_CLEAR_* to an enum class

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: chenpighead, Assigned: chenpighead)

References

Details

Attachments

(7 files)

StyleFloat could be a candidate for naming this enum class.
https://bugzilla.mozilla.org/show_bug.cgi?id=1277133#c10 has instructions if any newcomer wants to pick this up.
Mentor: manishearth
Keywords: good-first-bug
Whiteboard: [lang=c++]
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #0)
> StyleFloat could be a candidate for naming this enum class.

I mean StyleClear.... sry
(In reply to Manish Goregaokar [:manishearth] from comment #1)
> https://bugzilla.mozilla.org/show_bug.cgi?id=1277133#c10 has instructions if
> any newcomer wants to pick this up.

Nice, thank you for the pointers. :-)
Assignee: nobody → jeremychen
Status: NEW → ASSIGNED
Comment on attachment 8784260 [details]
Bug 1297306 - part1:remove unused NS_STYLE_CLEAR_* condition.

https://reviewboard.mozilla.org/r/73780/#review71650

Actually how can we guarantee the flag bits would all be zero...
Attachment #8784260 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8784261 [details]
Bug 1297306 - part6:replace NS_STYLE_CLEAR_* with StyleClear enum class.

https://reviewboard.mozilla.org/r/73782/#review71652

I would like to review again after you address the issues below.

::: dom/html/HTMLBRElement.cpp:33
(Diff revision 1)
> -  { "left", NS_STYLE_CLEAR_LEFT },
> -  { "right", NS_STYLE_CLEAR_RIGHT },
> -  { "all", NS_STYLE_CLEAR_BOTH },
> -  { "both", NS_STYLE_CLEAR_BOTH },
> +  { "left", static_cast<int>(StyleClear::Left) },
> +  { "right", static_cast<int>(StyleClear::Right) },
> +  { "all", static_cast<int>(StyleClear::Both) },
> +  { "both", static_cast<int>(StyleClear::Both) },

Could you add a separate patch which adds something like [1] to nsAttrValue::EnumTable, so that we can get rid of the casts here?

I guess that would need an additional patch to move IsEnumFittingWithin to mfbt first.

[1] https://dxr.mozilla.org/mozilla-central/rev/052656fc513c05da969590ac5934abd67271a897/layout/style/nsCSSProps.h#363-371

::: layout/generic/BlockReflowInput.cpp:1097
(Diff revision 1)
>    }
>  #endif
>  
>  #ifdef NOISY_FLOAT_CLEARING
>    printf("BlockReflowInput::ClearFloats: aBCoord=%d breakType=%d\n",
> -         aBCoord, aBreakType);
> +         aBCoord, static_cast<int>aBreakType);

Would be better if we output a string for different values. But given this just preserves the current behavior, keeping it as-is is also fine.

If you want to make it output a string, that's probably worth a separate patch as well.

::: layout/generic/nsBlockFrame.cpp:1926
(Diff revision 1)
> -           line->GetBreakTypeBefore(), line->GetBreakTypeAfter(),
> +           static_cast<int>(line->GetBreakTypeBefore()),
> +           static_cast<int>(line->GetBreakTypeAfter()),

Like above, it would be better if we can output a string for different values.

::: layout/generic/nsBlockFrame.cpp:2030
(Diff revision 1)
>    }
>  }
>  
>  static bool LineHasClear(nsLineBox* aLine) {
>    return aLine->IsBlock()
> -    ? (aLine->GetBreakTypeBefore() ||
> +    ? (static_cast<int>(aLine->GetBreakTypeBefore()) ||

It should be comparing with None_, not casting to int.

::: layout/generic/nsBlockFrame.cpp:4123
(Diff revision 1)
>    if (NS_INLINE_IS_BREAK(frameReflowStatus) || 
> -      (NS_STYLE_CLEAR_NONE != aState.mFloatBreakType)) {
> +      (StyleClear::None_ != aState.mFloatBreakType)) {

As far as you are here, could you remove the tailing whitespace in the line above?

Also the inner parens here are not necessary, please remove them.

::: layout/generic/nsLineBox.h
(Diff revision 1)
>    bool HasBreakAfter() const {
> -    return !IsBlock() && NS_STYLE_CLEAR_NONE != mFlags.mBreakType;
> +    return !IsBlock() && StyleClear::None_ != mFlags.mBreakType;
>    }
> -  void SetBreakTypeAfter(uint8_t aBreakType) {
> +  void SetBreakTypeAfter(StyleClear aBreakType) {
>      NS_ASSERTION(!IsBlock(), "Only inlines have break-after");
> -    NS_ASSERTION(aBreakType <= LINE_MAX_BREAK_TYPE, "bad break type");

Why removing this assertion? It should be "aBreakType < StyleClear::Line", I suppose?

::: layout/generic/nsLineBox.h:675
(Diff revision 1)
>      uint32_t mHasBullet : 1;
>      // Indicates that this line *may* have a placeholder for a float
>      // that was pushed to a later column or page.
>      uint32_t mHadFloatPushed : 1;
>      uint32_t mHasHashedFrames: 1;
> -    uint32_t mBreakType : 4;
> +    StyleClear mBreakType : 4;

You should not change this unless you change the type of all of other bits to bool.

On MSVC, since storage type of StyleClear is uint8_t, different from uint32_t, this field would be put at the fifth byte rather than being merged with all bits above. And even worse, the struct would then cost 8 bytes because the compiler would generate padding after that because there is a uint32_t.

We probably need a static_assert near the union below to assert that sizeof FlagBits is not larger than sizeof uint32_t.

::: layout/generic/nsLineBox.cpp
(Diff revision 1)
> -  static_assert(NS_STYLE_CLEAR_MAX <= 15,
> -                "FlagBits needs more bits to store the full range of "
> -                "break type ('clear') values");

You probably should not remove this assertion, unless you can prove it is no necessary (which means if you change the bits StyleClear needs, build would fail).

It seems to me neither MSVC nor Clang warns that, and only GCC 4.9+ does that for enum class. However, it is not clear to me whether we are running GCC 4.9+ on our infra.

Also, if you can remove this assertion, it probably doesn't make any sense to keep StyleClear::Max at all.

::: layout/style/nsRuleNode.cpp:1339
(Diff revision 1)
>      aField = static_cast<type_>(value); \
>    }
>  
>    DEFINE_ENUM_CLASS_SETTER(StyleBoxSizing, Content, Border)
>    DEFINE_ENUM_CLASS_SETTER(StyleFillRule, Nonzero, Evenodd)
> +  DEFINE_ENUM_CLASS_SETTER(StyleClear, None_, Max)

It seems this list is in alphabetic order. We shouldn't break the order without good reason. Please put StyleClear before StyleFillRule.

Also, are Max or Line a possible value from style? I suppose it should just be None_ to Both.

::: layout/style/nsStyleConsts.h:77
(Diff revision 1)
>  enum class StyleBoxShadowType : uint8_t {
>    Inset,
>  };
>  
> +// clear
> +// https://developer.mozilla.org/en-US/docs/Web/CSS/clear

This is not necessary.

It should be a link to spec if we really want to put something here, but I don't think we need any.

::: layout/style/nsStyleConsts.h:78
(Diff revision 1)
>    Inset,
>  };
>  
> +// clear
> +// https://developer.mozilla.org/en-US/docs/Web/CSS/clear
> +enum class StyleClear : uint8_t {

And it seems you didn't remove the old constants?

::: layout/style/nsStyleConsts.h:85
(Diff revision 1)
> +  // StyleClear::Line can be added to one of the other values in layout
> +  // so it needs to use a bit value that none of the other values can have.

Based on the current code, it doesn't seem to me this is true... But it is fine to investigate this later.
Attachment #8784261 - Flags: review?(xidorn+moz)
Comment on attachment 8784262 [details]
Bug 1297306 - part7:replace StyleClear related NS_ASSERTION with MOZ_ASSERT.

https://reviewboard.mozilla.org/r/73784/#review71660

::: layout/generic/nsLineBox.h:403
(Diff revision 1)
>                   aBreakType == StyleClear::Left ||
>                   aBreakType == StyleClear::Right ||
>                   aBreakType == StyleClear::Both,
>                   "Only float break types are allowed before a line");

You should reindent these lines.
Attachment #8784262 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8784261 [details]
Bug 1297306 - part6:replace NS_STYLE_CLEAR_* with StyleClear enum class.

https://reviewboard.mozilla.org/r/73782/#review71652

> Would be better if we output a string for different values. But given this just preserves the current behavior, keeping it as-is is also fine.
> 
> If you want to make it output a string, that's probably worth a separate patch as well.

We have a ToString() function which converts a type into a string. All you need to do is implement 

std::ostream& operator<<(std::ostream& aStream, const StyleClear& aStyleClear)

so that ToString(aBreaType) will output a string.

http://searchfox.org/mozilla-central/rev/b38dbd1378cea4ae83bbc8a834cdccd02bbc5347/mfbt/ToString.h#23
Comment on attachment 8784261 [details]
Bug 1297306 - part6:replace NS_STYLE_CLEAR_* with StyleClear enum class.

https://reviewboard.mozilla.org/r/73782/#review71652

> Could you add a separate patch which adds something like [1] to nsAttrValue::EnumTable, so that we can get rid of the casts here?
> 
> I guess that would need an additional patch to move IsEnumFittingWithin to mfbt first.
> 
> [1] https://dxr.mozilla.org/mozilla-central/rev/052656fc513c05da969590ac5934abd67271a897/layout/style/nsCSSProps.h#363-371

Will do.

> We have a ToString() function which converts a type into a string. All you need to do is implement 
> 
> std::ostream& operator<<(std::ostream& aStream, const StyleClear& aStyleClear)
> 
> so that ToString(aBreaType) will output a string.
> 
> http://searchfox.org/mozilla-central/rev/b38dbd1378cea4ae83bbc8a834cdccd02bbc5347/mfbt/ToString.h#23

Xidorn, Ting-Yu, thank you for the review. I'm working on this. Will move this into a separate patch.

> It should be comparing with None_, not casting to int.

Will do.

> As far as you are here, could you remove the tailing whitespace in the line above?
> 
> Also the inner parens here are not necessary, please remove them.

Will do.

> Why removing this assertion? It should be "aBreakType < StyleClear::Line", I suppose?

This assertion looks like a safty check that we shall not use a undefined break type. However, after using enum class, I think we should get this covered already. No?

> You should not change this unless you change the type of all of other bits to bool.
> 
> On MSVC, since storage type of StyleClear is uint8_t, different from uint32_t, this field would be put at the fifth byte rather than being merged with all bits above. And even worse, the struct would then cost 8 bytes because the compiler would generate padding after that because there is a uint32_t.
> 
> We probably need a static_assert near the union below to assert that sizeof FlagBits is not larger than sizeof uint32_t.

Will undo this change and add a static_assert as you said in the next version. Thank you for the explanation.

> You probably should not remove this assertion, unless you can prove it is no necessary (which means if you change the bits StyleClear needs, build would fail).
> 
> It seems to me neither MSVC nor Clang warns that, and only GCC 4.9+ does that for enum class. However, it is not clear to me whether we are running GCC 4.9+ on our infra.
> 
> Also, if you can remove this assertion, it probably doesn't make any sense to keep StyleClear::Max at all.

You're right. Will undo this.

> It seems this list is in alphabetic order. We shouldn't break the order without good reason. Please put StyleClear before StyleFillRule.
> 
> Also, are Max or Line a possible value from style? I suppose it should just be None_ to Both.

You're right. Will fix this in the next version.

> This is not necessary.
> 
> It should be a link to spec if we really want to put something here, but I don't think we need any.

Okay.

> And it seems you didn't remove the old constants?

Yeah. Will do.
Comment on attachment 8785246 [details]
Bug 1297306 - part3:make BreakTypeToString() to be a member of nsLineBox.

https://reviewboard.mozilla.org/r/74522/#review72366

::: layout/generic/BlockReflowInput.cpp:1083
(Diff revision 1)
> +namespace mozilla {
> +
> +// Helper for print StyleClear as a string while debugging
> +#define PROCESS_ENUM_TO_STREAM(e) case(e): aStream << #e; break;
> +std::ostream&
> +operator<<(std::ostream& aStream, const StyleClear& aStyleClear)
> +{
> +  switch (aStyleClear) {
> +    PROCESS_ENUM_TO_STREAM(StyleClear::None_);
> +    PROCESS_ENUM_TO_STREAM(StyleClear::Left);
> +    PROCESS_ENUM_TO_STREAM(StyleClear::Right);
> +    PROCESS_ENUM_TO_STREAM(StyleClear::InlineStart);
> +    PROCESS_ENUM_TO_STREAM(StyleClear::InlineEnd);
> +    PROCESS_ENUM_TO_STREAM(StyleClear::Both);
> +    PROCESS_ENUM_TO_STREAM(StyleClear::Line);
> +    PROCESS_ENUM_TO_STREAM(StyleClear::Max);
> +    default:
> +      MOZ_ASSERT_UNREACHABLE("Unexpected StyleClear!");
> +      break;
> +  }
> +  return aStream;
> +}
> +#undef PROCESS_ENUM_TO_STREAM
> +
> +} // namespace mozilla

It's a bit too complicated. I think something like BreakTypeToString in nsLineBox.cpp would be enough. (Actually if you can reuse that directly somehow, that would probably be the best.)
Mentor: manishearth
Keywords: good-first-bug
Whiteboard: [lang=c++]
Attachment #8785244 - Flags: review?(xidorn+moz) → review?(nfroyd)
Comment on attachment 8785243 [details]
Bug 1297306 - part2:make the implementation of nsLineBox::LastChild() be behind DEBUG_FRAME_DUMP flag.

https://reviewboard.mozilla.org/r/74516/#review72642

I think you should make the declaration be wrapped in DEBUG rather than making the definition in DEBUG_FRAME_DUMP. There are several callers to this function in debug code (but not wrapped inside DEBUG_FRAME_DUMP): http://searchfox.org/mozilla-central/search?q=nsLineBox%3A%3ALastChild
Attachment #8785243 - Flags: review?(xidorn+moz)
Comment on attachment 8785245 [details]
Bug 1297306 - part5:create enum constructors for EnumTable.

https://reviewboard.mozilla.org/r/74520/#review72644

::: dom/base/nsAttrValue.h:293
(Diff revision 3)
> +    }
> +
>      /** The string the value maps to */
>      const char* tag;
>      /** The enum value that maps to this string */
> -    int16_t value;
> +    uint32_t value;

I don't really think this really needs to be uint32_t. It seems all uses fit in int16_t: you probably need to change some of their underlying type, though.

Actually, it seems to be more risky to change from a signed type to an unsigned type. Are you sure every possible value is nonnegative?

Also DOM part needs a DOM peer.
Attachment #8785245 - Flags: review?(amarchesini)
Comment on attachment 8784261 [details]
Bug 1297306 - part6:replace NS_STYLE_CLEAR_* with StyleClear enum class.

https://reviewboard.mozilla.org/r/73782/#review71652

> This assertion looks like a safty check that we shall not use a undefined break type. However, after using enum class, I think we should get this covered already. No?

OK.
Comment on attachment 8784261 [details]
Bug 1297306 - part6:replace NS_STYLE_CLEAR_* with StyleClear enum class.

https://reviewboard.mozilla.org/r/73782/#review72652

Looks good overall, with some nits below. I'd like to review the change to nsLineBox.h again.

::: layout/generic/nsBlockFrame.cpp:2030
(Diff revision 4)
>    }
>  }
>  
>  static bool LineHasClear(nsLineBox* aLine) {
>    return aLine->IsBlock()
> -    ? (aLine->GetBreakTypeBefore() ||
> +    ? ((aLine->GetBreakTypeBefore() != StyleClear::None_) ||

The additional parens are not necessary.

::: layout/generic/nsBlockFrame.cpp:4124
(Diff revision 4)
>    // break-after-not-complete. There are two situations: we are a
>    // block or we are an inline. This makes a total of 10 cases
>    // (fortunately, there is some overlap).
> -  aLine->SetBreakTypeAfter(NS_STYLE_CLEAR_NONE);
> -  if (NS_INLINE_IS_BREAK(frameReflowStatus) || 
> -      (NS_STYLE_CLEAR_NONE != aState.mFloatBreakType)) {
> +  aLine->SetBreakTypeAfter(StyleClear::None_);
> +  if (NS_INLINE_IS_BREAK(frameReflowStatus) ||
> +      (StyleClear::None_ != aState.mFloatBreakType)) {

The parens are not necessary, you can remove them.

::: layout/generic/nsBlockFrame.cpp:4131
(Diff revision 4)
> -    NS_ASSERTION((NS_STYLE_CLEAR_NONE != breakType) || 
> -                 (NS_STYLE_CLEAR_NONE != aState.mFloatBreakType), "bad break type");
> +    NS_ASSERTION((StyleClear::None_ != breakType) ||
> +                 (StyleClear::None_ != aState.mFloatBreakType), "bad break type");

ditto.

::: layout/generic/nsBlockFrame.cpp:4157
(Diff revision 4)
>        // If a float split and its prev-in-flow was followed by a <BR>, then combine 
>        // the <BR>'s break type with the inline's break type (the inline will be the very 

As far as you're here, could you also remove the tailing spaces on these lines?

::: layout/generic/nsLineBox.h:398
(Diff revision 4)
>    // Break information is applied *before* the line if the line is a block,
>    // or *after* the line if the line is an inline. Confusing, I know, but
>    // using different names should help.
> +  using StyleClear = mozilla::StyleClear;
>    bool HasBreakBefore() const {
> -    return IsBlock() && NS_STYLE_CLEAR_NONE != mFlags.mBreakType;
> +    return IsBlock() && static_cast<int>(StyleClear::None_) != mFlags.mBreakType;

Given that you repeat this static_cast<int>(StyleClear::*) many times below, I suggest you add a function in this class to get mFlags.mBreakType as StyleClear, e.g.
> StyleClear BreakType() const { return static_cast<StyleClear>(mFlags.mBreakType); }
so that you don't need to cast before comparing.

That function should probably be a protected one I guess.

::: layout/style/nsStyleStruct.h:2841
(Diff revision 4)
>    // [reset] See StyleFloat in nsStyleConsts.h.
>    mozilla::StyleFloat mFloat;
>    // [reset] Save mFloat for position:absolute/fixed; otherwise equal to mFloat.
>    mozilla::StyleFloat mOriginalFloat;
>  
> -  uint8_t mBreakType;           // [reset] see nsStyleConsts.h NS_STYLE_CLEAR_*
> +  // [reset] see StyleClear in nsStyleConsts.h

Let's remove the "see nsStyleConsts.h..." thing and keep the comment in the same line. Given it is now a enum class, I don't really think the additional comment is still necessary.
Attachment #8784261 - Flags: review?(xidorn+moz)
Comment on attachment 8785243 [details]
Bug 1297306 - part2:make the implementation of nsLineBox::LastChild() be behind DEBUG_FRAME_DUMP flag.

https://reviewboard.mozilla.org/r/74516/#review72662

Hmmm, looks like DEBUG always implies DEBUG_FRAME_DUMP. I guess this change is fine, then, although this function is not used in frame dump at all.
Attachment #8785243 - Flags: review+
Comment on attachment 8785246 [details]
Bug 1297306 - part3:make BreakTypeToString() to be a member of nsLineBox.

https://reviewboard.mozilla.org/r/74522/#review72666

::: layout/generic/nsLineBox.h:575
(Diff revision 3)
>                                      nsLineList_iterator& aEnd,
>                                      nsIFrame* aLastFrameBeforeEnd,
>                                      int32_t* aFrameIndexInLine);
>  
>  #ifdef DEBUG_FRAME_DUMP
> +  const char * BreakTypeToString(StyleClear aBreakType) const;

"const char*" (remove the whitespace before "*")

::: layout/generic/nsLineBox.cpp:195
(Diff revision 3)
>      fprintf_stderr(out, "%s\n", str.get());
>      fc = fc->Next();
>    }
>  }
>  
>  const char *

ditto.
Attachment #8785246 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8785245 [details]
Bug 1297306 - part5:create enum constructors for EnumTable.

Sad that it is still impossible to redirect review in MozReview :/
Attachment #8785245 - Flags: review?(xidorn+moz)
Comment on attachment 8785245 [details]
Bug 1297306 - part5:create enum constructors for EnumTable.

https://reviewboard.mozilla.org/r/74520/#review72644

> I don't really think this really needs to be uint32_t. It seems all uses fit in int16_t: you probably need to change some of their underlying type, though.
> 
> Actually, it seems to be more risky to change from a signed type to an unsigned type. Are you sure every possible value is nonnegative?

Yeah, the only reason is that ReferrerPolicy enum [0] is using nsIHttpChannel::REFERRER_POLICY_NO_REFERRER as its value where REFERRER_POLICY_NO_REFERRER is defined to be UINT32_MAX [1]. Maybe we should take this as a special case instead of using uint32_t for all the enum. Not sure if redefine REFERRER_POLICY_NO_REFERRER to be INT16_MAX could be another solution.

[0] http://searchfox.org/mozilla-central/rev/f80822840bc5f3d2d3cae3ece621ddbce72e7f54/netwerk/base/ReferrerPolicy.h#16
[1] http://searchfox.org/mozilla-central/rev/f80822840bc5f3d2d3cae3ece621ddbce72e7f54/netwerk/protocol/http/nsIHttpChannel.idl#67
Comment on attachment 8784261 [details]
Bug 1297306 - part6:replace NS_STYLE_CLEAR_* with StyleClear enum class.

https://reviewboard.mozilla.org/r/73782/#review72652

> Given that you repeat this static_cast<int>(StyleClear::*) many times below, I suggest you add a function in this class to get mFlags.mBreakType as StyleClear, e.g.
> > StyleClear BreakType() const { return static_cast<StyleClear>(mFlags.mBreakType); }
> so that you don't need to cast before comparing.
> 
> That function should probably be a protected one I guess.

Will do.
Comment on attachment 8785245 [details]
Bug 1297306 - part5:create enum constructors for EnumTable.

https://reviewboard.mozilla.org/r/74520/#review72644

> Yeah, the only reason is that ReferrerPolicy enum [0] is using nsIHttpChannel::REFERRER_POLICY_NO_REFERRER as its value where REFERRER_POLICY_NO_REFERRER is defined to be UINT32_MAX [1]. Maybe we should take this as a special case instead of using uint32_t for all the enum. Not sure if redefine REFERRER_POLICY_NO_REFERRER to be INT16_MAX could be another solution.
> 
> [0] http://searchfox.org/mozilla-central/rev/f80822840bc5f3d2d3cae3ece621ddbce72e7f54/netwerk/base/ReferrerPolicy.h#16
> [1] http://searchfox.org/mozilla-central/rev/f80822840bc5f3d2d3cae3ece621ddbce72e7f54/netwerk/protocol/http/nsIHttpChannel.idl#67

Also, if we decide to use int16_t, then we probably need to type every enum referenced by nsAttrValue::EnumTable with uint8_t. Because every untyped enum would be default typed with int (http://en.cppreference.com/w/cpp/language/enum), which is very likely to be int32_t and would never fit within int16_t.
Comment on attachment 8784261 [details]
Bug 1297306 - part6:replace NS_STYLE_CLEAR_* with StyleClear enum class.

https://reviewboard.mozilla.org/r/73782/#review72756

Looks good, thanks. r=me with nits below addressed.

::: layout/generic/nsLineBox.h:410
(Diff revisions 4 - 5)
> -    return IsBlock() ? static_cast<StyleClear>(mFlags.mBreakType)
> +    return IsBlock() ? BreakType()
>                       : StyleClear::None_;

These two lines can be merged.

::: layout/generic/nsLineBox.h:428
(Diff revisions 4 - 5)
> -    return !IsBlock() ? static_cast<StyleClear>(mFlags.mBreakType)
> +    return !IsBlock() ? BreakType()
>                        : StyleClear::None_;

ditto.
Attachment #8784261 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8784261 [details]
Bug 1297306 - part6:replace NS_STYLE_CLEAR_* with StyleClear enum class.

https://reviewboard.mozilla.org/r/73782/#review72756

> These two lines can be merged.

Will do. Thank you for the review. :-)
Comment on attachment 8785244 [details]
Bug 1297306 - part4:rename IsEnumFittingWithin with EnumTypeFitsWithin and move it to mfbt/EnumTypeTraits.h.

https://reviewboard.mozilla.org/r/74518/#review72834

I completely get the sentiment behind this, but I think if we're going to put this in MFBT, there are two things that we need:

1. We want code in MFBT to have tests.
2. Writing tests is not straightforward and suggests that this is not a great interface.  For instance, what does the following do:

```
enum default_enum {
  a,
  b,
};

static_assert(IsEnumFittingWithin<default_enum, int>::Value, "fail");
static_assert(!IsEnumFittingWithin<default_enum, unsigned int>::Value, "fail");
```

Simple enough, right?  This code fails on MSVC and GCC; the changes that make it work on one fail on the other.  I think there are problems both in the comparison being done (integer promotion) and with enums that aren't specified with a type.  Sadly, there's no way, AFAICT, to know whether an enum was specified with a type, or whether it's up to the implementation to choose a suitable base type.

I want to see this go in, but it needs to be more robust.
Attachment #8785244 - Flags: review?(nfroyd)
Comment on attachment 8785245 [details]
Bug 1297306 - part5:create enum constructors for EnumTable.

https://reviewboard.mozilla.org/r/74520/#review72846

::: dom/html/HTMLMenuItemElement.cpp:28
(Diff revision 4)
>  #define NS_ORIGINAL_CHECKED_VALUE (1 << 3)
>  #define NS_MENUITEM_TYPE(bits) ((bits) & ~( \
>    NS_CHECKED_IS_TOGGLED | NS_ORIGINAL_CHECKED_VALUE))
>  
> -enum CmdType                                                                 
> +enum CmdType : uint8_t
>  {                                                                            

remove this extra spaces.

::: dom/html/HTMLTrackElement.cpp:68
(Diff revision 4)
>  // Map html attribute string values to TextTrackKind enums.
>  static constexpr nsAttrValue::EnumTable kKindTable[] = {
> -  { "subtitles", static_cast<int16_t>(TextTrackKind::Subtitles) },
> -  { "captions", static_cast<int16_t>(TextTrackKind::Captions) },
> -  { "descriptions", static_cast<int16_t>(TextTrackKind::Descriptions) },
> -  { "chapters", static_cast<int16_t>(TextTrackKind::Chapters) },
> +  { "subtitles", (int16_t)TextTrackKind::Subtitles },
> +  { "captions", (int16_t)TextTrackKind::Captions },
> +  { "descriptions", (int16_t)TextTrackKind::Descriptions },
> +  { "chapters", (int16_t)TextTrackKind::Chapters },

why have you removed this static_cast?

::: dom/html/nsGenericHTMLElement.cpp:1197
(Diff revision 4)
>    static const nsAttrValue::EnumTable kReferrerTable[] = {
> -    { net::kRPS_No_Referrer, net::RP_No_Referrer },
> -    { net::kRPS_Origin, net::RP_Origin },
> -    { net::kRPS_Origin_When_Cross_Origin, net::RP_Origin_When_Crossorigin },
> -    { net::kRPS_No_Referrer_When_Downgrade, net::RP_No_Referrer_When_Downgrade },
> -    { net::kRPS_Unsafe_URL, net::RP_Unsafe_URL },
> +    { net::kRPS_No_Referrer, (int16_t)net::RP_No_Referrer },
> +    { net::kRPS_Origin, (int16_t)net::RP_Origin },
> +    { net::kRPS_Origin_When_Cross_Origin, (int16_t)net::RP_Origin_When_Crossorigin },
> +    { net::kRPS_No_Referrer_When_Downgrade, (int16_t)net::RP_No_Referrer_When_Downgrade },
> +    { net::kRPS_Unsafe_URL, (int16_t)net::RP_Unsafe_URL },

same here.
Attachment #8785245 - Flags: review?(amarchesini) → review+
Comment on attachment 8785245 [details]
Bug 1297306 - part5:create enum constructors for EnumTable.

https://reviewboard.mozilla.org/r/74520/#review72846

> remove this extra spaces.

Will do.

> why have you removed this static_cast?

I planned to remove this in a previous version, and then decided to add it here.
I guess I just forgot this is something already existed.
Sorry for the confusing. Will unchange this as what it was.
Thank you for the review.
Comment on attachment 8785244 [details]
Bug 1297306 - part4:rename IsEnumFittingWithin with EnumTypeFitsWithin and move it to mfbt/EnumTypeTraits.h.

Hi xidorn, please take a look at this change and give me some feedback. Thanks.
Attachment #8785244 - Flags: feedback?(xidorn+moz)
Comment on attachment 8785244 [details]
Bug 1297306 - part4:rename IsEnumFittingWithin with EnumTypeFitsWithin and move it to mfbt/EnumTypeTraits.h.

I don't think the added static_assert makes any sense.

I believe froydnj was asking you to add a new test inside mfbt/tests to ensure that this trait works as expected.

And we could add a comment before this trait states that it should not be used with an enum type without underlying type explicitly specified. (But we have no way to enforce this restriction in compile time.)
Attachment #8785244 - Flags: feedback?(xidorn+moz)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #65)
> Comment on attachment 8785244 [details]
> Bug 1297306 - part4:move IsEnumFittingWithin to mfbt/EnumTypeTraits.h.
> 
> I don't think the added static_assert makes any sense.
> 
> I believe froydnj was asking you to add a new test inside mfbt/tests to
> ensure that this trait works as expected.
> 
> And we could add a comment before this trait states that it should not be
> used with an enum type without underlying type explicitly specified. (But we
> have no way to enforce this restriction in compile time.)

Oh, I see. Will dive into mfbt/tests then.
Thank you for the explanation.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #65)
> I believe froydnj was asking you to add a new test inside mfbt/tests to
> ensure that this trait works as expected.

Several tests, but yes.

> And we could add a comment before this trait states that it should not be
> used with an enum type without underlying type explicitly specified. (But we
> have no way to enforce this restriction in compile time.)

That would be the minimum amount of documentation required, yes.  I'm still not sure this is the best trait to put in MFBT, but maybe with sufficient documentation it would be OK.
(In reply to Nathan Froyd [:froydnj] from comment #67)
> That would be the minimum amount of documentation required, yes.  I'm still
> not sure this is the best trait to put in MFBT, but maybe with sufficient
> documentation it would be OK.

I suppose if we want to use it in multiple components, MFBT is the best home for it. And it doesn't seem to me we can make it any better given the current language support. If one day we have more powerful trait, we can replace usage of this one with that.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=773ec013e0af&selectedJob=26907213

Looks like windows platforms (Windows 8 x64 and Windows XP) have problems on making unfixed enum with int64_t. :-/
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #76)
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=773ec013e0af&selectedJob=26907213
> 
> Looks like windows platforms (Windows 8 x64 and Windows XP) have problems on
> making unfixed enum with int64_t. :-/

For unfixed enum, it seems that promoting its underlying_type to a larger sized integral type can't be guaranteed. Maybe we should remove the tests for unfixed enum since they are implementation defined and have many uncertainties.
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #76)
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=773ec013e0af&selectedJob=26907213
> 
> Looks like windows platforms (Windows 8 x64 and Windows XP) have problems on
> making unfixed enum with int64_t. :-/

I think 64-bit unfixed enums (or even fixed ones) should be sufficiently rare that we could just:

static_assert(sizeof(EnumType) <= 4, "64-bit enums not currently supported");

in the type trait, and then if somebody (a) had 64-bit enums, and (b) wanted to use them with this trait, that person could spend the effort rewriting the trait to be more clever.
(In reply to Nathan Froyd [:froydnj] from comment #78)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #76)
> > try:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=773ec013e0af&selectedJob=26907213
> > 
> > Looks like windows platforms (Windows 8 x64 and Windows XP) have problems on
> > making unfixed enum with int64_t. :-/
> 
> I think 64-bit unfixed enums (or even fixed ones) should be sufficiently
> rare that we could just:
> 
> static_assert(sizeof(EnumType) <= 4, "64-bit enums not currently supported");
> 
> in the type trait, and then if somebody (a) had 64-bit enums, and (b) wanted
> to use them with this trait, that person could spend the effort rewriting
> the trait to be more clever.

In that case, I could remove the tests for 64-bit enums then. :-)
Comment on attachment 8785244 [details]
Bug 1297306 - part4:rename IsEnumFittingWithin with EnumTypeFitsWithin and move it to mfbt/EnumTypeTraits.h.

https://reviewboard.mozilla.org/r/74518/#review74852

Remove the tests for unfixed enum since they are implementation defined and have many uncertainties. For example, if we make an unfixed enum with only uint8_t values, it may still have a underlying_type with int32_t.

Add MOZ_IS_MSVC flag to prevent the signed/unsigned mismatch warnings on MSVC.

Add a static_assert for preventing use of 64-bit enums as comment 87 said.
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #87)
> Comment on attachment 8785244 [details]
> Bug 1297306 - part4:move IsEnumFittingWithin to mfbt/EnumTypeTraits.h.
> 
> https://reviewboard.mozilla.org/r/74518/#review74852
> 
> Remove the tests for unfixed enum since they are implementation defined and
> have many uncertainties. For example, if we make an unfixed enum with only
> uint8_t values, it may still have a underlying_type with int32_t.
> 
> Add MOZ_IS_MSVC flag to prevent the signed/unsigned mismatch warnings on
> MSVC.
> 
> Add a static_assert for preventing use of 64-bit enums as comment 87 said.

Oops, this should be comment 78.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9b0842f46f8
Comment on attachment 8785244 [details]
Bug 1297306 - part4:rename IsEnumFittingWithin with EnumTypeFitsWithin and move it to mfbt/EnumTypeTraits.h.

https://reviewboard.mozilla.org/r/74518/#review74826

Thanks for the tests; lots of comments here, but no major blockers.

::: mfbt/EnumTypeTraits.h:9
(Diff revision 6)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_EnumTypeTraits_h
> +#define mozilla_EnumTypeTraits_h
> +
> +#include <limits>

Please add a single-line comment like:

```
/* Type traits for enums. */
```

so the file gets a useful description in DXR.

::: mfbt/EnumTypeTraits.h:14
(Diff revision 6)
> +// Type trait that determines whether the integral or enum type Type can fit
> +// within the integral type Storage without loss. This should not be used with
> +// an enum type without underlying type explicitly specified (fixed).

MFBT style for these sorts of comments is:

```
/*
 * Type trait that determines...
 * ...
 * ...explicitly specified.
 */
```

The comment could use a little wordsmithing:

"Type trait that determines whether the enum type T can fit within the integral type Storage without data loss.  This trait should be used with caution with an enum type whose underlying type has not been explicitly specified: for such enums, the C++ implementation is free to choose a type no smaller than `int` whose range encompasses all possible values of the enum.  So for an enum with only small non-negative values, the underlying type may be either `int` or `unsigned int`, depending on the whims of the implementation."

Note the correspondence between T in the above comment and the template parameter.  If you would like to use Type instead, please make it consistent in both places.  The above suggestion also constrains T to be an enum type only, since allowing integral types for T really doesn't fit with the name of the trait or the intent of the header.

::: mfbt/EnumTypeTraits.h:17
(Diff revision 6)
> +template<typename T, typename Storage>
> +struct IsEnumFittingWithin
> +  : IntegralConstant<
> +      bool,
> +      std::is_integral<Storage>::value &&
> +      (std::numeric_limits<typename std::underlying_type<T>::type>::min() >=
> +       std::numeric_limits<Storage>::min()) &&
> +      (std::numeric_limits<typename std::underlying_type<T>::type>::max() <=
> +       std::numeric_limits<Storage>::max())
> +    >

I think we may be able to avoid the MSVC issues by writing the code like so:

```
namespace detail {

template<size_t EnumSize, bool EnumSigned, size_t StorageSize, bool StorageSigned>
struct EnumFitsWithinHelper;

// Signed enum, signed storage.
template<size_t EnumSize, size_t StorageSize>
struct EnumFitsWithinHelper<EnumSize, true, StorageSize, true>
  : public std::integral_constant<bool, (EnumSize <= StorageSize)>
{};

// Signed enum, unsigned storage.
template<size_t EnumSize, size_t StorageSize>
struct EnumFitsWithinHelper<EnumSize, true, StorageSize, false>
  : public td::integral_constant<bool, false>
{};

// Unsigned enum, signed storage.
template<size_t EnumSize, size_t StorageSize>
struct EnumFitsWithinHelper<EnumSize, false, StorageSize, true>
  : public std::integral_constant<bool, (EnumSize * 2 <= StorageSize)>
{};

// Unsigned enum, unsigned storage.
template<size_t EnumSize, size_t StorageSize>
struct EnumFitsWithinHelper<EnumSize, false, StorageSize, false>
  : public std::integral_constant<bool, (EnumSize <= StorageSize)>
{};

}

template<typename T, typename Storage>
struct EnumTypeFitsWithin
  : public detail::EnumFitsWithinHelper<sizeof(T), std::is_signed<typename std::underlying_type<T>::type>::value,
                                        sizeof(Storage), std::is_signed<Storage>::value>
{
  static_assert<std::is_enum<T>::value, "must provide an enum type");
  static_assert<std::is_integral<Storage>::value, "must provide an integral type");
};
```

I think that handles the 64-bit issues and any signed/unsigned comparison warnings that might crop up, too.  That way we don't have to include `<limits>`, either.

I do think we should rename the trait to `EnumTypeFitsWithin`; it reads a little better to me and also makes it clear that we're checking the type of the enum, not the values of the enum.

::: mfbt/EnumTypeTraits.h:27
(Diff revision 6)
> +{
> +};

We should add:

```
static_assert(std::is_enum<T>::value, "must provide enum type");
static_assert(std::is_integral<Storage>::value, "must provide integral type");
```

here, regardless of whether we use the more clever implementation for `EnumTypeFitsWithin`.

::: mfbt/tests/TestEnumTypeTraits.cpp:25
(Diff revision 6)
> +// Per the specification, if the underlying type is not fixed, the value is
> +// convertible to the first type from the following list able to hold their
> +// entire value range: int, unsigned int, long, unsigned long, long long,
> +// or unsigned long long. So, we make the largest possible value range for

I do not see this language in the spec.  I see in [dcl.enum]:

"For an enumeration whose underlying type is not fixed, the underlying type is an integral type that can represent all the enumerator values defined in the enumeration. If no integral type can represent all the enumerator values, the enumeration is ill-formed. It is implementation-defined which integral type is used as the underlying type except that the underlying type shall not be larger than int unless the value of an enumerator cannot fit in an int or unsigned int."

In particular, the spec's language gives the implementation freedom to choose `unsigned int` for enums, even if the range of values could fit in an `int`.

Can you modify the comment here?

::: mfbt/tests/TestEnumTypeTraits.cpp:53
(Diff revision 6)
> +// As to the tests for enum with unfixed underlying type, we can't test
> +// them precisely, since int is implementation defined. So, the tests only
> +// check for int32_t/uint32_t and its promoted integral types.

Thank you for writing complete tests here.

These tests very much depend on `int32_t` and `uint32_t` being the same size as `int`; would you please add a static assertion to that effect?

I'm a little confused about this description, though.  I think the second part of the first sentence is really trying to say, "...since the size of the underlying type for unfixed enums is implementation defined."  Is that correct?  If so, please modify the first sentence accordingly.  But then I'm not sure how the second sentence follows from the first--since the tests are checking a lot more things than just int32_t/uint32_t and their promoted integral types!  Can you please rewrite the second sentence to more clearly express your intent here?

::: mfbt/tests/TestEnumTypeTraits.cpp:67
(Diff revision 6)
> +  TestShouldFit<UnfixedEnumFor_int8_t, int32_t>();
> +  TestShouldFit<UnfixedEnumFor_int8_t, int64_t>();

Shouldn't these (and other unfixed enum tests) also `TestShouldNotFit` for `uint32_t` and `uint64_t`?

::: mfbt/tests/moz.build:20
(Diff revision 6)
>      'TestCheckedInt',
>      'TestCountPopulation',
>      'TestCountZeroes',
>      'TestEndian',
>      'TestEnumSet',
> +    'TestEnumTypeTraits',

I think you also need to add this to `testing/cppunittest.ini`.
Attachment #8785244 - Flags: review?(nfroyd)
Comment on attachment 8785244 [details]
Bug 1297306 - part4:rename IsEnumFittingWithin with EnumTypeFitsWithin and move it to mfbt/EnumTypeTraits.h.

https://reviewboard.mozilla.org/r/74518/#review74826

> Please add a single-line comment like:
> 
> ```
> /* Type traits for enums. */
> ```
> 
> so the file gets a useful description in DXR.

Will do.

> MFBT style for these sorts of comments is:
> 
> ```
> /*
>  * Type trait that determines...
>  * ...
>  * ...explicitly specified.
>  */
> ```
> 
> The comment could use a little wordsmithing:
> 
> "Type trait that determines whether the enum type T can fit within the integral type Storage without data loss.  This trait should be used with caution with an enum type whose underlying type has not been explicitly specified: for such enums, the C++ implementation is free to choose a type no smaller than `int` whose range encompasses all possible values of the enum.  So for an enum with only small non-negative values, the underlying type may be either `int` or `unsigned int`, depending on the whims of the implementation."
> 
> Note the correspondence between T in the above comment and the template parameter.  If you would like to use Type instead, please make it consistent in both places.  The above suggestion also constrains T to be an enum type only, since allowing integral types for T really doesn't fit with the name of the trait or the intent of the header.

These words are definitely better. Thank you for the polishing. Will make the comments agree with MFBT style.

> I think we may be able to avoid the MSVC issues by writing the code like so:
> 
> ```
> namespace detail {
> 
> template<size_t EnumSize, bool EnumSigned, size_t StorageSize, bool StorageSigned>
> struct EnumFitsWithinHelper;
> 
> // Signed enum, signed storage.
> template<size_t EnumSize, size_t StorageSize>
> struct EnumFitsWithinHelper<EnumSize, true, StorageSize, true>
>   : public std::integral_constant<bool, (EnumSize <= StorageSize)>
> {};
> 
> // Signed enum, unsigned storage.
> template<size_t EnumSize, size_t StorageSize>
> struct EnumFitsWithinHelper<EnumSize, true, StorageSize, false>
>   : public td::integral_constant<bool, false>
> {};
> 
> // Unsigned enum, signed storage.
> template<size_t EnumSize, size_t StorageSize>
> struct EnumFitsWithinHelper<EnumSize, false, StorageSize, true>
>   : public std::integral_constant<bool, (EnumSize * 2 <= StorageSize)>
> {};
> 
> // Unsigned enum, unsigned storage.
> template<size_t EnumSize, size_t StorageSize>
> struct EnumFitsWithinHelper<EnumSize, false, StorageSize, false>
>   : public std::integral_constant<bool, (EnumSize <= StorageSize)>
> {};
> 
> }
> 
> template<typename T, typename Storage>
> struct EnumTypeFitsWithin
>   : public detail::EnumFitsWithinHelper<sizeof(T), std::is_signed<typename std::underlying_type<T>::type>::value,
>                                         sizeof(Storage), std::is_signed<Storage>::value>
> {
>   static_assert<std::is_enum<T>::value, "must provide an enum type");
>   static_assert<std::is_integral<Storage>::value, "must provide an integral type");
> };
> ```
> 
> I think that handles the 64-bit issues and any signed/unsigned comparison warnings that might crop up, too.  That way we don't have to include `<limits>`, either.
> 
> I do think we should rename the trait to `EnumTypeFitsWithin`; it reads a little better to me and also makes it clear that we're checking the type of the enum, not the values of the enum.

Thank you so much for the mentoring. This is much more clean and clear. Will make these changes accordingly.

> We should add:
> 
> ```
> static_assert(std::is_enum<T>::value, "must provide enum type");
> static_assert(std::is_integral<Storage>::value, "must provide integral type");
> ```
> 
> here, regardless of whether we use the more clever implementation for `EnumTypeFitsWithin`.

Will do.

> I do not see this language in the spec.  I see in [dcl.enum]:
> 
> "For an enumeration whose underlying type is not fixed, the underlying type is an integral type that can represent all the enumerator values defined in the enumeration. If no integral type can represent all the enumerator values, the enumeration is ill-formed. It is implementation-defined which integral type is used as the underlying type except that the underlying type shall not be larger than int unless the value of an enumerator cannot fit in an int or unsigned int."
> 
> In particular, the spec's language gives the implementation freedom to choose `unsigned int` for enums, even if the range of values could fit in an `int`.
> 
> Can you modify the comment here?

I think you're right. I'll modify the comment accordingly.

> Thank you for writing complete tests here.
> 
> These tests very much depend on `int32_t` and `uint32_t` being the same size as `int`; would you please add a static assertion to that effect?
> 
> I'm a little confused about this description, though.  I think the second part of the first sentence is really trying to say, "...since the size of the underlying type for unfixed enums is implementation defined."  Is that correct?  If so, please modify the first sentence accordingly.  But then I'm not sure how the second sentence follows from the first--since the tests are checking a lot more things than just int32_t/uint32_t and their promoted integral types!  Can you please rewrite the second sentence to more clearly express your intent here?

You're right. I shall modify the comments accordingly.

The thing is, for an enumeration whose underlying type is not fixed, the underlying type is int (int16_t/int32_t) or unsigned int (uint16_t/uint32_t).
Since the size of the underlying type for unfixed enums is implementation defined, make unfixed enum for int8_t/int16_t/int32_t would probably the same. We can only ignore the tests for the types like int8_t/int16_t but start from int32_t, ex, TestShouldFit<UnfixedEnumFor_int8_t, int32_t>(), to do the least unit test. This is what I intent to express before. Though I'm not sure if this is still worth to test in my second thoughts. So I remove the tests for unfixed enum in the latest patch.

> Shouldn't these (and other unfixed enum tests) also `TestShouldNotFit` for `uint32_t` and `uint64_t`?

Yes they should.

> I think you also need to add this to `testing/cppunittest.ini`.

Will do.
Comment on attachment 8785244 [details]
Bug 1297306 - part4:rename IsEnumFittingWithin with EnumTypeFitsWithin and move it to mfbt/EnumTypeTraits.h.

https://reviewboard.mozilla.org/r/74518/#review74984

Issues related to unfixed enum types remain open because I removed the tests for unfixed enum types, as comment 90 states. If you think we should more or less test unfixed enum types, then I'll fix those issues.
Comment on attachment 8785244 [details]
Bug 1297306 - part4:rename IsEnumFittingWithin with EnumTypeFitsWithin and move it to mfbt/EnumTypeTraits.h.

https://reviewboard.mozilla.org/r/74518/#review75194

Thanks for the updates.  r=me with the change below (and the obvious change to remove the 64-bit `static_assert`.

::: mfbt/tests/TestEnumTypeTraits.cpp
(Diff revisions 6 - 8)
> -  // check for int64_t
> -  MAKE_FIXED_EMUM_FOR_TYPE(int64_t);
> -  TestShouldNotFit<FixedEnumFor_int64_t, int8_t>();

I think with the changes to the implementation of `EnumTypeFitsWithin`, we should be able to support 64-bit enums easily.  Could you please put the 64-bit tests back?
Attachment #8785244 - Flags: review?(nfroyd) → review+
Comment on attachment 8785244 [details]
Bug 1297306 - part4:rename IsEnumFittingWithin with EnumTypeFitsWithin and move it to mfbt/EnumTypeTraits.h.

https://reviewboard.mozilla.org/r/74518/#review75194

Will do. Thank you for the review. :-)
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/787d89593566
part1:remove unused NS_STYLE_CLEAR_* condition. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/907028433e46
part2:make the implementation of nsLineBox::LastChild() be behind DEBUG_FRAME_DUMP flag. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/cd5cf15a6fa9
part3:make BreakTypeToString() to be a member of nsLineBox. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/4088eddbb339
part4:rename IsEnumFittingWithin with EnumTypeFitsWithin and move it to mfbt/EnumTypeTraits.h. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/b122c6cd236a
part5:create enum constructors for EnumTable. r=baku
https://hg.mozilla.org/integration/autoland/rev/67f271218828
part6:replace NS_STYLE_CLEAR_* with StyleClear enum class. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/5f10d3f47379
part7:replace StyleClear related NS_ASSERTION with MOZ_ASSERT. r=xidorn
You need to log in before you can comment on or make changes to this bug.