1523072 - Update DOM IPC naming for fission (TabParent, TabChild, etc.)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Jonathan Watt [:jwatt]]

Bug 1500257 changes how we use some classes resulting in their names being quite confusing and creating friction for people trying to understand the code. For example, TabChild and TabParent will now be used for iframes of arbitrary depth, not just for a top-level window. I didn't look to closely, but maybe this also applies to PBrowser (there's no comment documenting what PBrowser is or what it's for :-/ so who knows... :-P ).

Comment 1

[Jonathan Watt [:jwatt]]

Nika, do you have any thoughts on which classes should be renamed or what their names should be?

Comment 2

[Nika Layzell [:nika] (ni? for response)]

PBrowser is the name of the underlying protocol for TabParent/Child. I don't know the history as to why the names don't line up, but I'm definitely down with getting them fixed up.

There are a few obvious options:

1. Align with the Tab name, (rename the protocol to PTab). Unfortunately, we're now using this actor not only for tabs but generally for contiguous out-of-process subtrees, making that name perhaps unfortunate. The benefit is that the change will be smaller.
2. Align with the Browser name, (rename the actors to BrowserParent, and BrowserChild). This is pretty good, but also could cause confusion because it's also used for the root browsing contexts in subframes, which may not be clear from the name.
3. Come up with some other name, which I don't have any amazing suggestions for, but perhaps someone does.

ni? bz in case he has thoughts.

Comment 3

[Ryan Hunt [:rhunt] (on leave until early May)]

One suggestion might be to follow chrome with their usage of 'Frame'.

PBrowser -> PFrame
TabParent -> FrameParent
TabChild -> FrameChild

PRemoteFrame -> PSubFrame
RemoteFrameParent -> SubFrameParent
RemoteFrameChild -> SubFrameChild

I also considered 'PRemoteFrame -> PChildFrame' but that has the consequence of 'RemoteFrameChild -> ChildFrameChild' which might be confusing.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I find the use of "Frame" to mean "any window, including toplevel" in Chrome somewhat confusing...

So just so I understand, in the fission world PBrowser/TabParent/TabChild correspond to subsets of the browser context tree that are maximal, connected, and same-process, right?

Does RemoteFrame correspond to basically the roots of such subsets?

Comment 5

[Ryan Hunt [:rhunt] (on leave until early May)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #4)

I find the use of "Frame" to mean "any window, including toplevel" in Chrome
somewhat confusing...

So just so I understand, in the fission world PBrowser/TabParent/TabChild
correspond to subsets of the browser context tree that are maximal,
connected, and same-process, right?

Yes, that's correct.

Does RemoteFrame correspond to basically the roots of such subsets?

PRemoteFrame is the link between parent and child of 'subsets of the browser context tree that are maximal connected, and same-process'.

Comment 6

[Ryan Hunt [:rhunt] (on leave until early May)]

(In reply to Ryan Hunt [:rhunt] from comment #5)

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #4)

I find the use of "Frame" to mean "any window, including toplevel" in Chrome
somewhat confusing...

So just so I understand, in the fission world PBrowser/TabParent/TabChild
correspond to subsets of the browser context tree that are maximal,
connected, and same-process, right?

Yes, that's correct.

Does RemoteFrame correspond to basically the roots of such subsets?

PRemoteFrame is the link between parent and child of 'subsets of the browser
context tree that are maximal connected, and same-process'.

If that's clear at all.. The terminology is a bit confusing to me.

Also I understand the 'Frame' terminology that Chrome uses being confusing. I just don't have a better suggestion.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ah, so PBrowser talks between a content process and the parent process and PRemoteFrame talks between two content processes?

Still trying to tease out the details here... Say I have a toplevel page from site A which loads a subframe from site B which then loads a subframe from site A. The two site A pages have different TabChild instances that they talk to, right?

Comment 8

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: fission-ipc.svg

Ah, so PBrowser talks between a content process and the parent process and PRemoteFrame talks between two content processes?

Still trying to tease out the details here... Say I have a toplevel page from site A which loads a subframe from site B which then loads a subframe from site A. The two site A pages have different TabChild instances that they talk to, right?

If I understand correctly, I believe it would look something like the attached sketch. Nika can correct me if I'm wrong.

Comment 9

[Neha Kochar [:neha]]

Thank you, Ryan for the attachment - I found it very helpful, I'm sure others will do.

Adding ni for Nika, so she can review and respond here.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Yup, that looks pretty accurate in terms of how the links exist between the processes.

Comment 11

[Ryan Hunt [:rhunt] (on leave until early May)]

(In reply to Ryan Hunt [:rhunt] from comment #3)

One suggestion might be to follow chrome with their usage of 'Frame'.

PBrowser -> PFrame
TabParent -> FrameParent
TabChild -> FrameChild

PRemoteFrame -> PSubFrame
RemoteFrameParent -> SubFrameParent
RemoteFrameChild -> SubFrameChild

I also considered 'PRemoteFrame -> PChildFrame' but that has the consequence
of 'RemoteFrameChild -> ChildFrameChild' which might be confusing.

Thinking more, I don't have a problem with using 'Browser' consistently either.

For example,

PBrowser -> PBrowser
TabParent -> BrowserParent
TabChild -> BrowserChild

PRemoteFrame -> PBrowserBridge
RemoteFrameParent -> BrowserBridgeParent
RemoteFrameChild -> BrowserBridgeChild

Comment 12

[Jonathan Watt [:jwatt]]

Does that sound okay, Nika, Boris?

Comment 13

[Nika Layzell [:nika] (ni? for response)]

It sounds OK to me - I'm all for consistency in whatever form it takes :-)

Comment 14

[Boris Zbarsky [:bzbarsky]]

I like the naming proposed in comment 11.

Comment 15

[Ryan Hunt [:rhunt] (on leave until early May)]

Okay, sounds good. I'll work on the patch. I'll probably also roll some documentation into this as well.

Comment 16

[Ryan Hunt [:rhunt] (on leave until early May)]

I'm going to split this up and try to land it in stages.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Yeah, let's not overload Frame even more, or let's rename nsIFrame to LayoutFragment or LayoutBox before? :)

Comment 18

[Neha Kochar [:neha]]

Work was done in bug 1532725 and bug 1534395.

Comment 19

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: updated-fission-ipc.svg

I've updated the diagram of Fission IPC.

Comment 20

[Bobby Holley (:bholley)]

This diagram is super useful. Can you link to it from the code comments for the various pieces?

Comment 21

[Ryan Hunt [:rhunt] (on leave until early May)]

Good idea. I'm going to try to add it to the in-tree documentation in bug 1547812.