Closed
Bug 1420298
Opened 7 years ago
Closed 7 years ago
Implement retained-vs-non-retained display list checker
Categories
(Core :: Web Painting, enhancement)
Core
Web Painting
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: mozbugz, Assigned: mozbugz)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
"layout.display-list.build-twice" runs a non-retained display list builder after a retained one.
It would be useful to compare their results, to hopefully highlight bugs in the retained builder.
I will add "layout.display-list.retain.verify" to do that, and output the differing display lists.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
Sorry for the big-ish patch, but it's mostly new interconnected code. But if you'd prefer me to break it in more easily-reviewable pieces, just ask!
Try with layout.display-list.build-twice=true:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be90790c5378d45b1ab7f9c78ec23b663c449eb9
(Pick almost any mochitest log, and look for "****" to find checker results.)
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8931534 [details]
Bug 1420298 'layout.display-list.retain.verify' to debug retained-dl -
https://reviewboard.mozilla.org/r/202656/#review207986
Looks great, especially since we know it already found real bugs.
Do you think it's worth adding an NS_ASSERTION for when it fails? That won't crash, but it will cause test failures, so it'll be easier to find where we have problems. Depends on the rate of false positives I guess.
It's also a lot of new code into nsLayoutUtils. Think you could move the new class into nsLayoutDebugger (layout/base, but probably should be layout/painting), or a new file in layout/painting?
r=me with that.
Attachment #8931534 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 4•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8931534 [details]
Bug 1420298 'layout.display-list.retain.verify' to debug retained-dl -
https://reviewboard.mozilla.org/r/202656/#review207986
Thank you for the quick review.
Re: NS_ASSERTION,
- The check is disabled by default (it's expensive!), so the assertion wouldn't appear unless we added tests/builds with the pref turned on;
- If enabled, I don't think it would be useful now or even acceptable, because of bug 1418840 which gets triggered during most mochitest runs (when changing sub-doc between each test). So we should wait until that one is fixed, and any other pending ones if any, to make sure we start from an all-green state.
Ok I will move the code elsewhere.
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8931534 [details]
Bug 1420298 'layout.display-list.retain.verify' to debug retained-dl -
https://reviewboard.mozilla.org/r/202656/#review208324
C/C++ static analysis found 1 defect in this patch.
You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
::: layout/painting/DisplayListChecker.cpp:341
(Diff revision 2)
> +DisplayListChecker::DisplayListChecker(nsDisplayList* aList, const char* aName)
> + : mBlueprint(MakeUnique<DisplayListBlueprint>(aList, aName))
> +{
> +}
> +
> +DisplayListChecker::~DisplayListChecker() {}
Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]
DisplayListChecker::~DisplayListChecker() {}
^
= default;
Comment hidden (mozreview-request) |
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2948003d230a
'layout.display-list.retain.verify' to debug retained-dl - r=mattwoodrow
Comment 9•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•