1323539 - Remove PLayer

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 1

[Mason Chang [Inactive] [:mchang]]

This is the best christmas present ever.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, clean up ShadowableLayer

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.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, remove PLayer

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.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, remove PLayer

Cleaned up a bit.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dc3465913259e9fcf079988c64a1df7b7675275

Comment 6

[Matt Woodrow (:mattwoodrow)]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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)

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/da479aa758a8
https://hg.mozilla.org/mozilla-central/rev/11873cde7a59

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/f241b4570f53
Followup for graphics branch to remove PLayer leftover bits. r=me