FrameLayerBuilder::GetPaintedLayerScaleForFrame calls nsLayoutUtils::GetTransformToAncestor, passing the root presContext's root frame. Once we have OOP iframes that will need to take account of transforms above any OOP iframe, otherwise we could either waste memory rendering layers at too high a resolution, or else render them at too low a resolution and end up pixelated.

Boris was interested in taking a look at this. Assigning for now.

I think this should go something like this:

• When the transform from an embedding element (e.g. iframe) to its docs
  root element changes, send a message to the chrome process using
  RemoteFrameChild notifying it of the new transform

• The RemoteFrameParent should loop over all TabParents under it (i.e.
  the tree of processes for oop-iframes) notifying them of the change
  in transform to screen space.

• The TabChild in each notified process should store the transform
  to screen space, and FrameLayerBuilder::GetPaintedLayerScaleForFrame
  calls should use that transform.

Does that sound about right, Ryan? Are there any complications that you can think of that I'm missing?

Forgot the needinfo.

Boris, you'll probably find this diagram useful for understanding the relationship between objects in the different processes:

https://bug1523072.bmoattachments.org/attachment.cgi?id=9046000

We should fix this for M2 I think.

(In reply to Jonathan Watt [:jwatt] from comment #2)

I think this should go something like this:

• When the transform from an embedding element (e.g. iframe) to its docs
  root element changes, send a message to the chrome process using
  RemoteFrameChild notifying it of the new transform

• The RemoteFrameParent should loop over all TabParents under it (i.e.
  the tree of processes for oop-iframes) notifying them of the change
  in transform to screen space.

• The TabChild in each notified process should store the transform
  to screen space, and FrameLayerBuilder::GetPaintedLayerScaleForFrame
  calls should use that transform.

Does that sound about right, Ryan? Are there any complications that you can
think of that I'm missing?

Yes, that sounds right to me. There might be some complications with animations and such, but I think that's a good start.

(In reply to Jonathan Watt [:jwatt] from comment #4)

Boris, you'll probably find this diagram useful for understanding the relationship between objects in the different processes:

https://bug1523072.bmoattachments.org/attachment.cgi?id=9046000

Note for myself, renaming:
(Bug 1523072)
PBrowser -> PBrowser
TabParent -> BrowserParent
TabChild -> BrowserChild

(Bug 1532725)
PRemoteFrame -> PBrowserBridge
RemoteFrameParent -> BrowserBridgeParent
RemoteFrameChild -> BrowserBridgeChild

Hi Jonathan,

This bug may change how we get the transforms in frame layer builder and let us send IPC messages via PBrowserBridge and PBrowser. Do we have a suitable preference to protect this?

(In reply to Boris Chiou [:boris] from comment #8)

Hi Jonathan,

This bug may change how we get the transforms in frame layer builder and let us send IPC messages via PBrowserBridge and PBrowser. Do we have a suitable preference to protect this?

I guess I can just use this pref, fission.oopif.attribute, and check if BrowserBridgeChild exists on the sender because it looks like we use [1] as the condition to create BrowserBridgeChild if we enable oop-iframe.

[1] https://searchfox.org/mozilla-central/rev/7c20ad925005fbad7b8e08813115f1ec7fa1c248/dom/base/nsFrameLoader.cpp#1864-1869

Hi Cameron,

I have a question about when to update the frame transform for this step in this bug:

When the transform from an embedding element (e.g. iframe) to its docs root element changes, send a message to the chrome process using BrowserBridgeChild notifying it of the new transform

It seems we have to send the transform update to chrome process when:

1. First paint
2. Any style change on transform or geometric updates.
3. Reflow?

Basically, this frame transform is used for painting, after we calculating the frame tree. At first, I thought maybe somewhere in RestyleManager, but I'm not familiar with this, and the timing may not be correct. Therefore, I may need your suggestions where are the suitable functions to send the IPC messages (i.e. update the frame transform in TabChild). Thanks.

I don't think we need to do it for any geometry updates or reflow, just transforms, since that's all that FrameLayerBuilder::GetPaintedLayerScaleForFrame looks at.

For responding to a change in the specified transform on an element, you can do it somewhere under RestyleManager::ProcessRestyledFrames. We have existing change hints that get generated when transforms are created or changed (nsChangeHint_UpdateTransformLayer and nsChangeHint_AddOrRemoveTransform), but both are hints in the "not handled for descendants" category (nsChangeHint_Hints_NeverHandledForDescendants). This means that if we have

  <div>
    <div>
      <iframe>

and we change the transform on both divs, then we'll generate those hints for both of them.

When we detect a transform change on an element, we'll need to search for all of the descendant <iframe>s to recompute their transform. If we do that, then we want to make sure that if we generate the same hint on a descendant element that we don't process it again (since we will have already searched through this subtree). So I think that means we should add a new change hint to do this work, and add it to the "is handled for descendants" category. That will ensure it's automatically skipped if we generated it on an ancestor.

We do this kind of traversal down the tree to apply a change hint in SyncViewsAndInvalidateDescendants.

I'm not sure what implications transform animations have here.

One I'm wondering about too: should this new hint be in nsChangeHint_Hints_CanIgnoreIfNotVisible or not?

(In reply to Boris Chiou [:boris] from comment #9)

(In reply to Boris Chiou [:boris] from comment #8)

Hi Jonathan,

This bug may change how we get the transforms in frame layer builder and let us send IPC messages via PBrowserBridge and PBrowser. Do we have a suitable preference to protect this?

I guess I can just use this pref, fission.oopif.attribute, and check if BrowserBridgeChild exists on the sender because it looks like we use [1] as the condition to create BrowserBridgeChild if we enable oop-iframe.

[1] https://searchfox.org/mozilla-central/rev/7c20ad925005fbad7b8e08813115f1ec7fa1c248/dom/base/nsFrameLoader.cpp#1864-1869

I wonder whether we need to protect the behavior by a pref. For bug 1518919, what I have in my mind is that each pres shell has the propagated visibility information regardless of OOPIF and use the value just like cache instead of walking up document tree even if it's not in OOPIF.

(In reply to Cameron McCormack (:heycam) from comment #13)

One I'm wondering about too: should this new hint be in nsChangeHint_Hints_CanIgnoreIfNotVisible or not?

Yes.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)

I wonder whether we need to protect the behavior by a pref. For bug 1518919, what I have in my mind is that each pres shell has the propagated visibility information regardless of OOPIF and use the value just like cache instead of walking up document tree even if it's not in OOPIF.

Actually, I'm not sure about the propagated visibility information. :)

In my current patch, I don't use this pref. Instead, I check BrowserBridgeChild. If it is an nullptr, that means we are not using oop-iframe, so don't need to update the transform in TabChild, and keep this transform as an identity matrix. Just want to avoid unnecessary IPC messages.

Should we be sharing this information as part of painting, rather than a style change? I think we also need to share scale factors that originate from zooming, and DPI scaling, not just CSS transforms.

The scaling here just needs to be best-effort, as it just affects memory usage and rasterization quality, but doesn't affect correctness.

When we try to paint a remote document (building a placeholder Layer or WebRender display item for the OOP content) should be an easy time to determine what the scale factors to the root are (since we compute them already) and we can share them to the child at that point (probably only if they changed). This process will naturally be recursive for nested OOP <iframes>.

I think we should also consider a different (but related) problem, a page with an <iframe height="10000px">. For this page we almost certainly want to let the OOP iframe know that it only needs to paint the currently visible subset, rather than trying to paint 10k high Layers. This visible subset can change, mainly due to scrolling the outer document, but possibly also in other ways.

We also probably want to drop Layers for an OOP iframe when it gets scrolled out of view.

For this case, again, the time when we have the best understanding on what painting is required for the <iframe> is when we're painting the outer.

Combining both these requirements in a single message that we send each time we paint the outer seems like the easiest way forward.

For non-WebRender, the scale factors are currently handled here - https://searchfox.org/mozilla-central/rev/ddd1679c0534f7ddf36cafddd17b710c4fefe3c4/layout/ipc/RenderFrame.cpp#246

In that code we're just setting the scale on the Layer, since we currently have no way to ask the remote document to take the scale into account.

If we added a call to notify the remote document of these scale factors, then the expected behaviour is that the remote document would draw at the suggested scale, but also put an inverse scale transform on the root. That way the final rendering of the remote document is still 1.0, but if it still matches the scale we set in RenderFrame, then the two cancel out, and we composite without scaling. If there's a race, and the two no longer match, then things still render correct.

That code doesn't currently have the visible rect, but the caller (FrameLayerBuilder) does, and we could pass that into BuildLayer.

The WebRender equivalent is here - https://searchfox.org/mozilla-central/rev/ddd1679c0534f7ddf36cafddd17b710c4fefe3c4/layout/ipc/RenderFrame.cpp#279

StackingContextHelper::aSc has GetInheritedScale for finding the scale factors.

The FrameLayerBuilder code also has handling for animations [1] such that it tries to keep the scale factors passed down constant during an animation to prevent constant re-rasterization.

Using the same code paths mean we also get that behaviour, which I suspect is what we want.

[1] https://searchfox.org/mozilla-central/rev/ddd1679c0534f7ddf36cafddd17b710c4fefe3c4/layout/painting/FrameLayerBuilder.cpp#6031

Hi, rhunt,

I'm trying to share the CSS transform information with both oop-iframe and non-oop iframe in the same document. For example:

<div id="parent">
  <iframe fission id="iframe-oop" src="remote.html"></iframe>
  <iframe id="iframe-non-oop" src="remote2.html"></iframe>
</div>

When I have a transform style change on "parent" div, we will propagate the transform update to both iframes. For "iframe-oop", it's very simple, I checked if it is an oop-iframe, and it is, so we follow the steps in comment 2 (i.e. send the IPC message via BrowserBridge and store the updated transform in remote TabChild).

However, I'm not sure how to update the transform for "iframe-non-oop" element. It is not an oop-iframe, so we don't have BrowserBridge. I still want to update the transform and go through all the iframes inside its subtree. My question is: For non-oop iframe, there is no TabChild, right? That means we should not store the transform information in TabChild. Besides, I tried to get the nsIPresShell or nsIPresContext inside this non-oop iframe by its browsing context, but unfortunately, I got null values. Therefore, I have no idea how to update the transform for the non-oop iframe. Not sure which way or which function should I use to share the CSS transform. Could you please give me some hints? Thanks.

(In reply to Matt Woodrow (:mattwoodrow) from comment #17)

Should we be sharing this information as part of painting, rather than a style change? I think we also need to share scale factors that originate from zooming, and DPI scaling, not just CSS transforms.

It seems rhunt has a patch to address this in Bug 1525720. :)

(In reply to Boris Chiou [:boris] from comment #21)

(In reply to Matt Woodrow (:mattwoodrow) from comment #17)

Should we be sharing this information as part of painting, rather than a style change? I think we also need to share scale factors that originate from zooming, and DPI scaling, not just CSS transforms.

It seems rhunt has a patch to address this in Bug 1525720. :)

I'm sorry, I intended to sync up with you yesterday but got caught up traveling for the graphics work week.

I think this bug (bug 1519546), bug 1518919, bug 1518917, and bug 1525720 can all be solved in the same way.

Roughly all of these bugs are about communicating the following information about oop-iframes:

1. Are they visible or not? Either by visibility, being clipped, scrolled out of view, or being tabbed away.
2. Which area of the iframe is visible? By being clipped or scrolled partially out of view.
3. What resolution should they render at? CSS transform and zooming

Matt's point is that we can get all of this information after painting happens by looking at the layer tree (or webrender display list). Each OOP-iframe is represented as an RefLayer (or IframeDisplayItem for webrender). I'll stick with RefLayer's for now.

We can determine (1) by looking to see which LayerID's are referenced via RefLayers in a layer tree. If a LayerID of a OOP-iframe is present in a layer tree, then it's visible. If not, it isn't.

We can determine (2) by looking at the visible region attached to the RefLayers in the layer tree.

We can determine (3) by combining ancestor scales of each RefLayer.

I wrote a patch before the work week that implements some of this. In order to not duplicate effort, I think it makes sense to just have one fix for all three features.

We can probably keep all of these bugs open for now, but I intend to pick up work on this again in bug 1525720 now.

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

(In reply to Boris Chiou [:boris] from comment #21)

(In reply to Matt Woodrow (:mattwoodrow) from comment #17)

Should we be sharing this information as part of painting, rather than a style change? I think we also need to share scale factors that originate from zooming, and DPI scaling, not just CSS transforms.

It seems rhunt has a patch to address this in Bug 1525720. :)

I'm sorry, I intended to sync up with you yesterday but got caught up
traveling for the graphics work week.

I think this bug (bug 1519546), bug 1518919, bug 1518917, and bug 1525720
can all be solved in the same way.

Roughly all of these bugs are about communicating the following information
about oop-iframes:

1. Are they visible or not? Either by visibility, being clipped,
   scrolled out of view, or being tabbed away.
2. Which area of the iframe is visible? By being clipped or scrolled
   partially out of view.
3. What resolution should they render at? CSS transform and zooming

Matt's point is that we can get all of this information after painting
happens by looking at the layer tree (or webrender display list). Each
OOP-iframe is represented as an RefLayer (or IframeDisplayItem for
webrender). I'll stick with RefLayer's for now.

We can determine (1) by looking to see which LayerID's are referenced via
RefLayers in a layer tree. If a LayerID of a OOP-iframe is present in a
layer tree, then it's visible. If not, it isn't.

We can determine (2) by looking at the visible region attached to the
RefLayers in the layer tree.

We can determine (3) by combining ancestor scales of each RefLayer.

I wrote a patch before the work week that implements some of this. In order
to not duplicate effort, I think it makes sense to just have one fix for all
three features.

We can probably keep all of these bugs open for now, but I intend to pick up
work on this again in bug 1525720 now.

Actually now that I look at bug 1518919 further, it seems that there are places in the code base that care specifically about the CSS visibility attribute and not about generally whether the OOP-iframe is being presented on the screen or not. That's going to require a different solution.

I see. Thanks for this. For this bug, let's wait for the implementation of bug 1525720. :)

As there's a lot of overlap with this bug and bug 1518917, I'm going to combine them into one.

This is currently blocked on the tab switching work, as there are some infrastructure changes I was hoping we could use. I've looked into it more and I think we can actually work around this, so I'm going to unblock this bug.

And because I have a prototype, I'm going to try to take the bug and try to finish it.

This commits adds EffectsInfo for tracking graphical effects
that are applied to a remote browser's ancestors, and
EffectsInfoUpdate for updating child remote browsers
EffectsInfo's. These will be used in following commits.

This commit adds an IPDL method for updating a remote browser's
EffectsInfo, and an IPDL method for updating the EffectsInfo
of all remote child browsers of a remote browser.

A following commit will actually use the EffectsInfo for
enabling/disabling rendering for a remote browser, and another
commit will actually use these IPDL methods.

Depends on D31471

Currently, BrowserChild rendering is enabled and disabled by RecvRenderLayers. This
method is called by the tab switching code. This commit factors out the methods to
enable/disable rendering and makes the determination to become visible or hidden
depend on the RecvRenderLayers method and the current EffectsInfo.

Depends on D31472

The default state of a BrowserChild seems to be 'visible' even if it has
never received the RecvRenderLayers message. This can confuse the
logic added in the previous commit, so this commit defaults OOP-iframes
to be rendering. In a future commit, I'd like to clean up this logic.

Depends on D31473

This commit adds some plumbing so that layer managers can communicate
EffectsInfoUpdate's for a paint to the BrowserChild that embeds them.

Depends on D31474

This commit adds layers support for gathering EffectsInfoUpdates by
traversing the layer tree after a paint.

Depends on D31475

This commit adds two small fixes to make the OOP-iframe code more robust.

The first fixes a crash during shutdown for a tab that has OOP-iframes. It's
possible for the BrowserBridgeParent's to be shutdown before the root
BrowserParent. In this case, SetOwnerElement will run for the root BrowserParent,
but the child BrowserBridgeParent's will not have BrowserParent actors.

The second hooks up the BrowserBridgeParent constructor to actually use
the result code of BrowserBridgeParent::Init.

Depends on D31476

I'm posting my current progress, and would like to get this reviewed (and potentially landed) to avoid the situation in bug 1525720 where it's 17 patches at once.

Currently the patches add a basic framework for transferring graphical effects between OOP-iframes, but only supports 'visibility' and doesn't support webrender.

You can test this out on eqrion.github.io/web-tests/fission/activation.html with fission.oopif.attribute=true. As you scroll iframes into and out of the display port, they will activate/deactivate rendering.

This commit adds a list of EffectsInfoUpdate's to nsDisplayListBuilder that
are accumulated during a paint by nsDisplayRemote::{BuildLayer,
CreateWebrenderCommands}.

After a paint has completed we send the list through to the BrowserParent.

Depends on D31475

WebRender support will be finished in the following commit.

Depends on D31473

The nsDisplayListBuilder bits were added in the previous commit.

Depends on D32476

I've reworked the patch stack here again to use an approach that Matt Woodrow suggested. The code is more straightforward and should work in the parent process and the child process now.

The previous patches also had an in issue where an OOP-iframe would only be disabled when it was scrolled into view and then out of view, which is now fixed.

The root PBrowser actor needs special case visibility behavior to satisfy the async tab
switcher. This commit adds a flag to track whether a BrowserChild is part of the root
actor.

Depends on D31472

Backed out 9 changesets (bug 1519546)for causing build bustages on nsLayoutUtils.cpp and browser chrome failures on nsLayoutUtils.cpp

Backout revision https://hg.mozilla.org/integration/mozilla-inbound/rev/8a7d9dfad47ecac0167c162676624acb224410e4

Failed pushes https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=bbcfdcc12774c1b8d78c6420614141382fed3d40
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=249476581&revision=04b0e8a22ee2e0bb67c6cff81b7a9cf312df9e2f

Failure logs - build bustages https://treeherder.mozilla.org/logviewer.html#?job_id=249449957&repo=mozilla-inbound
- browser chrome failures https://treeherder.mozilla.org/logviewer.html#?job_id=249476581&repo=mozilla-inbound

Ryan can you please take a look?

The problem was a missing constructor was causing some fields to be uninitialized. Thankfully ASAN caught this.

Here's a full try run. [1]

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f4b8a4c48260a29c388c980d01da6f390889ae2

https://hg.mozilla.org/mozilla-central/rev/2ced46502b55
https://hg.mozilla.org/mozilla-central/rev/1ba5519d5970
https://hg.mozilla.org/mozilla-central/rev/b186cb12283e
https://hg.mozilla.org/mozilla-central/rev/4658bf615f4b
https://hg.mozilla.org/mozilla-central/rev/7eac769c7c13
https://hg.mozilla.org/mozilla-central/rev/4df5fa6fa785
https://hg.mozilla.org/mozilla-central/rev/b81cf35a573e
https://hg.mozilla.org/mozilla-central/rev/ac0ad5d8e9a5