Closed Bug 1369260 Opened 7 years ago Closed 7 years ago

Remove use of MOZ_ASSERT_IF in layout

Categories

(Core :: Layout, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: dbaron, Assigned: dbaron)

Details

Attachments

(1 file)

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
(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 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?
Attachment #8873269 - Flags: review?(cam) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/3853c5543f6e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: