792652 - IPDL: Refactor the division of code that's generated and code that's manually written

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We currently autogenerate a lot of code, e.g. for managing listener maps, that seems like it could be manually written in C++.  Manually-written code is considerably easier to maintain.

There were reasons the division was done like it was at the time, but these are worth revisiting every few years with better hindsight.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Get rid of CloneProtocol

CloneProtocol is leftover code from Nuwa. We can remove it now.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Simplify IPDL type hierarchy

Currently all our protocols inherit from IProtocolManager<IProtocol>. I have
no idea why. This patch switches everything over to IProtocol, without any
templates. I had to move ReadActor to the .cpp file to avoid redefinition
errors.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: IToplevelProtocol refactoring

Currently toplevel protocols inherit from both IProtocolManager<IProtocol>
and IToplevelProtocol. This change makes IToplevelProtocol inherit from
IProtocol, so now toplevel protocols only inherit from
IToplevelProtocol.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Remove methods from MessageListener

With this change, MessageChannel stores mListener as an IToplevelProtocol
rather than a MessageListener (which isn't really a useful concept on
its own). The MessageListener methods are split out to IProtocol and
IToplevelProtocol. MessageListener gets deleted. Some of the inline
functions in MessageChannel had to be moved to MessageChannel.cpp since
IToplevelProtocol isn't defined in MessageChannel.h.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Store Manager() in IProtocol

This patch stores mManager in IProtocol rather than in each individual
PFoo. It also adds a generic accessor for that field. Note that each
individual protocol still defines a Manager() function that returns
PFooParent or whatever. I tried to get rid of that but it was a lot
of work.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Stop generating some simple sub-protocol methods

This moves some of the generated methods in subprotocols that simply defer
to the parent protocol to IProtocol. These methods are still overridden in
the toplevel protocol.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Move mChannel to IProtocol

This moves the mChannel field to IProtocol. The toplevel protocol still keeps
its own mChannel field that's a MessageChannel (no pointer).

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Stop generating code for toplevel-only methods in non-toplevel protocols

Moves OnProcessingError, OnChannelError, OnChannelConnected so that they're
only generated for toplevel protocols. For some reason APZCTreeManagerChild
implemented OnProcessingError. I'm not sure what the intention was there so
I removed it.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Add mSide to IProtocol

This patch adds a field to hold ParentSide/ChildSide state. Useful for
later changes.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Move FatalError to IProtocol

This patch moves FatalError to IProtocol. I had to make a few changes.
- I added a ProtocolName() method to find the name of the protocol.
- I gave the two-argument version of FatalError its own name. Otherwise
  C++ doesn't like there to be two virtual methods with the same name
  in cases where one is overridden and the other isn't (as happens
  in IToplevelProtocol).

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Move shmem methods to ProtocolUtils

This patch moves some shmem management code into IProtocol. Someday
maybe we can get rid of IHadBetterBeIPDLCodeCallingThis_OtherwiseIAmADoodyhead.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Remove pointless OnFoo methods

A bunch of these methods just delegate from OnFoo to Foo. Not
sure why we have OnFoo.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Move over toplevel process ID field

Moves some state to IToplevelProtocol.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Move some channel methods to IToplevelProtocol

Move Open, Close, SetReplyTimeoutMs to IToplevelProtocol.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Move TakeMinidump to IToplevelProtocol

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Move toplevel actor map to IToplevelProtocol

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Move over toplevel shmem code

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Move mId to toplevel protocol

Comment 19

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

Comment on attachment 8807317 [details] [diff] [review]
Get rid of CloneProtocol

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

Yay.

Comment 20

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

Comment on attachment 8807324 [details] [diff] [review]
Stop generating code for toplevel-only methods in non-toplevel protocols

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

::: gfx/layers/ipc/APZCTreeManagerChild.cpp
@@ -262,5 @@
>    return true;
>  }
>  
> -void
> -APZCTreeManagerChild::OnProcessingError(

Maybe this was a top-level protocol at some point, but yes: makes sense to delete it now.

Comment 21

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

Comment on attachment 8807327 [details] [diff] [review]
Move shmem methods to ProtocolUtils

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

Sweet. This should simplify some stuff in Graphics.

Comment 22

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

Comment on attachment 8807330 [details] [diff] [review]
Move some channel methods to IToplevelProtocol

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

Do we need all the SetOtherProcessId calls around gecko anymore?

Comment 23

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

Comment on attachment 8807334 [details] [diff] [review]
Move over toplevel shmem code

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

Amazing all this stuff was shoved in lower.py, heh.

Comment 24

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yeah, I realized that we can now eliminate IShmemAllocator. That thing makes absolutely no sense.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92e7fee81fa2
Get rid of CloneProtocol (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/004cd692ba6d
Simplify IPDL type hierarchy (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1829d5358808
IToplevelProtocol refactoring (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbbe3a8c00e7
Remove methods from MessageListener (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a1e3136323a
Store Manager() in IProtocol (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3e496eab991
Stop generating some simple sub-protocol methods (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/63a3c8e4016e
Move mChannel to IProtocol (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8edf4b247250
Stop generating code for toplevel-only methods in non-toplevel protocols (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdf86b8b9c43
Add mSide to IProtocol (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/59e6d0a4f35b
Move FatalError to IProtocol (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf9c4164eb43
Move shmem methods to ProtocolUtils (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/94fcd3bf3f34
Remove pointless OnFoo methods (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ba36b8ac60c
Move over toplevel process ID field (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b658ebaad5d7
Move some channel methods to IToplevelProtocol (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/75855b5a9ab9
Move TakeMinidump to IToplevelProtocol (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/95eff6c45cae
Move toplevel actor map to IToplevelProtocol (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddd915ab4a48
Move over toplevel shmem code (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/90eaf6aec002
Move mId to toplevel protocol (r=dvander)

Comment 26

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

I had to back these out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=38836252&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2098e1d1f2d

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a1013533726
Get rid of CloneProtocol (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ad0b451ee01
Simplify IPDL type hierarchy (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/732e9da2ae25
IToplevelProtocol refactoring (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6209f046a283
Remove methods from MessageListener (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6642de5fb7a
Store Manager() in IProtocol (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b4e8fdc3b83
Stop generating some simple sub-protocol methods (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5435dcdac909
Move mChannel to IProtocol (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fe6dbb18a96
Stop generating code for toplevel-only methods in non-toplevel protocols (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/905828d9d423
Add mSide to IProtocol (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c07bc3a61d8
Move FatalError to IProtocol (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/93a3d7897772
Move shmem methods to ProtocolUtils (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6a0511d3d11
Remove pointless OnFoo methods (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/2abbc39dc74e
Move over toplevel process ID field (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2236f71a3ea
Move some channel methods to IToplevelProtocol (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/56e30c8c735b
Move TakeMinidump to IToplevelProtocol (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6feaf5672e0
Move toplevel actor map to IToplevelProtocol (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/85c13e8c3b61
Move over toplevel shmem code (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba1901eafef5
Move mId to toplevel protocol (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fb5ed600895
IPDL refactoring may need a clobber

Comment 28

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I did a bunch of retriggers of that build here:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=90eaf6aec002136b753bb7f921cce5141dcd0f12

It never failed again. I suspect there may have been an issue where we need to clobber. I could be wrong, but it seems like a possibility. I'm not sure what else to do since I can't reproduce the issue anywhere. Anyway, I relanded.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3a1013533726
https://hg.mozilla.org/mozilla-central/rev/7ad0b451ee01
https://hg.mozilla.org/mozilla-central/rev/732e9da2ae25
https://hg.mozilla.org/mozilla-central/rev/6209f046a283
https://hg.mozilla.org/mozilla-central/rev/f6642de5fb7a
https://hg.mozilla.org/mozilla-central/rev/1b4e8fdc3b83
https://hg.mozilla.org/mozilla-central/rev/5435dcdac909
https://hg.mozilla.org/mozilla-central/rev/7fe6dbb18a96
https://hg.mozilla.org/mozilla-central/rev/905828d9d423
https://hg.mozilla.org/mozilla-central/rev/1c07bc3a61d8
https://hg.mozilla.org/mozilla-central/rev/93a3d7897772
https://hg.mozilla.org/mozilla-central/rev/e6a0511d3d11
https://hg.mozilla.org/mozilla-central/rev/2abbc39dc74e
https://hg.mozilla.org/mozilla-central/rev/c2236f71a3ea
https://hg.mozilla.org/mozilla-central/rev/56e30c8c735b
https://hg.mozilla.org/mozilla-central/rev/b6feaf5672e0
https://hg.mozilla.org/mozilla-central/rev/85c13e8c3b61
https://hg.mozilla.org/mozilla-central/rev/ba1901eafef5
https://hg.mozilla.org/mozilla-central/rev/7fb5ed600895