Closed Bug 1388162 Opened 3 years ago Closed 3 years ago

Add a Destroy function to display items

Categories

(Core :: Web Painting, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow, NeedInfo)

References

Details

Attachments

(1 file)

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.
Attachment #8894640 - Flags: review?(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
Attachment #8894640 - Flags: review?(mstange) → review+
(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.
Ah, I see. I guess you could make it an enum class, which would provide the necessary namespacing?
Pushed by mwoodrow@mozilla.com:
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
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
Flags: needinfo?(matt.woodrow)
Pushed by mwoodrow@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/54fa8df4ba28
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.