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

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: 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.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Still rough WIP, some bugs fixed

Same approach.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Gut platform widgets from content processes

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: 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.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: 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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 10

[Oleg Romashin (:romaxa)]

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

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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

[Jim Mathies [:jimm]]

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

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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!

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part a: Fix an assertion that will soon no longer hold and remove obsoleted already_AddRefed idiom in two places

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part b: Fix a hidden header depedency and add a PresShell::GetLayerManager() helper

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part c: Add two trivial helper methods to nsFrameLoader

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part d: Add some helper methods and functions to TabChild and TabParent

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part e: Implement PuppetWidget::Destroy. r=roc

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part f: Add a "destroy" phase to PBrowser shutdown to make clean-up easier

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: 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

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part h: Add a Layer::Destroy interface. sr=vlad

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part i: Implement ShadowLayer::Destroy() for common-case shutdown, and ShadowableLayer::Destroy() for emergency-case shutdown

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part j: Add a PRenderFrame protocol for cross-process frame objects

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part k: Implement the compositor side of PRenderFrame, which grafts a shadow layer tree into the compositor's tree

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part l: Implement the child side of PRenderFrame, which just acts as a conduit for shadow layers

Comment 30

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part m: Add API for attaching a "remote frame" to its corresponding nsFrameLoader, so that the frame can be found during painting

Comment 31

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part n: Hook layout/ipc into the build system, and integrate PRenderFrame into the PBrowser family

Comment 32

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: 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

Comment 33

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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 34

[Timothy Nikkel (:tnikkel)]

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 35

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

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());

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 39

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 40

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 41

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part b: Fix a hidden header depedency and add a PresShell::GetNearestLayerManager() helper, v2

Comment 42

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 43

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 44

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 45

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

[Timothy Nikkel (:tnikkel)]

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.

Comment 47

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 48

[Timothy Nikkel (: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.

Comment 49

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Done.

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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?

Comment 52

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 53

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 54

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part k: Implement the compositor side of PRenderFrame, which grafts a shadow layer tree into the compositor's tree, v2

Comment 55

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part n: Hook layout/ipc into the build system, and integrate PRenderFrame into the PBrowser family, v2

Comment 56

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: 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

Comment 57

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Note to self and others: bug 576192 is where the initial fennec+<browser> work is happening.

Comment 58

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Pushed parts a-d and part f
http://hg.mozilla.org/mozilla-central/rev/4cf30534e60b
http://hg.mozilla.org/mozilla-central/rev/e26839cea31c
http://hg.mozilla.org/mozilla-central/rev/ca01466ef0c6
http://hg.mozilla.org/mozilla-central/rev/2c0f4e0ff55d
http://hg.mozilla.org/mozilla-central/rev/715f808b0d2b

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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)

Comment 60

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 61

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 62

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 63

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 65

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 66

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yeah, Disconnect is still fine with me, just make sure you document it as a remote-layers-only thing.

Comment 67

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 68

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 69

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 70

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 71

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 72

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 73

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part h: Add a Layer::Disconnect interface

Comment 74

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part i: Implement ShadowLayer::Disconnect() for common-case shutdown, and ShadowableLayer::Disconnect() for emergency-case shutdown

Comment 75

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: 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.

Comment 76

[Mats Palmgren (inactive)]

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/

Comment 77

[Mats Palmgren (inactive)]

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/

Comment 78

[Mats Palmgren (inactive)]

Attachment: make it compile with --disable-ipc

FWIW, the above patches fails to build with --disable-ipc

Comment 79

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 80

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Filed bug 589320 for the printfs.

Comment 81

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/projects/cedar/rev/0e941af684ab
http://hg.mozilla.org/projects/cedar/rev/4b1b6833bd5a
http://hg.mozilla.org/projects/cedar/rev/1a2663f57705
http://hg.mozilla.org/projects/cedar/rev/dc39260535b2
http://hg.mozilla.org/projects/cedar/rev/84f936191578
http://hg.mozilla.org/projects/cedar/rev/9fb8965e9a5a
http://hg.mozilla.org/projects/cedar/rev/9cfe2875b4e6
http://hg.mozilla.org/projects/cedar/rev/ad761a2b8161
http://hg.mozilla.org/projects/cedar/rev/e5bb865360bc
http://hg.mozilla.org/projects/cedar/rev/9dfdccfd5bdc

Vlad, I landed the two Layer::Disconnect as pending-r, since you seem OK with the general approach.  Leaving this bug open to follow up on review comments.

Comment 82

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oops sorry Rob, landed also pending-sr from you on part k.

Comment 83

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Followup note: I distributed Mats's --disable-ipc patch into the appropriate places in my mq and landed that too.