Closed Bug 1523072 Opened Last year Closed 9 months ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
Fission Milestone M2

People

(Reporter: jwatt, Assigned: rhunt)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

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

Priority: -- → P3

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

Flags: needinfo?(nika)

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.

Flags: needinfo?(nika) → needinfo?(bzbarsky)

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.

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?

Flags: needinfo?(bzbarsky)

(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'.

(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.

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?

Attached image 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.

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.

Flags: needinfo?(nika)

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

Flags: needinfo?(nika)

(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

Does that sound okay, Nika, Boris?

Flags: needinfo?(nika)
Flags: needinfo?(bzbarsky)

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

Flags: needinfo?(nika)

I like the naming proposed in comment 11.

Flags: needinfo?(bzbarsky)

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

Assignee: nobody → rhunt
Fission Milestone: --- → M2
Depends on: 1532725

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

Summary: Rename TabChild, TabParent, etc. for OOP frames → Update DOM IPC naming for fission (TabParent, TabChild, etc.)
Depends on: 1534395
Component: DOM → DOM: Core & HTML

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

Status: NEW → ASSIGNED

Work was done in bug 1532725 and bug 1534395.

Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Attached image updated-fission-ipc.svg

I've updated the diagram of Fission IPC.

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

Flags: needinfo?(rhunt)
Depends on: 1547812

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

Flags: needinfo?(rhunt)
Depends on: 1555789
You need to log in before you can comment on or make changes to this bug.