1274891 - [Refactoring] convert nsRestyleHint and nsChangeHint from enum to enum class

Status: NEW

(Core :: CSS Parsing and Computation, defect)

Description

[Jeremy Chen ([e-mail])]

Per bug 1273766 comment 12, using enum classes gets us a small amount of safety, since we can't accidentally treat an nsChangeHint as an integer, and we would get to remove the inline operators in favour of using MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS.

Comment 1

[Boris Chiou [:boris]]

Attachment: Bug 1274891 - Part 1: Use enum class for nsChangeHint.

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

Comment 2

[Boris Chiou [:boris]]

Attachment: Bug 1274891 - Part 2: Replace nsChangeHint(0) with nsChangeHint::None.

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

Comment 3

[Boris Chiou [:boris]]

Attachment: Bug 1274891 - Part 3: Move macros into nsChangeHint enum class.

And use all caps to nsChangeHint_Hints_NotHandledForDescendants to
follow the coding style for macros.

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

Comment 4

[Boris Chiou [:boris]]

Attachment: Bug 1274891 - Part 4: Use enum class for nsRestyleHint.

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

Comment 5

[Boris Chiou [:boris]]

Attachment: Bug 1274891 - Part 5: Introduce nsRestyleHint::None.

We use enum class now, so I think it's better to replace nsRestyleHint(0) with
nsRestyleHint::Empty. Let both nsChangeHint and nsRestyleHint use |Empty| as 0.

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

Comment 6

[Boris Chiou [:boris]]

Comment on attachment 8821090 [details]
Bug 1274891 - Part 1: Use enum class for nsChangeHint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100466/diff/1-2/

Comment 7

[Boris Chiou [:boris]]

Comment on attachment 8821091 [details]
Bug 1274891 - Part 2: Replace nsChangeHint(0) with nsChangeHint::None.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100468/diff/1-2/

Comment 8

[Boris Chiou [:boris]]

Comment on attachment 8821092 [details]
Bug 1274891 - Part 3: Move macros into nsChangeHint enum class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100470/diff/1-2/

Comment 9

[Boris Chiou [:boris]]

Comment on attachment 8821090 [details]
Bug 1274891 - Part 1: Use enum class for nsChangeHint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100466/diff/2-3/

Comment 10

[Boris Chiou [:boris]]

Comment on attachment 8821091 [details]
Bug 1274891 - Part 2: Replace nsChangeHint(0) with nsChangeHint::None.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100468/diff/2-3/

Comment 11

[Boris Chiou [:boris]]

Comment on attachment 8821092 [details]
Bug 1274891 - Part 3: Move macros into nsChangeHint enum class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100470/diff/2-3/

Comment 12

[Boris Chiou [:boris]]

Comment on attachment 8821473 [details]
Bug 1274891 - Part 4: Use enum class for nsRestyleHint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100760/diff/1-2/

Comment 13

[Boris Chiou [:boris]]

Comment on attachment 8821474 [details]
Bug 1274891 - Part 5: Introduce nsRestyleHint::None.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100762/diff/1-2/

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8821090 [details]
Bug 1274891 - Part 1: Use enum class for nsChangeHint.

https://reviewboard.mozilla.org/r/100466/#review101216

r=me with these things addressed.

::: dom/svg/nsSVGElement.cpp:2400
(Diff revision 2)
>      // The SetAttrAndNotify doesn't happen for transform changes caused by
>      // 'animateTransform' though (and sending out the mutation events that
>      // nsNodeUtills::AttributeChanged dispatches would be inappropriate
>      // anyway), so we need to post the change event ourself.
>      nsChangeHint changeHint = GetAttributeChangeHint(transformAttr, aModType);
> -    if (changeHint) {
> +    if (static_cast<bool>(changeHint)) {

Seems more common to use |bool(...)| rather than |static_cast<bool>(...)| in the codebase, so let's do that.

::: layout/base/RestyleManager.cpp:315
(Diff revision 2)
>        aContent, ContentTag(aElement, 0), frame));
>  #endif
>  
>    // the style tag has its own interpretation based on aHint
>    nsChangeHint hint = aElement->GetAttributeChangeHint(aAttribute, aModType);
> +  bool reframe = static_cast<bool>(hint & nsChangeHint::ReconstructFrame);

