Closed Bug 1323539 Opened 3 years ago Closed 3 years ago

Remove PLayer

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(2 files, 1 obsolete file)

PLayer has no actual methods, it just serves as an association between IPC endpoints. It's had a fair share of memory management bugs, and sending lots of constructor messages during layer building is unnecessary.

Instead, we can just send IDs as part of layer transactions, and create a mapping on the compositor side as needed.
This is the best christmas present ever.
I'm not sure why, but ClientLayer implements some internal details of ShadowableLayer. This patch hides ShadowableLayer details and moves some logic out of ClientLayer's destructor.
Attachment #8819221 - Flags: review?(matt.woodrow)
Attached patch part 2, remove PLayer (obsolete) — Splinter Review
This removes PLayer, ShadowLayerChild, and ShadowLayerParent. Instead of making actors, we now simply generate a unique ID. Transaction edit ops reference these IDs instead of actors.

On the LayerTransactionParent side, when we get a "create layer" command, we add a mapping from the given ID to the new layer. Any future op can reference this id. When the client layer is destroyed, we send a message to unmap the id in the compositor, which also destroys the compositor layer.

I don't think this changes any existing semantics, except that we should be sending less messages during layer construction. I'm not attempting to batch destruction messages here.

This is probably easiest to review by starting with the changes to .ipdl files and LayersTypes.h, then ShadowLayerForwarder, ClientLayerManager, and LayerTransactionParent.
Attachment #8819243 - Flags: review?(matt.woodrow)
Cleaned up a bit.
Attachment #8819243 - Attachment is obsolete: true
Attachment #8819243 - Flags: review?(matt.woodrow)
Attachment #8819247 - Flags: review?(matt.woodrow)
Attachment #8819221 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8819247 [details] [diff] [review]
part 2, remove PLayer

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

::: gfx/layers/ipc/LayerTransactionParent.h
@@ +202,5 @@
>    // containers in the "real" layer tree
>    RefPtr<Layer> mRoot;
> +
> +  // Mapping from LayerHandles to Layers.
> +  nsDataHashtable<nsUint64HashKey, RefPtr<Layer>> mLayerMap;

Could use nsRefPtrHashtable if you wanted.
Attachment #8819247 - Flags: review?(matt.woodrow) → review+
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da479aa758a8
Don't access ShadowableLayer from ClientLayer's destructor. (bug 1323539 part 1, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/11873cde7a59
Remove PLayer. (bug 1323539 part 2, r=mattwoodrow)
https://hg.mozilla.org/mozilla-central/rev/da479aa758a8
https://hg.mozilla.org/mozilla-central/rev/11873cde7a59
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/f241b4570f53
Followup for graphics branch to remove PLayer leftover bits. r=me
Depends on: 1387659
You need to log in before you can comment on or make changes to this bug.