1295111 - Add runtime / compile time check for RestyleManagerBase::ChangeHintToString

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1295111 part 1 - Add UpdateBackgroundPosition hint to ChangeHintToString.

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

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1295111 part 2 - Add static_assert to ensure that is updated properly.

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

Comment 3

[Markus Stange [:mstange]]

Thanks for catching that.

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8781089 [details]
Bug 1295111 part 1 - Add UpdateBackgroundPosition hint to ChangeHintToString.

https://reviewboard.mozilla.org/r/71596/#review69196

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8781089 [details]
Bug 1295111 part 1 - Add UpdateBackgroundPosition hint to ChangeHintToString.

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

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8781090 [details]
Bug 1295111 part 2 - Add static_assert to ensure that is updated properly.

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/df6e1c83af8d
https://hg.mozilla.org/mozilla-central/rev/7f8b1c2a0802