Closed Bug 1295111 Opened 4 years ago Closed 4 years ago

Add runtime / compile time check for RestyleManagerBase::ChangeHintToString

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(2 files)

Sometimes people miss the "IMPORTANT NOTE" in nsChangeHint and forget to update the name list in RestyleManagerBase::ChangeHintToString, which may cause issues when debugging.

Luckily sometimes this can be caught by Coverity (when nsChangeHint_Hints_NoHandledForDescendants is updated), and that's why I found they are out-of-sync currently. But we are not always lucky, so we need a more robust way to catch that.
Thanks for catching that.
Comment on attachment 8781089 [details]
Bug 1295111 part 1 - Add UpdateBackgroundPosition hint to ChangeHintToString.

https://reviewboard.mozilla.org/r/71596/#review69196
Attachment #8781089 - Flags: review?(dbaron) → review+
Comment on attachment 8781090 [details]
Bug 1295111 part 2 - Add static_assert to ensure that is updated properly.

https://reviewboard.mozilla.org/r/71598/#review69198

::: layout/base/nsChangeHint.h:209
(Diff revision 1)
>    // IMPORTANT NOTE: When adding new hints, consider whether you need to
>    // add them to NS_HintsNotHandledForDescendantsIn() below.  Please also
>    // add them to RestyleManager::ChangeHintToString.

Even though it's just a few lines below, please update this comment to say to modify nsChangeHint_AllHints.

::: layout/style/nsStyleContext.cpp:1224
(Diff revision 1)
>        hint |= nsChangeHint_RepaintFrame;
>      }
>    }
>  
> +  MOZ_ASSERT(NS_IsHintSubset(hint, nsChangeHint_AllHints),
> +             "Added any new hint without bumping AllHints?");

"any new hint" -> "a new hint"
Comment on attachment 8781090 [details]
Bug 1295111 part 2 - Add static_assert to ensure that is updated properly.

https://reviewboard.mozilla.org/r/71598/#review69202

Oops, I hit the new "Publish" button when I meant to hit "Finish" instead.

Thanks for fixing this.
Attachment #8781090 - Flags: review?(dbaron) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/df6e1c83af8d
part 1 - Add UpdateBackgroundPosition hint to ChangeHintToString. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/7f8b1c2a0802
part 2 - Add static_assert to ensure that is updated properly. r=dbaron
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.