Open
Bug 1274891
Opened 8 years ago
Updated 2 years ago
[Refactoring] convert nsRestyleHint and nsChangeHint from enum to enum class
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
NEW
People
(Reporter: chenpighead, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(5 files)
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.
Updated•8 years ago
|
Assignee: nobody → boris.chiou
Updated•8 years ago
|
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
mozreview-review |
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()
Attachment #8821090 -
Flags: review+
Comment 15•7 years ago
|
||
mozreview-review |
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,
Attachment #8821091 -
Flags: review+
Comment 16•7 years ago
|
||
mozreview-review-reply |
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•7 years ago
|
||
mozreview-review |
Comment on attachment 8821091 [details] Bug 1274891 - Part 2: Replace nsChangeHint(0) with nsChangeHint::None. https://reviewboard.mozilla.org/r/100468/#review101258
Comment 18•7 years ago
|
||
mozreview-review-reply |
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•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Attachment #8821092 -
Flags: review?(cam)
Attachment #8821473 -
Flags: review?(cam)
Attachment #8821474 -
Flags: review?(cam)
Comment 25•7 years ago
|
||
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•7 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 29•7 years ago
|
||
mozreview-review |
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?
Updated•7 years ago
|
Summary: convert nsRestyleHint and nsChangeHint from enum to enum class → [Refactoring] convert nsRestyleHint and nsChangeHint from enum to enum class
Comment 30•7 years ago
|
||
Let's hold this bug until stylo is stable.
Updated•7 years ago
|
Attachment #8821473 -
Flags: review?(cam)
Attachment #8821474 -
Flags: review?(cam)
Updated•7 years ago
|
Attachment #8821092 -
Flags: review?(cam)
Updated•6 years ago
|
Assignee: boris.chiou → nobody
Status: ASSIGNED → NEW
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•