Update DOM IPC naming for fission (TabParent, TabChild, etc.)
Categories
(Core :: DOM: Core & HTML, enhancement, P3)
Tracking
()
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 ).
Updated•6 years ago
|
Reporter | ||
Comment 1•6 years ago
|
||
Nika, do you have any thoughts on which classes should be renamed or what their names should be?
Comment 2•6 years ago
|
||
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:
- Align with the
Tab
name, (rename the protocol toPTab
). 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. - Align with the
Browser
name, (rename the actors toBrowserParent
, andBrowserChild
). 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. - 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.
Assignee | ||
Comment 3•6 years ago
|
||
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•6 years ago
|
||
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?
Assignee | ||
Comment 5•6 years ago
|
||
(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'.
Assignee | ||
Comment 6•6 years ago
|
||
(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•6 years ago
|
||
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?
Assignee | ||
Comment 8•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
Yup, that looks pretty accurate in terms of how the links exist between the processes.
Assignee | ||
Comment 11•6 years ago
|
||
(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 -> FrameChildPRemoteFrame -> PSubFrame
RemoteFrameParent -> SubFrameParent
RemoteFrameChild -> SubFrameChildI 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
Reporter | ||
Comment 12•6 years ago
|
||
Does that sound okay, Nika, Boris?
Comment 13•6 years ago
|
||
It sounds OK to me - I'm all for consistency in whatever form it takes :-)
Assignee | ||
Comment 15•6 years ago
|
||
Okay, sounds good. I'll work on the patch. I'll probably also roll some documentation into this as well.
Updated•6 years ago
|
Assignee | ||
Comment 16•6 years ago
|
||
I'm going to split this up and try to land it in stages.
Assignee | ||
Updated•6 years ago
|
Updated•6 years ago
|
Comment 17•6 years ago
|
||
Yeah, let's not overload Frame even more, or let's rename nsIFrame to LayoutFragment or LayoutBox before? :)
Updated•6 years ago
|
Comment 18•6 years ago
|
||
Work was done in bug 1532725 and bug 1534395.
Assignee | ||
Comment 19•6 years ago
|
||
I've updated the diagram of Fission IPC.
Comment 20•6 years ago
|
||
This diagram is super useful. Can you link to it from the code comments for the various pieces?
Assignee | ||
Comment 21•6 years ago
|
||
Good idea. I'm going to try to add it to the in-tree documentation in bug 1547812.
Assignee | ||
Updated•6 years ago
|
Description
•