1388162 - Add a Destroy function to display items

Status: RESOLVED FIXED

(Core :: Web Painting, enhancement)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: display-item-destroy

If we want to retain display items, and recycle memory from unused ones, then we need to know when they get destroyed.

This adds a Destroy function to nsDisplayItem and makes the destructor protected so that all callers use that instead.

Later on, we can use the Destroy function to tell the builder that it can recycle the memory for this item.

Comment 1

[Markus Stange [:mstange]]

Comment on attachment 8894640 [details] [diff] [review]
display-item-destroy

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

This seems reasonable.

::: layout/painting/nsDisplayList.h
@@ +128,5 @@
> +  }
> +
> +
> +// Contains all the type integers for each display list item type
> +#include "nsDisplayItemTypes.h"

I don't really understand the reasoning for putting these enums outside nsDisplayItem. I appreciate that it makes the types easier to type (haha) but I also wonder if names like TYPE_MASK are really unique enough to not collide with something else. We're completely outside any namespace here, aren't we?

@@ +3852,5 @@
>      mBaseVisibleRect = mVisibleRect;
>    }
>    virtual ~nsDisplayWrapList();
> +
> +  virtual void Destroy(nsDisplayListBuilder* aBuilder) override {                                                                                                                  

lots of whitespace here

@@ +4674,5 @@
>      StoreList(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,
>                nsDisplayItem* aItem) :
>        nsDisplayWrapList(aBuilder, aFrame, aItem) {}
>      virtual ~StoreList() {}
> +    

whitespace

@@ +4734,5 @@
>    {
>      MOZ_COUNT_DTOR(nsDisplayTransform);
>    }
>  #endif
> +  

whitespace

@@ +5124,5 @@
>      if (mList.GetChildren()->GetTop()) {
>        static_cast<nsDisplayTransform*>(mList.GetChildren()->GetTop())->DoUpdateBoundsPreserves3D(aBuilder);
>      }
>    }
> +  

whitespace

Comment 2

[Matt Woodrow (:mattwoodrow)]

(In reply to Markus Stange [:mstange] from comment #1)
> Comment on attachment 8894640 [details] [diff] [review]
> display-item-destroy
> 
> Review of attachment 8894640 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems reasonable.
> 
> ::: layout/painting/nsDisplayList.h
> @@ +128,5 @@
> > +  }
> > +
> > +
> > +// Contains all the type integers for each display list item type
> > +#include "nsDisplayItemTypes.h"
> 
> I don't really understand the reasoning for putting these enums outside
> nsDisplayItem. I appreciate that it makes the types easier to type (haha)
> but I also wonder if names like TYPE_MASK are really unique enough to not
> collide with something else. We're completely outside any namespace here,
> aren't we?

The builder needs to know about the enum for the Allocate function, and the builder is defined before nsDisplayItem.

It's pretty painful to untangle.

Comment 3

[Markus Stange [:mstange]]

Ah, I see. I guess you could make it an enum class, which would provide the necessary namespacing?

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9985564e081
Add a Destroy function to nsDisplayItem to use instead of manually invoking the destructor, this will allow us to recycle them in the future. r=mstange

Comment 5

[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 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54fa8df4ba28
Add a Destroy function to nsDisplayItem to use instead of manually invoking the destructor, this will allow us to recycle them in the future. r=mstange

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/54fa8df4ba28