This cast shouldn't be needed since the result of the operator& is a kind of intermediate object that implicitly converts to bool.

::: layout/base/RestyleManager.cpp:951
(Diff revision 2)
>    if (MOZ_UNLIKELY(IsDisconnected()) ||
>        MOZ_UNLIKELY(PresContext()->PresShell()->IsDestroying())) {
>      return;
>    }
>  
> -  if (aRestyleHint == 0 && !aMinChangeHint) {
> +  if (aRestyleHint == 0 && !static_cast<bool>(aMinChangeHint)) {

bool()

::: layout/base/RestyleManager.cpp:1575
(Diff revision 2)
>      if (!layer &&
>          nsLayoutUtils::HasEffectiveAnimation(mFrame, layerInfo.mProperty)) {
>        hint |= layerInfo.mChangeHint;
>      }
>    }
> -  if (hint) {
> +  if (static_cast<bool>(hint)) {

bool()

::: layout/base/RestyleManager.cpp:3338
(Diff revision 2)
>  
>    PROFILER_LABEL_PRINTF("ElementRestyler", "ComputeStyleChangeFor",
>                          js::ProfileEntry::Category::CSS,
>                          content ? "Element: %s" : "%s",
>                          content ? localDescriptor.get() : "");
> -  if (aMinChange) {
> +  if (static_cast<bool>(aMinChange)) {

bool()

::: layout/base/RestyleManagerBase.cpp:162
(Diff revision 2)
> -  uint32_t hint = aHint & ((1 << ArrayLength(names)) - 1);
> -  uint32_t rest = aHint & ~((1 << ArrayLength(names)) - 1);
> -  if (hint == nsChangeHint_Hints_NotHandledForDescendants) {
> +  uint32_t hint = static_cast<nsChangeHintTypeBase>(aHint) &
> +                    ((1 << ArrayLength(names)) - 1);
> +  uint32_t rest = static_cast<nsChangeHintTypeBase>(aHint) &
> +                    ~((1 << ArrayLength(names)) - 1);

Hmm, the fact that we disable operator== on nsChangeHint makes this a real pain, here. :-)

::: layout/base/RestyleManagerBase.cpp:873
(Diff revision 2)
> -  NS_ASSERTION(nsChangeHint_size_t(aChange) ==
> -                          (aChange & (nsChangeHint_RepaintFrame |
> -                                      nsChangeHint_SyncFrameView |
> -                                      nsChangeHint_UpdateOpacityLayer |
> -                                      nsChangeHint_SchedulePaint)),
> +  NS_ASSERTION(static_cast<nsChangeHintTypeBase>(aChange) ==
> +               static_cast<nsChangeHintTypeBase>(
> +                 aChange & (nsChangeHint::RepaintFrame |
> +                            nsChangeHint::SyncFrameView |
> +                            nsChangeHint::UpdateOpacityLayer |
> +                            nsChangeHint::SchedulePaint)),
>                 "Invalid change flag");

I guess we could do:

  NS_ASSERTION(!(aChange & ~(nsChangeHint::RepaintFrame |
                             ...

and avoid the casting.

::: layout/base/RestyleManagerBase.cpp:925
(Diff revision 2)
>                 "Unexpected UpdateTransformLayer hint");
>  
>    if (aPresShell->IsPaintingSuppressed()) {
>      // Don't allow synchronous rendering changes when painting is turned off.
> -    aChange &= ~nsChangeHint_RepaintFrame;
> -    if (!aChange) {
> +    aChange &= ~nsChangeHint::RepaintFrame;
> +    if (!static_cast<bool>(aChange)) {

!bool()

::: layout/base/RestyleManagerBase.cpp:949
(Diff revision 2)
>  
>      if (propagatedFrame != aFrame) {
> -      DoApplyRenderingChangeToTree(propagatedFrame, nsChangeHint_RepaintFrame);
> -      aChange &= ~nsChangeHint_RepaintFrame;
> -      if (!aChange) {
> +      DoApplyRenderingChangeToTree(propagatedFrame,
> +                                   nsChangeHint::RepaintFrame);
> +      aChange &= ~nsChangeHint::RepaintFrame;
> +      if (!static_cast<bool>(aChange)) {

!bool()

::: layout/base/RestyleTracker.cpp:99
(Diff revision 2)
>            LoggingDepth(), RestyleManager::StructsToLog());
>      }
>  #endif
>      mRestyleManager->RestyleElement(aElement, primaryFrame, aChangeHint,
>                                      *this, aRestyleHint, aRestyleHintData);
> -  } else if (aChangeHint &&
> +  } else if (static_cast<bool>(aChangeHint) &&

bool()

::: layout/base/ServoRestyleManager.cpp:38
(Diff revision 2)
>    if (MOZ_UNLIKELY(IsDisconnected()) ||
>        MOZ_UNLIKELY(PresContext()->PresShell()->IsDestroying())) {
>      return;
>    }
>  
> -  if (aRestyleHint == 0 && !aMinChangeHint && !HasPendingRestyles()) {
> +  if (aRestyleHint == 0 && !static_cast<bool>(aMinChangeHint) &&

!bool()

::: layout/base/ServoRestyleManager.cpp:73
(Diff revision 2)
>    if (aRestyleHint & eRestyle_Subtree) {
>      aRestyleHint &= ~eRestyle_Subtree;
>      aRestyleHint |= eRestyle_Self | eRestyle_SomeDescendants;
>    }
>  
> -  if (aRestyleHint || aMinChangeHint) {
> +  if (aRestyleHint || static_cast<bool>(aMinChangeHint)) {

bool()

::: layout/base/ServoRestyleManager.cpp:139
(Diff revision 2)
>    nsIFrame* primaryFrame = aElement->GetPrimaryFrame();
>  
>    // FIXME(bholley): Once we transfer ownership of the styles to the frame, we
>    // can fast-reject without the FFI call by checking mServoData for null.
>    nsChangeHint changeHint = Servo_CheckChangeHint(aElement);
> -  if (changeHint) {
> +  if (static_cast<bool>(changeHint)) {

bool()

::: layout/base/ServoRestyleManager.cpp:155
(Diff revision 2)
>      return;
>    }
>  
>    // If we have a frame and a non-zero + non-reconstruct change hint, we need to
>    // attach a new style context.
> -  bool recreateContext = primaryFrame && changeHint;
> +  bool recreateContext = primaryFrame && static_cast<bool>(changeHint);

bool()

::: layout/base/nsChangeHint.h:22
(Diff revision 2)
>  
>  // Defines for various style related constants
>  
> -enum nsChangeHint {
> -  nsChangeHint_Empty = 0,
> +typedef uint32_t nsChangeHintTypeBase;
> +enum class nsChangeHint : nsChangeHintTypeBase {
> +  Empty = 0,

I think I prefer the existing naming of "None".

::: layout/base/nsPresContext.cpp:1886
(Diff revision 2)
>    if (mUsesViewportUnits && mPendingViewportChange) {
>      // Rebuild all style data without rerunning selector matching.
>      aRestyleHint |= eRestyle_ForceDescendants;
>    }
>  
> -  if (aRestyleHint || aChangeHint) {
> +  if (aRestyleHint || static_cast<bool>(aChangeHint)) {

bool()

::: layout/style/nsStyleContext.cpp:1050
(Diff revision 2)
>            this##struct_->CalcDifference(*other##struct_ EXTRA_DIFF_ARGS);     \
>          NS_ASSERTION(NS_IsHintSubset(difference, maxDifference),              \
>                       "CalcDifference() returned bigger hint than "            \
>                       "MaxDifference()");                                      \
>          hint |= difference;                                                   \
> -        if (!difference) {                                                    \
> +        if (!static_cast<bool>(difference)) {                   \

!bool()

and please fix up the backslash alignment.

::: layout/style/nsStyleContext.cpp:1061
(Diff revision 2)
>          nsChangeHint difference =                                             \
>            this##struct_->CalcDifference(*other##struct_ EXTRA_DIFF_ARGS);     \
>          NS_ASSERTION(NS_IsHintSubset(difference, maxDifference),              \
>                       "CalcDifference() returned bigger hint than "            \
>                       "MaxDifference()");                                      \
> -        if (!difference) {                                                    \
> +        if (!static_cast<bool>(difference)) {                   \

Here too.

::: layout/style/nsStyleStruct.cpp:2518
(Diff revision 2)
>    NS_FOR_VISIBLE_IMAGE_LAYERS_BACK_TO_FRONT(i, moreLayers) {
>      if (i < lessLayers.mImageCount) {
>        nsChangeHint layerDifference =
>          moreLayers.mLayers[i].CalcDifference(lessLayers.mLayers[i]);
>        hint |= layerDifference;
> -      if (layerDifference &&
> +      if (static_cast<bool>(layerDifference) &&

bool()

::: layout/style/nsStyleStruct.cpp:2536
(Diff revision 2)
>    if (aType == nsStyleImageLayers::LayerType::Mask &&
>        mImageCount != aNewLayers.mImageCount) {
> -    hint |= nsChangeHint_UpdateEffects;
> +    hint |= nsChangeHint::UpdateEffects;
>    }
>  
> -  if (hint) {
> +  if (static_cast<bool>(hint)) {

bool()

::: layout/style/nsStyleStruct.cpp:3348
(Diff revision 2)
>  
>      if (mBackfaceVisibility != aNewData.mBackfaceVisibility) {
> -      transformHint |= nsChangeHint_RepaintFrame;
> +      transformHint |= nsChangeHint::RepaintFrame;
>      }
>  
> -    if (transformHint) {
> +    if (static_cast<bool>(transformHint)) {

bool()

::: layout/style/nsStyleStruct.cpp:3397
(Diff revision 2)
>    // that opportunity to handle dynamic changes appropriately.
>  
> -  // But we still need to return nsChangeHint_NeutralChange for these
> +  // But we still need to return nsChangeHint::NeutralChange for these
>    // properties, since some data did change in the style struct.
>  
> -  if (!hint &&
> +  if (!static_cast<bool>(hint) &&

bool()

::: layout/style/nsStyleStruct.cpp:3890
(Diff revision 2)
>        mWebkitTextStrokeColor != aNewData.mWebkitTextStrokeColor) {
> -    hint |= nsChangeHint_SchedulePaint |
> -            nsChangeHint_RepaintFrame;
> +    hint |= nsChangeHint::SchedulePaint |
> +            nsChangeHint::RepaintFrame;
>    }
>  
> -  if (hint) {
> +  if (static_cast<bool>(hint)) {

bool()

::: layout/style/nsStyleStruct.cpp:4213
(Diff revision 2)
>  
>    if (mMixBlendMode != aNewData.mMixBlendMode) {
> -    hint |= nsChangeHint_RepaintFrame;
> +    hint |= nsChangeHint::RepaintFrame;
>    }
>  
> -  if (!hint &&
> +  if (!static_cast<bool>(hint) &&

!bool()

::: layout/tables/nsTableFrame.cpp:4859
(Diff revision 2)
>    if (!oldStyleData)
>      return false;
>  
>    const nsStyleBorder* newStyleData = aNewStyleContext->StyleBorder();
>    nsChangeHint change = newStyleData->CalcDifference(*oldStyleData);
> -  if (!change)
> +  if (!static_cast<bool>(change))

!bool()

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8821091 [details]
Bug 1274891 - Part 2: Replace nsChangeHint(0) with nsChangeHint::None.

https://reviewboard.mozilla.org/r/100468/#review101230

r=me but I think we should stay with the nsChangeHint::None naming,

Comment 16

[Boris Chiou [:boris]]

Comment on attachment 8821091 [details]
Bug 1274891 - Part 2: Replace nsChangeHint(0) with nsChangeHint::None.

https://reviewboard.mozilla.org/r/100468/#review101230

How about replace |Empty| with |None|, so we keep nsChangeHint::None and nsRestyleHint::None(part 5)?

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8821091 [details]
Bug 1274891 - Part 2: Replace nsChangeHint(0) with nsChangeHint::None.

https://reviewboard.mozilla.org/r/100468/#review101258

Comment 18

[Boris Chiou [:boris]]

Comment on attachment 8821090 [details]
Bug 1274891 - Part 1: Use enum class for nsChangeHint.

https://reviewboard.mozilla.org/r/100466/#review101216

> I think I prefer the existing naming of "None".

OK. I will replace it with |None| in part 2.

Comment 19

[Boris Chiou [:boris]]

Comment on attachment 8821090 [details]
Bug 1274891 - Part 1: Use enum class for nsChangeHint.

https://reviewboard.mozilla.org/r/100466/#review101216

> This cast shouldn't be needed since the result of the operator& is a kind of intermediate object that implicitly converts to bool.

I tried that, but got this error message: 

error: no viable conversion from 'mozilla::CastableTypedEnumResult<nsChangeHint>' to 'bool'

I guess only "if(mozilla::CastableTypedEnumResult<nsChangeHint>)" could convert to bool implicitly, because we define 'operator bool()' explicitly, so I'd like to use the alternate way:
`!!(hint & nsChangeHint::ReconstructFrame)`
because we define "operator!()" implicitly.

Comment 20

[Boris Chiou [:boris]]

Comment on attachment 8821090 [details]
Bug 1274891 - Part 1: Use enum class for nsChangeHint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100466/diff/3-4/

Comment 21

[Boris Chiou [:boris]]

Comment on attachment 8821091 [details]
Bug 1274891 - Part 2: Replace nsChangeHint(0) with nsChangeHint::None.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100468/diff/3-4/

Comment 22

[Boris Chiou [:boris]]

Comment on attachment 8821092 [details]
Bug 1274891 - Part 3: Move macros into nsChangeHint enum class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100470/diff/3-4/

Comment 23

[Boris Chiou [:boris]]

Comment on attachment 8821473 [details]
Bug 1274891 - Part 4: Use enum class for nsRestyleHint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100760/diff/2-3/

Comment 24

[Boris Chiou [:boris]]

Comment on attachment 8821474 [details]
Bug 1274891 - Part 5: Introduce nsRestyleHint::None.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100762/diff/2-3/

Comment 25

[Boris Chiou [:boris]]

Part 4 made some tests failed:

TEST-UNEXPECTED-FAIL | layout/style/test/test_restyles_in_smil_animation.html | should restyle in most frames
TEST-UNEXPECTED-FAIL | layout/style/test/test_restyles_in_smil_animation.html | should restyle again 

and

TEST-UNEXPECTED-FAIL | dom/animation/test/chrome/test_restyles.html | CSS animations running on the main-thread should update style on the main thread - got +0, expected 5
...

so I'm trying to figure out what happened.

Comment 26

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #25)
> Part 4 made some tests failed:
> 
> TEST-UNEXPECTED-FAIL |
> layout/style/test/test_restyles_in_smil_animation.html | should restyle in
> most frames
> TEST-UNEXPECTED-FAIL |
> layout/style/test/test_restyles_in_smil_animation.html | should restyle
> again 
> 
> and
> 
> TEST-UNEXPECTED-FAIL | dom/animation/test/chrome/test_restyles.html | CSS
> animations running on the main-thread should update style on the main thread
> - got +0, expected 5
> ...
> 
> so I'm trying to figure out what happened.

I know the root cause: we use RestyleManagerBase::RestyleHintToString() to convert the nsRestyleHint into a string, so these tests can check the nsRestyleHints by these strings. I revised the string format in part 4, but didn't revise the string format in the test cases. I will update part 4 soon.

Comment 27

[Boris Chiou [:boris]]

Comment on attachment 8821473 [details]
Bug 1274891 - Part 4: Use enum class for nsRestyleHint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100760/diff/3-4/

Comment 28

[Boris Chiou [:boris]]

Comment on attachment 8821474 [details]
Bug 1274891 - Part 5: Introduce nsRestyleHint::None.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100762/diff/3-4/

Comment 29

[Cameron McCormack (:heycam)]

Comment on attachment 8821092 [details]
Bug 1274891 - Part 3: Move macros into nsChangeHint enum class.

https://reviewboard.mozilla.org/r/100470/#review105840

::: layout/base/nsChangeHint.h:280
(Diff revision 4)
>  // The most hints that NS_HintsNotHandledForDescendantsIn could possibly return:
> -#define nsChangeHint_Hints_NotHandledForDescendants nsChangeHint( \
> -          nsChangeHint::UpdateTransformLayer | \
> +#define NS_CHANGE_HINT_NOT_HANDLED_FOR_DESCENDANTS          \
> +  nsChangeHint(nsChangeHint::UpdateTransformLayer |         \

Can we move this into the "useful unions" section above, and name it "NotHandledForDescendantsHints" instead?

Comment 30

[Boris Chiou [:boris]]

Let's hold this bug until stylo is stable.