Closed
Bug 1369260
Opened 7 years ago
Closed 7 years ago
Remove use of MOZ_ASSERT_IF in layout
Categories
(Core :: Layout, enhancement)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: dbaron, Assigned: dbaron)
Details
Attachments
(1 file)
33.53 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8873269 -
Flags: review?(cam)
Comment 2•7 years ago
|
||
(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•7 years ago
|
||
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+
Assignee | ||
Comment 4•7 years ago
|
||
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.
Assignee | ||
Comment 5•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=773feb9ea45848885710508dc5c49ebde1d95f05&group_state=expanded
Assignee | ||
Comment 6•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3853c5543f6e4e8ef24cc1ac47be5a65e4b672e0 Bug 1369260 - Remove use of MOZ_ASSERT_IF in layout. r=heycam
Comment 7•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3853c5543f6e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•