1369260 - Remove use of MOZ_ASSERT_IF in layout

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

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

As I've said before, as module owner I prefer that MOZ_ASSERT_IF not be
used in the module because I consider it to be unreadable.  However, a
few uses have crept in, and this patch removes them.

I consider it to be unreadable because the name looks like a name that
uses smalltalk-ish naming conventions, i.e., with a part of the name
corresponding to each parameter, in order.  However, the parameters are
in the order opposite the name.

This was written primarily with the vim commands:
:%s/MOZ_ASSERT_IF(\([^,]*\),/MOZ_ASSERT(!\1 ||/
:wn
followed by manual cleanup for indentation and removal of !!.

MozReview-Commit-ID: G6rLbOn7k8d

Comment 1

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

Attachment: Remove use of MOZ_ASSERT_IF in layout

Comment 2

[Cameron McCormack (:heycam)]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #0)
> As I've said before, as module owner I prefer that MOZ_ASSERT_IF not be
> used in the module because I consider it to be unreadable.  However, a
> few uses have crept in, and this patch removes them.

I haven't seen you say this before, is it written down somewhere?  If not, could you note it in a wiki page somewhere so I can point patch authors to it when reviewing?

(I personally have a slight dislike for MOZ_ASSERT_IF, but only because it doesn't take a message argument.)

Comment 3

[Cameron McCormack (:heycam)]

Comment on attachment 8873269 [details] [diff] [review]
Remove use of MOZ_ASSERT_IF in layout

Review of attachment 8873269 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with these things fixed.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +10961,5 @@
>      }
>  
>      // Make sure we eagerly performed the servo cascade when the anonymous
>      // nodes were created.
> +    MOZ_ASSERT(!content->IsStyledByServo() && content->IsElement() ||

Hmm, this seems wrong.  We need to wrap the whole first argument to MOZ_ASSERT_IF in parens here.

::: layout/base/nsStyleChangeList.cpp
@@ +44,5 @@
>    // If Servo fires reconstruct at a node, it is the only change hint fired at
>    // that node.
>    if (IsServo()) {
>      for (size_t i = 0; i < Length(); ++i) {
> +      MOZ_ASSERT(!aContent && ((aHint | (*this)[i].mHint) & nsChangeHint_ReconstructFrame) ||

Here too.

::: layout/painting/nsCSSRendering.cpp
@@ +2505,5 @@
>                    "Frame is expected to be provided to PaintBackground");
>  
>    // If we're drawing all layers, aCompositonOp is ignored, so make sure that
>    // it was left at its default value.
> +  MOZ_ASSERT(!aParams.layer == -1 ||

And here.

::: layout/style/ServoStyleSet.cpp
@@ +382,5 @@
>    bool forAnimationOnly =
>      aRestyleBehavior == TraversalRestyleBehavior::ForAnimationOnly;
>    bool postTraversalRequired = Servo_TraverseSubtree(
>      aRoot, mRawSet.get(), &snapshots, aRootBehavior, aRestyleBehavior);
> +  MOZ_ASSERT(!isInitial || forReconstruct || !postTraversalRequired);

And here.

::: layout/svg/nsSVGUtils.cpp
@@ +543,1 @@
>                  svgReset->mClipPath.GetType() == StyleShapeSourceType::URL);

Maybe reindent this line?

Comment 4

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

Oops, thanks.  Should have thought about this a drop more.

I did the above, although for the first two I used !a || !b || ... rather than !(a && b) || ... , and for the fourth I used !=.  There were also a few others that were clearer with parentheses, although && having higher precedence than || would have made it ok.

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=773feb9ea45848885710508dc5c49ebde1d95f05&group_state=expanded

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3853c5543f6e4e8ef24cc1ac47be5a65e4b672e0
Bug 1369260 - Remove use of MOZ_ASSERT_IF in layout.  r=heycam

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3853c5543f6e