remove dead CloneManagees and related code

RESOLVED FIXED in Firefox 51



3 years ago
3 years ago


(Reporter: froydnj, Assigned: froydnj)


Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)



(3 attachments, 1 obsolete attachment)



3 years ago
As suggested in bug 1297804 comment 9, all this stuff is dead code, so we should get rid of it.

Comment 1

3 years ago
CloneOpenedToplevels, which is never called, is the only interesting
caller of CloneToplevel.  And CloneToplevel, in turn, is the only
interesting caller of CloneManagees.  Which means we can ditch all this
code for a decent amount of space savings, both in code and writable
static data (no more useless virtual function entries in vtables).

This part is the big win, about 128K on x86-64 Linux.
Attachment #8786901 - Flags: review?(wmccloskey)

Comment 2

3 years ago
The only thing we needed opened actor tracking for was the ability to
clone all the actors.  But now that we no longer have support for
cloning actors, we no longer need to track the actors that we've cloned,
which makes a number of things significantly simpler.

I'm less sure about this patch--whether or not I did it correctly--so I split
this one out as a separate part to make it hopefully easier to review.
Attachment #8786902 - Flags: review?(wmccloskey)
Attachment #8786901 - Flags: review?(wmccloskey) → review+
Comment on attachment 8786902 [details] [diff] [review]
part 2 - remove opened actor tracking from IToplevelProtocol

Review of attachment 8786902 [details] [diff] [review]:

I wish I could give this r++!

::: gfx/layers/ipc/CompositorBridgeParent.cpp
@@ -732,5 @@
>  bool
>  CompositorBridgeParent::Bind(Endpoint<PCompositorBridgeParent>&& aEndpoint)
>  {
> -  if (!aEndpoint.Bind(this, nullptr)) {

I think removing this is incorrect. We just want to get rid of the null parameter.

::: ipc/glue/ProtocolUtils.cpp
@@ +72,5 @@
>  }
>  IToplevelProtocol::~IToplevelProtocol()
>  {
>    StaticMutexAutoLock al(gProtocolMutex);

Let's get rid of this mutex too (the var as well as the acquisition).
Attachment #8786902 - Flags: review?(wmccloskey) → review+

Comment 4

3 years ago
It no longer protects any global state, and is therefore pure overhead.
Attachment #8787003 - Flags: review?(wmccloskey)

Comment 5

3 years ago
With the removed Bind() call put back.  I was removing them before I realized
what they were used for, and clearly forgot to put that one back.  Carrying
over r+.
Attachment #8787004 - Flags: review+


3 years ago
Attachment #8786902 - Attachment is obsolete: true
Attachment #8787003 - Flags: review?(wmccloskey) → review+

Comment 6

3 years ago
Pushed by
part 1 - remove CloneManagees/CloneToplevel code from IPDL; r=billm
part 2 - remove opened actor tracking from IToplevelProtocol; r=billm
part 3 - remove gProtocolMutex; r=billm
You need to log in before you can comment on or make changes to this bug.