Electrolysis: Hook TabChild's shadow layer tree into its frame or widget in the browser process

RESOLVED FIXED

Status

()

Core
Graphics
--
enhancement
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b1+)

Details

Attachments

(16 attachments, 13 obsolete attachments)

2.75 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.89 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.43 KB, patch
Details | Diff | Splinter Review
2.32 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.68 KB, patch
smaug
: review+
Benjamin Smedberg
: superreview+
Details | Diff | Splinter Review
11.76 KB, patch
vlad
: review+
Details | Diff | Splinter Review
3.03 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.75 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.50 KB, patch
roc
: review+
Details | Diff | Splinter Review
8.76 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
18.05 KB, patch
Benjamin Smedberg
: superreview+
Details | Diff | Splinter Review
14.27 KB, patch
mats
: review+
Details | Diff | Splinter Review
1.41 KB, patch
Details | Diff | Splinter Review
20.41 KB, patch
vlad
: review+
Details | Diff | Splinter Review
15.64 KB, patch
mats
: review+
Details | Diff | Splinter Review
3.99 KB, patch
Details | Diff | Splinter Review
This allows us to composite content trees in the browser (or compositor) process just as browser trees are.  This work requires widget invalidations/scrolls to be asynchronously forwarded from browser-->content.  Or alternately, the content process could directly register for invalidation events on platforms where that's supported, and only paint to retained buffers + publish an update to the browser process upon invalidation.
Assignee: nobody → jones.chris.g
According to the mobile guys, the fennec frontend forwards widget events already, so we may just need to hook content layer trees into a frame.
Summary: Electrolysis: Hook TabChild's shadow layer tree into its widget in the browser process → Electrolysis: Hook TabChild's shadow layer tree into its frame or widget in the browser process
The basic plan of attack here will be

 - Teach LayerManager to draw retained ThebesLayer content without requiring a gfxContext target

 - Add a PRenderFrame (name TBD) protocol that models a top-level document frame ("frame" in the layout/ sense, not HTML/XUL sense) in the child process.  Layer subtrees will be attached to these guys.

 - Store the parent-side object corresponding to PRenderFrame into nsFrameLoader in the browser process, keyed somehow

 - Add a special case to nsSubDocumentFrame::BuildDisplayList() that creates an nsRemoteDisplayItem (name TBD) referencing the parent-side PRenderFrame recovered from its nsFrameLoader.  This is more complicated than it seemingly needs to be because the layout frame lifetimes in browser/content processes are not coupled in any way; different subdoc frames might consume the same shadow layer subtree and shadow layer subtrees from different child-side frames might be used by the same subdoc frame (that will be the common case)

 - Return the shadow layer subtree from nsRemoteDisplayItem::BuildLayer, to be rendered just like in-process layers by gfx/layers code
Created attachment 457446 [details] [diff] [review]
Very rough WIP

This patch should give an idea of the general approach.  There are lots of problems, but it's working a very basic way.  Just looking for high-level feedback of the form "approach looks OK" or "this is completely 'tarded", don't expect a close read.
Attachment #457446 - Flags: feedback?(tnikkel)
Created attachment 458580 [details] [diff] [review]
Still rough WIP, some bugs fixed

