remove dead CloneManagees and related code

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

Trunk
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Assignee

Description

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

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)
Assignee

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+
Assignee

Comment 4

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

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+
Assignee

Updated

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

Comment 6

3 years ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee21ab5cb479
part 1 - remove CloneManagees/CloneToplevel code from IPDL; r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/67e8b862bdb0
part 2 - remove opened actor tracking from IToplevelProtocol; r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/47f3a6275d66
part 3 - remove gProtocolMutex; r=billm
You need to log in before you can comment on or make changes to this bug.