1388161 - Store display list dirty rects in the builder rather than as a parameter

Status: RESOLVED FIXED

(Core :: Web Painting, enhancement)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: change-builddisplaylist-signature

Retained display lists needs two rects (the visible rect, and the rect that we're actually building for), and passing both of these to every BuildDisplayList call got painful.

We already store the dirty rect on the display list builder, so this just makes sure that it's always accurate and drops the parameter.

Comment 1

[Markus Stange [:mstange]]

Comment on attachment 8894638 [details] [diff] [review]
change-builddisplaylist-signature

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

::: layout/generic/nsBlockFrame.cpp
@@ +6776,5 @@
>           ++line) {
>        nsRect lineArea = line->GetVisualOverflowArea();
>        if (!lineArea.IsEmpty()) {
>          // Because we have a cursor, the combinedArea.ys are non-decreasing.
>          // Once we've passed aDirtyRect.YMost(), we can never see it again.

Completely unrelated, but I wonder how this works with top-to-bottom text.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +3526,5 @@
>        scrolledRectClipState.ClipContainingBlockDescendants(
>          scrolledRectClip + aBuilder->ToReferenceFrame(mOuter));
>  
> +      nsDisplayListBuilder::AutoBuildingDisplayList
> +        building(aBuilder, mOuter, dirtyRect, aBuilder->IsAtRootOfPseudoStackingContext());

So do you use AutoBuildingDisplayList whenever you don't want to call SetDirtyRect twice? (once to set, and another time to restore)

@@ +3623,5 @@
>      wasUsingDisplayPort = nsLayoutUtils::HasDisplayPort(content);
>  
>      if (aAllowCreateDisplayPort) {
>        nsLayoutUtils::MaybeCreateDisplayPort(*aBuilder, mOuter);
> +      

you're adding whitespace here

::: layout/generic/nsPageFrame.cpp
@@ +554,5 @@
>      nsRect dirtyRect = child->GetVisualOverflowRectRelativeToSelf();
> +    nsDisplayListBuilder::AutoBuildingDisplayList
> +      buildingForChild(aBuilder, child, dirtyRect,
> +                       aBuilder->IsAtRootOfPseudoStackingContext());
> +    child->BuildDisplayListForStackingContext(aBuilder, &content);

Maybe wrap this in its own scope so that this AutoBuildingDisplayList doesn't wrap all the other AutoBuildingDisplayList objects in this function. Might not make a difference, but the way it is now makes me (possibly unnecessarily) worry that it might not be ok when I read this code.

::: layout/painting/nsDisplayList.h
@@ +1383,5 @@
>      nsDisplayListBuilder* mBuilder;
>      Preserves3DContext mSavedCtx;
>    };
>  
> +  const nsRect GetPreserves3DRects() const {

why Rects and not Rect?

@@ +1414,5 @@
>    /**
>     * Returns whether a frame acts as an animated geometry root, optionally
>     * returning the next ancestor to check.
>     */
>    AGRState IsAnimatedGeometryRoot(nsIFrame* aFrame,

All the AGR methods and mFrameToAnimatedGeometryRootMap seem to have become public. Intentional?

::: layout/tables/nsTableFrame.cpp
@@ +1398,3 @@
>  {
>    for (nsTableRowFrame* row = aRowGroup->GetFirstRow(); row; row = row->GetNextRow()) {
> +    if (!aBuilder->GetDirtyRect().Intersects(nsRect(row->GetNormalPosition(), row->GetSize()))) {

This line is a bit on the long side.

::: layout/xul/nsRootBoxFrame.cpp
@@ +174,5 @@
>    if (mContent && mContent->GetProperty(nsGkAtoms::DisplayPortMargins)) {
>      // The XUL document's root element may have displayport margins set in
>      // ChromeProcessController::InitializeRoot, and we should to supply the
>      // base rect.
> +    nsRect displayPortBase = aBuilder->GetDirtyRect().Intersect(nsRect(nsPoint(0, 0), GetSize()));

long line

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f2dd8f13b53
Store the dirty rect on the display list builder rather than passing it as a parameter to BuildDisplayList. r=mstange

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out the push with bug 1388614, bug 1388162 and bug 1388161:

bug 1388614: https://hg.mozilla.org/integration/mozilla-inbound/rev/97d71323c3f9a6b7d034a692ae2ad9cd489357f0
bug 1388162: https://hg.mozilla.org/integration/mozilla-inbound/rev/3c4d5576374d3a4dd74945e7246576a8877dca79
bug 1388161: https://hg.mozilla.org/integration/mozilla-inbound/rev/c0f5be3f9f5239098364712f31d7a80f3e10cd3b

Push with failures (all non-e10s?): https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6b2edbf5944a1afbb8670920eae565e5543a735e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=122267227&repo=mozilla-inbound

[task 2017-08-10T12:19:28.831476Z] 12:19:28     INFO - TEST-START | dom/animation/test/chrome/test_animation_performance_warning.html
[task 2017-08-10T12:19:29.736537Z] 12:19:29     INFO - TEST-INFO | started process screentopng
[task 2017-08-10T12:19:30.035665Z] 12:19:30     INFO - TEST-INFO | screentopng: exit 0
[task 2017-08-10T12:19:30.037377Z] 12:19:30     INFO - Buffered messages logged at 12:19:29
[task 2017-08-10T12:19:30.038090Z] 12:19:30     INFO - TEST-PASS | dom/animation/test/chrome/test_animation_performance_warning.html | Bug 1196114 - Test metadata related to which animation properties are running on the compositor - Bug 1196114 - Test metadata related to which animation properties are running on the compositor: Elided 21 passes or known failures.
[task 2017-08-10T12:19:30.038143Z] 12:19:30     INFO - Buffered messages finished
[task 2017-08-10T12:19:30.038604Z] 12:19:30     INFO - TEST-UNEXPECTED-FAIL | dom/animation/test/chrome/test_animation_performance_warning.html | preserve-3d transform - preserve-3d transform: assert_true: runningOnCompositor property should be true on transform expected true got false

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d668e62d4691
Store the dirty rect on the display list builder rather than passing it as a parameter to BuildDisplayList. r=mstange

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/d668e62d4691