Same approach.
Attachment #457446 - Attachment is obsolete: true
Attachment #458580 - Flags: feedback?(tnikkel)
Attachment #457446 - Flags: feedback?(tnikkel)
In nsFrameFrame.cpp, could you please put |using mozilla::dom::RenderFrameParent;| after all includes? Also, in nsIPresShell.h, s/namespace layers{/namespace layers {/ (note the space), and please use 2-space indent in TabChild.cpp
Created attachment 459929 [details] [diff] [review]
Gut platform widgets from content processes
Attachment #459929 - Flags: superreview?
Created attachment 459931 [details] [diff] [review]
Implement a "puppet widget" stand-in for platform widgets, for content processes

With the previous two patches, we reach something of a crossroads wrt fennec 2.0a1.  Fennec *renders* just fine with these patches, but focus and key events are totally broken.  With them, Oleg and I also have cross-process layers rendering for <browser>s (keys and focus similarly borked there).

I know nothing about how key events or focus are supposed to work, so fixing them may be easy or not.  If it's easy, I would recommend taking these patches for 2.0a1.  We'll save needless rendering to a widget we just draw a canvas over, and we can continue to work on cross-process layers for <browser> on m-c without disturbing 2.0a1.  If fixing focus/keys are hard, these patches will have to land on a project branch (not e10s) along with cross-process layers, to be merged back in after 2.0a1 branches/is released.

Can someone knowledgeable comment here?  I'll post to m.d.t.dom and .p.mobile too.
Attachment #459931 - Flags: superreview?(roc)
Attachment #459929 - Flags: superreview? → superreview?(benjamin)
Created attachment 460090 [details] [diff] [review]
Implement a "puppet widget" stand-in for platform widgets, for content processes, v2

Same as before, but with a two-widget, one-level "hierarchy" so that resizes etc. are forwarded properly.  This fixes resizing test-ipc with remote layers.
Attachment #460090 - Flags: superreview?(roc)
Attachment #459931 - Attachment is obsolete: true
Attachment #459931 - Flags: superreview?(roc)
(In reply to comment #8)
> Same as before, but with a two-widget, one-level "hierarchy" so that resizes
> etc. are forwarded properly.

What do you mean by this, exactly?
I see two assertions with these patches:

###!!! ASSERTION: aChild already in the tree: '!aChild->GetParent()', file gfx/layers/basic/BasicLayers.cpp, line 167

and more often:
###!!! ASSERTION: aChild not our child: 'aChild->GetParent() == this', file gfx/layers/basic/BasicLayers.cpp, line 210
(In reply to comment #9)
> (In reply to comment #8)
> > Same as before, but with a two-widget, one-level "hierarchy" so that resizes
> > etc. are forwarded properly.
> 
> What do you mean by this, exactly?

The view/widget code I've encountered while working on this bug relies on getting the top-level widget's native window ID, creating another nsIWidget for that ID, and having that new widget receive platform resize, paint, etc. notifications.  (That's reasonable, I'm not judging.)  But, fake widgets break that assumption.  So instead, I have the fake widget backend implement the dumbest possible widget hierarchy, hard-coded for content processes: each fake widget may have one and only one child (I forgot to add an assertion about the "one and only one" part, will do locally).  Then if a fake widget has a child, it just forwards resize etc. requests along to its child.  This is just a hack.
(In reply to comment #10)
> I see two assertions with these patches:
> 
> ###!!! ASSERTION: aChild already in the tree: '!aChild->GetParent()', file
> gfx/layers/basic/BasicLayers.cpp, line 167
> 
> and more often:
> ###!!! ASSERTION: aChild not our child: 'aChild->GetParent() == this', file
> gfx/layers/basic/BasicLayers.cpp, line 210

I'm aware, I obliquely referred to this in the checkin comment for http://hg.mozilla.org/users/cjones_mozilla.com/mcmq/rev/1719bd51e751.  Thanks.
Comment on attachment 460090 [details] [diff] [review]
Implement a "puppet widget" stand-in for platform widgets, for content processes, v2

The widgetry work has moved to bug 582057.  Sorry for the upcoming bugspam.
Attachment #460090 - Attachment is obsolete: true
Attachment #460090 - Flags: superreview?(roc)
Attachment #459929 - Attachment is obsolete: true
Attachment #459929 - Flags: superreview?(benjamin)
(In reply to comment #11)
> The view/widget code I've encountered while working on this bug relies on
> getting the top-level widget's native window ID, creating another nsIWidget for
> that ID, and having that new widget receive platform resize, paint, etc.
> notifications.

two nsIWidgets for the same native window? That makes no sense to me. Did Jim add that?
(In reply to comment #14)
> (In reply to comment #11)
> > The view/widget code I've encountered while working on this bug relies on
> > getting the top-level widget's native window ID, creating another nsIWidget for
> > that ID, and having that new widget receive platform resize, paint, etc.
> > notifications.
> 
> two nsIWidgets for the same native window? That makes no sense to me. Did Jim
> add that?

I don't know.  The situation in content processes is, each remote tab (<browser remote>) owns an nsWebBrowser (apparently part of our embedding API), and the nsWebBrowser is associated with a "top-level" widget.  Then an nsView creates its own widget from the "top-level" widget's window ID.  I changed nsView to explicitly create its widget as a child of the nsWebBrowser's nsIWidget in content processes, and then I added the hacked one-level fake widget hierarchy.  I can provide backtraces from the nsView widget creation, if that would be helpful.

Comment 16

8 years ago
(In reply to comment #14)
> (In reply to comment #11)
> > The view/widget code I've encountered while working on this bug relies on
> > getting the top-level widget's native window ID, creating another nsIWidget for
> > that ID, and having that new widget receive platform resize, paint, etc.
> > notifications.
> 
> two nsIWidgets for the same native window? That makes no sense to me. Did Jim
> add that?

I think he's referring to the parent widget / inner borderless child widget we create for all top level windows. That's been removed for top level windows on windows, but still exists for pretty much everything else / every other platform. The top level parent is owned by nsWebBrowser, the child is owned by an nsView created in nsDocumentViewer via MakeWindow.

(We still do this for dialogs on windows as well, although I think that would be easy to update.)
Comment on attachment 458580 [details] [diff] [review]
Still rough WIP, some bugs fixed

Patch-bomb coming up tomorrow, so with full r- capabilities pending, feedback doesn't seem worth your while.  Unless you would prefer to do it that way!
Attachment #458580 - Flags: feedback?(tnikkel)
Created attachment 461114 [details] [diff] [review]
part a: Fix an assertion that will soon no longer hold and remove obsoleted already_AddRefed idiom in two places
Attachment #458580 - Attachment is obsolete: true
Attachment #461114 - Flags: review?(roc)
Created attachment 461115 [details] [diff] [review]
part b: Fix a hidden header depedency and add a PresShell::GetLayerManager() helper
Attachment #461115 - Flags: review?(tnikkel)
Created attachment 461116 [details] [diff] [review]
part c: Add two trivial helper methods to nsFrameLoader
Attachment #461116 - Flags: review?(Olli.Pettay)
Created attachment 461117 [details] [diff] [review]
part d: Add some helper methods and functions to TabChild and TabParent
Attachment #461117 - Flags: superreview?(Olli.Pettay)
Created attachment 461118 [details] [diff] [review]
part e: Implement PuppetWidget::Destroy. r=roc
Attachment #461118 - Flags: review?(roc)
Created attachment 461119 [details] [diff] [review]
part f: Add a "destroy" phase to PBrowser shutdown to make clean-up easier
Attachment #461119 - Flags: superreview?(benjamin)
Attachment #461119 - Flags: review?(Olli.Pettay)
Created attachment 461120 [details] [diff] [review]
part g: Fix bone-headed bugs with shadowable layers, remove unnecessary MOZ_LAYER_DECL_NAME()s, and have the "shadow layer manager" side keep a "shadow root" rather than setting the "real" root
Attachment #461120 - Flags: review?(vladimir)
Created attachment 461121 [details] [diff] [review]
part h: Add a Layer::Destroy interface. sr=vlad
Attachment #461121 - Flags: superreview?(vladimir)
Created attachment 461122 [details] [diff] [review]
part i: Implement ShadowLayer::Destroy() for common-case shutdown, and ShadowableLayer::Destroy() for emergency-case shutdown
Attachment #461122 - Flags: review?(vladimir)
Created attachment 461123 [details] [diff] [review]
part j: Add a PRenderFrame protocol for cross-process frame objects
Attachment #461123 - Flags: review?(tnikkel)
Created attachment 461124 [details] [diff] [review]
part k: Implement the compositor side of PRenderFrame, which grafts a shadow layer tree into the compositor's tree
Attachment #461124 - Flags: superreview?(roc)
Attachment #461124 - Flags: review?(tnikkel)
Created attachment 461125 [details] [diff] [review]
part l: Implement the child side of PRenderFrame, which just acts as a conduit for shadow layers
Attachment #461125 - Flags: review?(tnikkel)
Created attachment 461126 [details] [diff] [review]
part m: Add API for attaching a "remote frame" to its corresponding nsFrameLoader, so that the frame can be found during painting
Attachment #461126 - Flags: superreview?(Olli.Pettay)
Attachment #461126 - Flags: review?(tnikkel)
Created attachment 461127 [details] [diff] [review]
part n: Hook layout/ipc into the build system, and integrate PRenderFrame into the PBrowser family
Attachment #461127 - Flags: superreview?(benjamin)
Created attachment 461128 [details] [diff] [review]
part o: Connect the dots to enable drawing remote frames for <browser remote>: create the frame from the child-side PresShell, insert a display item for it in compositor-side SubdocFrame, and use the
Attachment #461128 - Flags: superreview?(roc)
Attachment #461128 - Flags: review?(tnikkel)
I owe some comments on these patches.  I'll try a "sum comment" rather than leaving bread crumbs on the individual patches.  The big picture is still pretty much as described in comment 2: in the content process, when we hit PresShell::Paint(), we create a "remote frame" if we didn't already have one.  Then during normal painting, gfx/layers code creates shadow layers under the covers, and forwards them to the compositor side of the remote frame.  When we get ready to paint on the compositor side, we hand off the shadow layer tree to its FrameLayerBuilder, and painting proceeds normally.  It's really about as simple as that, from a high level.

The first set of patches, a-d, implement some little helpers here and there that I wanted to use, and fixed some minor bugs.

Patch e has PuppetWidget early-destroy its layer manager so as to maintain the "common case" shadow layer lifetime described below.

Patch f changes the way PBrowser shuts down.  Instead of the chrome process immediately sending PBrowser::__delete__(), we instead first send a Destroy() method that kicks off cleanup.  This makes cleanup easier because it can happen before all PBrowser managees are __delete__()d.  (mfinkle additionally wants a frontend cleanup phase that naturally fits into this Destroy() cycle.)

Patch g fixes some mistakes I made in bug 570294.  There I only tested with a dedicated compositor process that didn't have layers of its own.  For fennec, however, the chrome process will composite the shadow layers with its own layers.  This means that ShadowableLayers don't always have a shadow (bug in BasicShadowableThebesLayer), and additionally SetRoot() on a shadow tree shouldn't set the layer manager's root: that's owned by the chrome process.

Patches h and i implement a more robust shadow-layer life cycle.  In the common case, cleanup proceeds as
  ~ShadowableLayer()
    [unmap Shmems]
    Send__delete__(this)
    === IPC ===
    ShadowLayer::ActorDestroy()
      *ShadowLayer::Destroy()
        [unmap Shmems]
        [remain as no-op husk until refcount hits 0]
In the uncommon case (i.e. a process crashes), we hit emergency cleanup.  Here, we do
  ShadowableLayer::Destroy()
    [drop IPC resources, wait for IPDL auto-cleanup to kick in]
--and--
  ~ShadowLayer()
    [drop IPC resources, wait for IPDL auto-cleanup to kick in]
I tested all three cases under valgrind, and they're clean for the executions I was able to force.

Part j adds a relatively trivial PRenderFrame protocol for hooking up shadow layer trees across processes.

Part k implements the compositor side of things.  Here, on a paint, we create a non-shadow ContainerLayer for the shadow layer tree, then translate that ContainerLayer into the right widget coordinates.  The translation is used because content process PuppetWidgets always paint to a virtual <0, 0> top-left point.

Part l implements the child side of PRenderFrame, which just asks as a dynamic scope for a shadow layer tree.  It doesn't do anything interesting on its own.

Part m adds API to account for the fact that the content-process frame tree life cycle is different from that in the chrome process.  nsFrameLoader is used a level of indirection; when a content-process layer-tree transaction occurs, we stow away the targeted PRenderFrame on the FrameLoader.  Then when the chrome process gets ready to paint, the frame loader's nsSubdocFrame asks the frame loader for its current PRenderFrame, and builds a display item out of that if it's available.

Part n is the boilerplate code for building PRenderFrame stuff and hooking into its IPDL family.

Part o puts all the pieces together, and has content and chrome processes use IPC-enabled layer managers.

Please let me know if I can clarify.  Hope this format works out ...
Comment on attachment 461115 [details] [diff] [review]
part b: Fix a hidden header depedency and add a PresShell::GetLayerManager() helper

nsIPresShell needs an IID bump.

>   /**
>+   * Get the layer manager for this.
>+   */
>+  virtual LayerManager* GetLayerManager() = 0;

The comment is a little misleading. The returned layer manager will be the layer manager for the nearest widget to the root frame of this presshell. Maybe even change the name of the function to GetNearestLayerManager.

>+LayerManager* PresShell::GetLayerManager()
>+{
>+  nsIFrame* root = GetRootFrame();
>+  if (root) {
>+    nsIWidget* widget = root->GetNearestWidget();
>+    if (widget) {
>+      return widget->GetLayerManager();
>+    }
>+  }
>+  return nsnull;
>+}

For the old use of this code in DocumentStatesChanged it didn't matter if we didn't have a root frame because there would be no controls to update if there was no root frame. I don't know exactly where you call this in the later parts but I think you might need to call it when there is no root frame. For example, we do painting when there is no root frame. So I think we need to fallback to using the root view, or just use the root view instead, to get the widget, ie mViewManager->GetRootView().
Comment on attachment 461116 [details] [diff] [review]
part c: Add two trivial helper methods to nsFrameLoader

 >+nsIFrame*
>+nsFrameLoader::GetPrimaryFrameOfOwningContent() const
>+{
>+  return mOwnerContent ? mOwnerContent->GetPrimaryFrame() : nsnull;
>+}
>+
This is so trivial that this could be inline, especially if this is called a lot.

>+
>+  /** Return the document that owns this. */
>+  nsIDocument* GetOwnerDoc() { return mOwnerContent->GetOwnerDoc(); }
Null check mOwnerContent
Attachment #461116 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 461117 [details] [diff] [review]
part d: Add some helper methods and functions to TabChild and TabParent


>+inline TabChild*
>+GetTabChildFrom(nsIDocShell* aDocShell)
>+{
>+    nsCOMPtr<nsIDocShellTreeItem> treeItem(do_QueryInterface(aDocShell));
>+    if (!treeItem) {
>+        return nsnull;
>+    }
>+    nsCOMPtr<nsIDocShellTreeOwner> treeOwner;
>+    if (NS_FAILED(treeItem->GetTreeOwner(getter_AddRefs(treeOwner))) ||
>+        !treeOwner) {
>+        return nsnull;
>+    }
>+    nsCOMPtr<nsITabChild> iTabChild(do_GetInterface(treeOwner));
>+    return static_cast<TabChild*>(iTabChild.get());
>+}

nsCOMPtr<nsITabChild> tc = do_GetInterface(aDocShell); should work, at least based
on nsDocShell::GetInterface
So this method could be
nsCOMPtr<nsITabChild> tc = do_GetInterface(aDocShell);
return return static_cast<TabChild*>(tc.get());
Attachment #461117 - Flags: superreview?(Olli.Pettay) → superreview+
Comment on attachment 461119 [details] [diff] [review]
part f: Add a "destroy" phase to PBrowser shutdown to make clean-up easier

With this change TabChild::ActorDestroy's content could be moved
to TabChild::RecvDestroy(), I think.

But shouldn't cause any harm to have ActorDestroy, so r=me.
Attachment #461119 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 461126 [details] [diff] [review]
part m: Add API for attaching a "remote frame" to its corresponding nsFrameLoader, so that the frame can be found during painting

How do we clear mCurrentRemoteFrame when the child process dies or
is shutdown?
(In reply to comment #26)
> Created attachment 461122 [details] [diff] [review]
> part i: Implement ShadowLayer::Destroy() for common-case shutdown, and
> ShadowableLayer::Destroy() for emergency-case shutdown

Quick note: there's a .cpp file that I forgot to |hg add| to this patch, but it's really not surprising and does just what the header says.  Let me know what you prefer.
(In reply to comment #34)
> Comment on attachment 461115 [details] [diff] [review]
> part b: Fix a hidden header depedency and add a PresShell::GetLayerManager()
> helper
> 
> nsIPresShell needs an IID bump.
> 
> >   /**
> >+   * Get the layer manager for this.
> >+   */
> >+  virtual LayerManager* GetLayerManager() = 0;
> 
> The comment is a little misleading. The returned layer manager will be the
> layer manager for the nearest widget to the root frame of this presshell. Maybe
> even change the name of the function to GetNearestLayerManager.
> 

OK.

> >+LayerManager* PresShell::GetLayerManager()
> >+{
> >+  nsIFrame* root = GetRootFrame();
> >+  if (root) {
> >+    nsIWidget* widget = root->GetNearestWidget();
> >+    if (widget) {
> >+      return widget->GetLayerManager();
> >+    }
> >+  }
> >+  return nsnull;
> >+}
> 
> For the old use of this code in DocumentStatesChanged it didn't matter if we
> didn't have a root frame because there would be no controls to update if there
> was no root frame. I don't know exactly where you call this in the later parts
> but I think you might need to call it when there is no root frame.

Yes, possibly.  I call it on the compositor side approximately when a child-side frame has been created, and we need a "shadow manager" for its layers.

> For example,
> we do painting when there is no root frame. So I think we need to fallback to
> using the root view, or just use the root view instead, to get the widget, ie
> mViewManager->GetRootView().

OK.
Created attachment 461443 [details] [diff] [review]
part b: Fix a hidden header depedency and add a PresShell::GetNearestLayerManager() helper, v2
Attachment #461115 - Attachment is obsolete: true
Attachment #461443 - Flags: review?(tnikkel)
Attachment #461115 - Flags: review?(tnikkel)
(In reply to comment #35)
> Comment on attachment 461116 [details] [diff] [review]
> part c: Add two trivial helper methods to nsFrameLoader
> 
>  >+nsIFrame*
> >+nsFrameLoader::GetPrimaryFrameOfOwningContent() const
> >+{
> >+  return mOwnerContent ? mOwnerContent->GetPrimaryFrame() : nsnull;
> >+}
> >+
> This is so trivial that this could be inline, especially if this is called a
> lot.
> 

It's not called frequently by my patches, but I went ahead and made it inline.

> >+
> >+  /** Return the document that owns this. */
> >+  nsIDocument* GetOwnerDoc() { return mOwnerContent->GetOwnerDoc(); }
> Null check mOwnerContent

Done.
(In reply to comment #36)
> Comment on attachment 461117 [details] [diff] [review]
> part d: Add some helper methods and functions to TabChild and TabParent
> 
> 
> >+inline TabChild*
> >+GetTabChildFrom(nsIDocShell* aDocShell)
> >+{
> >+    nsCOMPtr<nsIDocShellTreeItem> treeItem(do_QueryInterface(aDocShell));
> >+    if (!treeItem) {
> >+        return nsnull;
> >+    }
> >+    nsCOMPtr<nsIDocShellTreeOwner> treeOwner;
> >+    if (NS_FAILED(treeItem->GetTreeOwner(getter_AddRefs(treeOwner))) ||
> >+        !treeOwner) {
> >+        return nsnull;
> >+    }
> >+    nsCOMPtr<nsITabChild> iTabChild(do_GetInterface(treeOwner));
> >+    return static_cast<TabChild*>(iTabChild.get());
> >+}
> 
> nsCOMPtr<nsITabChild> tc = do_GetInterface(aDocShell); should work, at least
> based
> on nsDocShell::GetInterface
> So this method could be
> nsCOMPtr<nsITabChild> tc = do_GetInterface(aDocShell);
> return return static_cast<TabChild*>(tc.get());

Cool thanks, much better.  Changed.
(In reply to comment #37)
> Comment on attachment 461119 [details] [diff] [review]
> part f: Add a "destroy" phase to PBrowser shutdown to make clean-up easier
> 
> With this change TabChild::ActorDestroy's content could be moved
> to TabChild::RecvDestroy(), I think.
> 
> But shouldn't cause any harm to have ActorDestroy, so r=me.

Well, they sort of serve different purposes.  RecvDestroy() will be the common-case, normal shutdown path.  It wouldn't be called e.g. if the chrome process crashes, but I'm not sure we really care about that case.  ActorDestroy() will always be called (unless we _exit() first).  It's not entirely clear to me which shutdown code should live where (~TabChild() is also in the mix), so since you're OK with the current impl maybe we should just revisit it as the need arises.
(In reply to comment #38)
> Comment on attachment 461126 [details] [diff] [review]
> part m: Add API for attaching a "remote frame" to its corresponding
> nsFrameLoader, so that the frame can be found during painting
> 
> How do we clear mCurrentRemoteFrame when the child process dies or
> is shutdown?

See RenderFrameParent::ActorDestroy in https://bug570620.bugzilla.mozilla.org/attachment.cgi?id=461124 (part k).
Comment on attachment 461443 [details] [diff] [review]
part b: Fix a hidden header depedency and add a PresShell::GetNearestLayerManager() helper, v2

>+   * Get the layer manager for the nearest widget to the root frame of
>+   * this presshell, or the widget of the root view if we don't have a
>+   * root frame.

We just look at the view, so don't need to mention the frame bits.

>+LayerManager* PresShell::GetNearestLayerManager()
>+{
>+  NS_ASSERTION(mViewManager, "Should have view manager");
>+
>+  nsIView* rootView;
>+  if (NS_SUCCEEDED(mViewManager->GetRootView(rootView)) && rootView) {
>+    if (nsIWidget* widget = rootView->GetWidget()) {
>+      return widget->GetLayerManager();
>+    }
>+  }
>+  return nsnull;
>+}

nsIView::GetWidget just returns the widget that the view has (so would be null if the view doesn't have a widget), it doesn't look at any parent views like nsIView::GetNearestWidget does. I would actually prefer the use of a function like this (but called GetLayerManager) if you don't need the "look in parent documents for a widget" behaviour. I think DocumentStatesChanged should still use the "look in parent documents for a widget" version.
Created attachment 461665 [details] [diff] [review]
part b: Fix a hidden header depedency and add a PresShell::GetLayerManager() helper

(In reply to comment #46)
> Comment on attachment 461443 [details] [diff] [review]
> part b: Fix a hidden header depedency and add a
> PresShell::GetNearestLayerManager() helper, v2
> 
> >+   * Get the layer manager for the nearest widget to the root frame of
> >+   * this presshell, or the widget of the root view if we don't have a
> >+   * root frame.
> 
> We just look at the view, so don't need to mention the frame bits.
> 

Oops, fixed.

> >+LayerManager* PresShell::GetNearestLayerManager()
> >+{
> >+  NS_ASSERTION(mViewManager, "Should have view manager");
> >+
> >+  nsIView* rootView;
> >+  if (NS_SUCCEEDED(mViewManager->GetRootView(rootView)) && rootView) {
> >+    if (nsIWidget* widget = rootView->GetWidget()) {
> >+      return widget->GetLayerManager();
> >+    }
> >+  }
> >+  return nsnull;
> >+}
> 
> nsIView::GetWidget just returns the widget that the view has (so would be null
> if the view doesn't have a widget), it doesn't look at any parent views like
> nsIView::GetNearestWidget does. I would actually prefer the use of a function
> like this (but called GetLayerManager) if you don't need the "look in parent
> documents for a widget" behaviour. I think DocumentStatesChanged should still
> use the "look in parent documents for a widget" version.

Done (reverted DocumentStatesChanged hunk).
Attachment #461443 - Attachment is obsolete: true
Attachment #461665 - Flags: review?(tnikkel)
Attachment #461443 - Flags: review?(tnikkel)
Comment on attachment 461665 [details] [diff] [review]
part b: Fix a hidden header depedency and add a PresShell::GetLayerManager() helper

>+   * Get the layer manager for the widget of the root view.
>+   */
>+  virtual LayerManager* GetLayerManager() = 0;

Just add "if it has one" to the comment.
Attachment #461665 - Flags: review?(tnikkel) → review+
+using mozilla::layers::BasicLayerManager;
+using mozilla::layers::BasicShadowLayerManager;
+using mozilla::layers::ContainerLayer;
+using mozilla::layers::Layer;
+using mozilla::layers::LayerManager;
+using mozilla::layers::PLayersParent;
+using mozilla::layers::ShadowLayersParent;

Why not just "using namespace mozilla::layers"?

+  // XXX: *lots* of tuning can be done here.  Could set
+  // ACTIVE/INACTIVE based on shadow-tree-update granularity, or
+  // partition shadow tree based on constituent update frequency, or
+  // ...  Just set ACTIVE for now.

I don't think we'll ever want these to be INACTIVE...

+SetTransformFor(ContainerLayer* aContainer, nsIFrame* aContainedFrame)
+{
+  NS_ABORT_IF_FALSE(aContainer && aContainedFrame, "args must be nonnull");
+
+  nsPoint offset;
+  nsIWidget* widget = aContainedFrame->GetNearestWidget(offset);

I don't know why you want this. I would have expected that in RenderFrameParent::BuildLayer you would be finding the offset from aFrame to the nsDisplayListBuilder's reference frame. So pass in aBuilder and call aBuilder->ToReferenceFrame(aFrame) to find out where aFrame's content should be positioned. If aFrame can have borders and padding, use aBuilder->ToReferenceFrame(aFrame->GetParent()) + aFrame->GetContentRect().TopLeft().
+  RenderFrameParent* frame();

Frame()

+  /*
+   * With single-process rendering, this method is a no-op that
+   * returns PR_TRUE.
+   *
+   * Otherwise, if we don't have a remote rendering frame, try to
+   * create it and set up layer forwarding.  Return PR_TRUE iff
+   * successful.
+   */
+  PRBool SetUpRemoteFrame();
+
+  /**
+   * Only meaningful with multi-process rendering.  Tear down our
+   * remote frame, if we have one.
+   */
+  void TearDownRemoteFrame();
+
+  /*
+   * Cast the spells needed to dig the child-side <browser> out of
+   * this.  Return NULL if it can't be found, e.g., if this isn't a
+   * child process.
+   */
+  TabChild* GetTabChild();
+
+  PRenderFrameChild* mRemoteFrame;

I do not like this stuff appearing in PresShell. Can we keep cross-process-rendering logic separated out by delegating it to the PuppetWidget maybe?

Updated

8 years ago
Attachment #461126 - Flags: superreview?(Olli.Pettay) → superreview+
(In reply to comment #50)
> +using mozilla::layers::BasicLayerManager;
> +using mozilla::layers::BasicShadowLayerManager;
> +using mozilla::layers::ContainerLayer;
> +using mozilla::layers::Layer;
> +using mozilla::layers::LayerManager;
> +using mozilla::layers::PLayersParent;
> +using mozilla::layers::ShadowLayersParent;
> 
> Why not just "using namespace mozilla::layers"?
> 

Will do whatever the style guide says after we write that, pending discussion in bug 582057.

> +  // XXX: *lots* of tuning can be done here.  Could set
> +  // ACTIVE/INACTIVE based on shadow-tree-update granularity, or
> +  // partition shadow tree based on constituent update frequency, or
> +  // ...  Just set ACTIVE for now.
> 
> I don't think we'll ever want these to be INACTIVE...
> 

Maybe.  Anyway, I'm not going to play with that in the foreseeable future, I'll drop the comment.

> +SetTransformFor(ContainerLayer* aContainer, nsIFrame* aContainedFrame)
> +{
> +  NS_ABORT_IF_FALSE(aContainer && aContainedFrame, "args must be nonnull");
> +
> +  nsPoint offset;
> +  nsIWidget* widget = aContainedFrame->GetNearestWidget(offset);
> 
> I don't know why you want this.

FTR, I don't know the best way to do things like this and so don't have any opinion.  Please dictate to me when I screw up (like below, thanks!).

> I would have expected that in
> RenderFrameParent::BuildLayer you would be finding the offset from aFrame to
> the nsDisplayListBuilder's reference frame. So pass in aBuilder and call
> aBuilder->ToReferenceFrame(aFrame) to find out where aFrame's content should be
> positioned. If aFrame can have borders and padding, use
> aBuilder->ToReferenceFrame(aFrame->GetParent()) +
> aFrame->GetContentRect().TopLeft().

Will fix, thanks.
(In reply to comment #51)
> +  RenderFrameParent* frame();
> 
> Frame()
> 

OK.

> +  /*
> +   * With single-process rendering, this method is a no-op that
> +   * returns PR_TRUE.
> +   *
> +   * Otherwise, if we don't have a remote rendering frame, try to
> +   * create it and set up layer forwarding.  Return PR_TRUE iff
> +   * successful.
> +   */
> +  PRBool SetUpRemoteFrame();
> +
> +  /**
> +   * Only meaningful with multi-process rendering.  Tear down our
> +   * remote frame, if we have one.
> +   */
> +  void TearDownRemoteFrame();
> +
> +  /*
> +   * Cast the spells needed to dig the child-side <browser> out of
> +   * this.  Return NULL if it can't be found, e.g., if this isn't a
> +   * child process.
> +   */
> +  TabChild* GetTabChild();
> +
> +  PRenderFrameChild* mRemoteFrame;
> 
> I do not like this stuff appearing in PresShell. Can we keep
> cross-process-rendering logic separated out by delegating it to the
> PuppetWidget maybe?

I'd like to discuss this change in person today (sent e-mail).  The reason this code lives in layout/ currently is that I think we'll eventually want to send a richer content-process frame tree to chrome.  The reason is, if/when we want to optimize double-tap zoom and/or sub-document scrolling, I suspect we'll want that logic to live in layout/ instead of gfx/layers.  But this is a pretty big design point, like I said I'd like to chat about it.
Created attachment 462695 [details] [diff] [review]
part k: Implement the compositor side of PRenderFrame, which grafts a shadow layer tree into the compositor's tree, v2
Attachment #461124 - Attachment is obsolete: true
Attachment #461127 - Attachment is obsolete: true
Attachment #461128 - Attachment is obsolete: true
Attachment #462695 - Flags: superreview?(roc)
Attachment #462695 - Flags: review?(tnikkel)
Attachment #461124 - Flags: superreview?(roc)
Attachment #461124 - Flags: review?(tnikkel)
Attachment #461128 - Flags: superreview?(roc)
Attachment #461128 - Flags: review?(tnikkel)
Attachment #461127 - Flags: superreview?(benjamin)
Created attachment 462697 [details] [diff] [review]
part n: Hook layout/ipc into the build system, and integrate PRenderFrame into the PBrowser family, v2
Attachment #462697 - Flags: superreview?(benjamin)
Created attachment 462698 [details] [diff] [review]
part o: Connect the dots to enable drawing remote frames for <browser remote>: create the frame from the child-side PresShell, insert a display item for it in compositor-side SubdocFrame[etc.], v2
Attachment #462698 - Flags: superreview?(roc)
Attachment #462698 - Flags: review?(tnikkel)
Note to self and others: bug 576192 is where the initial fennec+<browser> work is happening.

Updated

8 years ago
Attachment #461119 - Flags: superreview?(benjamin) → superreview+

Updated

8 years ago
Attachment #462697 - Flags: superreview?(benjamin) → superreview+

Updated

8 years ago
tracking-fennec: --- → 2.0b1+
Part k:

+      mContainer = aManager->CreateContainerLayer();

In other places we would want to call GetOldLayerFor here and not store the layer yourself in mContainer at all. That lets FrameLayerBuilder return an existing layer if there is one and we're updating the retained layer manager, but it will return null if the layer is not usable (e.g. because we're rendering fallback).

In this case though, there's no point since the shadow layers belong to mContainer and can't be moved elsewhere. What is the plan for handling fallback rendering? (e.g. someone doing a drawWindow of the browser window in a situation where we can't just blit the layer tree)
Attachment #461123 - Flags: review?(tnikkel) → review?(roc)
Attachment #461125 - Flags: review?(tnikkel) → review?(roc)
Attachment #461126 - Flags: review?(tnikkel) → review?(roc)
Comment on attachment 462695 [details] [diff] [review]
part k: Implement the compositor side of PRenderFrame, which grafts a shadow layer tree into the compositor's tree, v2

Mats, the place to start here is comment 33, which is an overview of what all this stuff is trying to accomplish.
Attachment #462695 - Flags: review?(tnikkel) → review?(matspal)
Attachment #462698 - Flags: review?(tnikkel) → review?(matspal)
Comment on attachment 461121 [details] [diff] [review]
part h: Add a Layer::Destroy interface. sr=vlad

There's a couple of problems here, largely stemming from this Destroy being treated as "optional" as opposed to part of a normal cleanup path.  That is, I don't see destructors in the next patch ever calling Destroy if it hasn't been called before.  That's fine, because Destroy use in shadow layers is trivial, but it's not trivial in the existing layers code that has Destroy calls.  The destructor must call Destroy at some point; I had this written down as the most-derived class calls Destroy, but it would be nice to enforce that somehow.

Also, you need to update the existing Destroy methods to follow your rules here, specifically calling the nearest base class's Destroy method.
Attachment #461121 - Flags: superreview?(vladimir) → superreview-
Part o:

     if (!mLayerManager) {
+#if !defined(MOZ_IPC)
       mLayerManager = new BasicLayerManager(this);
+#else
+      mLayerManager = new BasicShadowLayerManager(this);
+#endif
     }

Surely we don't want to create a ShadowLayerManager for non-accelerated chrome widgets?

+#  include "mozilla/layout/RenderFrameParent.h"

Remove spaces after #

+  // FIXME for multi-managed PLayers

What does this mean?

Otherwise looks good.
(In reply to comment #62)
> Part o:
> 
>      if (!mLayerManager) {
> +#if !defined(MOZ_IPC)
>        mLayerManager = new BasicLayerManager(this);
> +#else
> +      mLayerManager = new BasicShadowLayerManager(this);
> +#endif
>      }
> 
> Surely we don't want to create a ShadowLayerManager for non-accelerated chrome
> widgets?
> 

The memory difference between BasicLayerManager and BasicShadowLayerManager is that the latter (i) implements ShadowLayerForwarder, which adds 2*sizeof(void*) overhead; and (ii) has a sizeof(void*) inline member.  The behavioral difference is that BasicShadowLayerManager creates managee ShadowableLayers.  All ShadowableLayers have a sizeof(void*) memory overhead; the ones with shared surfaces add more.  Worst are ShadowableThebes/ShadowableImage, which have an additional sizeof(void*)+sizeof(nsIntSize) overhead.

The actual IPC code is all no-ops unless the manager has been hooked up properly.  So, the net result of summarily switching to BasicShadowLayerManager is at worst 2*sizeof(void*)+sizeof(nsIntSize) memory overhead per layer and more runtime |if (null)| checks.  This seemed acceptable to me.

The alternative is to add an API to allow nsIWidget to query whether it should use a BasicShadowLayerManager, or an API to tell it to do that.  This is easy inside PuppetWidget, where we can always assume BasicShadowLayerManager, but I couldn't think of a clean, simple way to do it in the chrome process.  I think the chrome widget will be created before chrome knows it has a <browser remote>, which seems to make things trickier.  Do you have ideas for this?

> +#  include "mozilla/layout/RenderFrameParent.h"
> 
> Remove spaces after #
> 

OK.

> +  // FIXME for multi-managed PLayers
> 
> What does this mean?
> 

IPDL protocols can have multiple "managers"; PLayers will be one such, since it will have PContent, PPlugin, and PCompositor as managers eventually.  When that happens, this code will have to know which manager is being used at runtime, or we'll need manager-specific paths.  Anyway, it's not a terribly useful comment since the code will fail to compile with multiple management, so I'll drop the comment.
Comment on attachment 462698 [details] [diff] [review]
part o: Connect the dots to enable drawing remote frames for <browser remote>: create the frame from the child-side PresShell, insert a display item for it in compositor-side SubdocFrame[etc.], v2

(In reply to comment #63)
> Do you have ideas for this?

No, I understand now. That's fine.
Attachment #462698 - Flags: superreview?(roc) → superreview+
(In reply to comment #61)
> Comment on attachment 461121 [details] [diff] [review]
> part h: Add a Layer::Destroy interface. sr=vlad
> 

We discussed this on IRC and settled on a better solution that hid away this Destroy() setup inside of gfx/layers/ipc.  Unfortunately that required a lot of refactoring that ended up hitting nasty OO issues.

We had also discussed s/Layer::Destroy/Layer::Disconnect/ as a less palatable solution.  Vlad, is that still OK enough with you for us to move forward?
Yeah, Disconnect is still fine with me, just make sure you document it as a remote-layers-only thing.
Gah, I missed this comment somehow.

(In reply to comment #59)
> Part k:
> 
> +      mContainer = aManager->CreateContainerLayer();
> 
> In other places we would want to call GetOldLayerFor here and not store the
> layer yourself in mContainer at all. That lets FrameLayerBuilder return an
> existing layer if there is one and we're updating the retained layer manager,
> but it will return null if the layer is not usable (e.g. because we're
> rendering fallback).
> 

I tried to follow the <canvas> impl as well as I understood it.  Looking back over it, nsHTMLCanvasFrame::BuildLayer has a
  oldLayer = aBuilder->LayerBuilder()->GetLeafLayerFor(aBuilder, aManager, aItem)
call that this impl doesn't, and checks a user data pointer to make sure the old layer is still current.  Is this what you have in mind?  I don't see what this buys us, but I'm not very familiar with FrameLayerBuilder.  Will do some more reading today.

> In this case though, there's no point since the shadow layers belong to
> mContainer and can't be moved elsewhere.

OK.  So keeping around mContainer is the right thing to do?

> What is the plan for handling fallback
> rendering? (e.g. someone doing a drawWindow of the browser window in a
> situation where we can't just blit the layer tree)

In what situation(s) could we not just blit the shadow layer tree?  As long as there's a remote frame and shadow tree present, wrt chrome code the shadow tree is just a set of immutable pixel buffers.

I'll ping you later today to make sure I understand all this; feeling a bit confused.
The problem is that we don't have an API right now to just blit the shadow layer subtree; we can only blit complete layer managers.
(In reply to comment #67)
> OK.  So keeping around mContainer is the right thing to do?

Yeah, I think we can do that, but for now you're going to have to check that mContainer's layer manager is the same as the layer manager passed in, and if it's not, we won't be able to render anything.
(In reply to comment #68)
> The problem is that we don't have an API right now to just blit the shadow
> layer subtree; we can only blit complete layer managers.

Right, drawWindow of the content window.  I keep thinking "window" in the OS window sense.(In reply to comment #69)

> (In reply to comment #67)
> > OK.  So keeping around mContainer is the right thing to do?
> 
> Yeah, I think we can do that, but for now you're going to have to check that
> mContainer's layer manager is the same as the layer manager passed in, and if
> it's not, we won't be able to render anything.

Hmm ... right now that's an assertion, because shadow layers can't (yet) deal with managers changing out from under them.  Is this case going to arise with drawWindow?  Is there any way to differentiate between drawWindow and the widget's layer manager changing?
(In reply to comment #70)
> (In reply to comment #69)
> > (In reply to comment #67)
> > > OK.  So keeping around mContainer is the right thing to do?
> > 
> > Yeah, I think we can do that, but for now you're going to have to check that
> > mContainer's layer manager is the same as the layer manager passed in, and if
> > it's not, we won't be able to render anything.
> 
> Hmm ... right now that's an assertion, because shadow layers can't (yet) deal
> with managers changing out from under them.  Is this case going to arise with
> drawWindow?

Yes.

> Is there any way to differentiate between drawWindow and the
> widget's layer manager changing?

Possibly, but that's not really the problem. The problem is not the widget layer manager changing, because that is not likely to happen. The problem is when someone says "render this content into this gfxContext, yo" and we decide to create a temporary layer manager to do that instead of using the widget's layer manager (e.g. if someone calls drawWindow without DRAWWINDOW_USE_WIDGET_LAYERS).

I think it's fine to just return no layer and draw nothing in that case, though, for now.
> > Is there any way to differentiate between drawWindow and the
> > widget's layer manager changing?
> 
> Possibly, but that's not really the problem. The problem is not the widget
> layer manager changing, because that is not likely to happen.

Sure, I just want to loudly assert if that does happen, since the shadow layers will fail spectacularly if it does.

> I think it's fine to just return no layer and draw nothing in that case,
> though, for now.

OK, will do.
Created attachment 467681 [details] [diff] [review]
part h: Add a Layer::Disconnect interface
Attachment #461121 - Attachment is obsolete: true
Attachment #461122 - Attachment is obsolete: true
Attachment #467681 - Flags: superreview?(vladimir)
Attachment #461122 - Flags: review?(vladimir)
Created attachment 467682 [details] [diff] [review]
part i: Implement ShadowLayer::Disconnect() for common-case shutdown, and ShadowableLayer::Disconnect() for emergency-case shutdown
Attachment #467682 - Flags: review?(vladimir)
Created attachment 467692 [details] [diff] [review]
part k: Implement the compositor side of PRenderFrame, which grafts a shadow layer tree into the compositor's tree

Changed RenderFrameParent::BuildLayer to return NULL if the shadow tree's container doesn't match the passed-in layer manager.  Now asserting that if there's a manager mismatch, then the passed-in layer manager is a "temporary" layer manager, like for drawWindow.
Attachment #462695 - Attachment is obsolete: true
Attachment #467692 - Flags: superreview?(roc)
Attachment #467692 - Flags: review?(matspal)
Attachment #462695 - Flags: superreview?(roc)
Attachment #462695 - Flags: review?(matspal)
Comment on attachment 467692 [details] [diff] [review]
part k: Implement the compositor side of PRenderFrame, which grafts a shadow layer tree into the compositor's tree

In layout/ipc/RenderFrameParent.h
> #ifndef mozilla_dom_RenderFrameParent_h

s/dom/layout/
Attachment #467692 - Flags: review?(matspal) → review+
Comment on attachment 462698 [details] [diff] [review]
part o: Connect the dots to enable drawing remote frames for <browser remote>: create the frame from the child-side PresShell, insert a display item for it in compositor-side SubdocFrame[etc.], v2

In dom/ipc/TabChild.cpp
>     printf("creating %d!\n", NS_IsMainThread());

There's a few printf:s still around.

In dom/ipc/TabChild.h
> #ifndef mozilla_tabs_TabChild_h

s/tabs/dom/
Attachment #462698 - Flags: review?(matspal) → review+
Created attachment 467851 [details] [diff] [review]
make it compile with --disable-ipc

FWIW, the above patches fails to build with --disable-ipc
(In reply to comment #76)
> Comment on attachment 467692 [details] [diff] [review]
> part k: Implement the compositor side of PRenderFrame, which grafts a shadow
> layer tree into the compositor's tree
> 
> In layout/ipc/RenderFrameParent.h
> > #ifndef mozilla_dom_RenderFrameParent_h
> 
> s/dom/layout/

Fixed.

(In reply to comment #77)
> Comment on attachment 462698 [details] [diff] [review]
> part o: Connect the dots to enable drawing remote frames for <browser remote>:
> create the frame from the child-side PresShell, insert a display item for it in
> compositor-side SubdocFrame[etc.], v2
> 
> In dom/ipc/TabChild.cpp
> >     printf("creating %d!\n", NS_IsMainThread());
> 
> There's a few printf:s still around.
> 

Some of these are still moderately useful ... I'll file a followup for nuking them.

> In dom/ipc/TabChild.h
> > #ifndef mozilla_tabs_TabChild_h
> 
> s/tabs/dom/

Fixed.

(In reply to comment #78)
> Created attachment 467851 [details] [diff] [review]
> make it compile with --disable-ipc
> 
> FWIW, the above patches fails to build with --disable-ipc

Thanks for the patch!  I'm going to integrate into my mq; parts of this touch bug 582057.
Filed bug 589320 for the printfs.
Oops sorry Rob, landed also pending-sr from you on part k.
Attachment #467692 - Flags: superreview?(roc) → superreview+
Attachment #467681 - Flags: superreview?(vladimir) → superreview+
Followup note: I distributed Mats's --disable-ipc patch into the appropriate places in my mq and landed that too.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.