745148 - Support content processes pushing layer transactions directly to omtc in parent process

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

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

In b2g, we'll have a set of content processes that want to push their updates directly to the off-main-thread compositor in the top-most parent (server) process.

Basically, we need to hand the widget code an "opens" or "bridge" channel to the compositor's thread context.

Comment 1

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

Should we extend CompositorFrameParent to support this? We don't want to duplicate that code.

Comment 2

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

There's no such animal.  My WIP is http://hg.mozilla.org/users/cjones_mozilla.com/mcmq/file/6e11a9641634/745148-directcompositor if you want to get a sense of what things look like.

Comment 3

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

Attachment: first version

Comment 4

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

Attachment: Core of the logic being added here

The real patch is of course much larger than this, but it's all boilerplate to get here.

I suspect adding a RefLayer type will be contentious, hence feedback request.  Alternate suggestions welcome.

Comment 5

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

Please ignore the |sCurrent| stuff in CompositorParent, it's a temporary hack.

Comment 6

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

Comment on attachment 640912 [details] [diff] [review]
Core of the logic being added here

Review of attachment 640912 [details] [diff] [review]:
-----------------------------------------------------------------

This seems reasonable assuming we can later extend RefLayer to refer to layer subtrees in the same layer manager, perhaps by adding a way to attach referent IDs to individual layers.

Comment 7

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

Attachment: Rebased

Comment 8

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

Attachment: part 0: Log the PID with MOZ_IPC_MESSAGE_LOG logging

Trivial helper.

Comment 9

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

Attachment: part 1: SyncLaunch() content processes when we're using direct-to-omtc drawing

This really stinks, but blame win32 and its lack of fork().

The motivating problem is, when we're using PCompositor to send messages directly from the content main thread to the compositor thread in the parent process, we need to PCompositor::Open() *right* after launching the content process.  Before any PBrowsers are created.

But to open the new channel, we have to use ShareHandle() on windows to send the child end of the pipe, and this requires having the HANDLE for the new process.  And getting that HANDLE requires disk IO.  Sigh.

This is bad, but I don't see a way around it, unfortunately.  On the bright side though, we only do this when we're using off-main-thread compositing, which means we can hide any latency here with an async animation.

Comment 10

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

I had one more patch for you, but looking back over it I'm not sure it's needed.  Will clean up and verify asap.

Comment 11

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

Attachment: part 1: Allow sending messages on an Opening channel, since they'll be queued if we're really actually still awaiting connect

Oh yeah, I remember what the problem was now.  Sigh.

So here's the story.  Chapter 1.  With content processes and OMTC, the main process does PCompositor::Open() to the content process, sending it a compositor Channel.  Previously, when opening these kinds of Channels, we blocked until we received the other side's pid.  That turns out to be unnecessary for "Opens" channels and child-process channels --- we already have the PID.

Who cares, you're probably asking yourself.  Well, we PCompositor::Open() asynchronously from the parent main thread.  The main thread goes back to its business, let's say painting using the sync PLayers protocol.  Meanwhile, we initialize the CompositorParent on the compositor thread, blocking in AsyncChannel::Open() on the other-side pid to be returned by the content-process main thread.  And when the content process starts up, the first thing it does is initialize the crash reporter, through sync SendPCrashReporterConstructor.  Hm.

 parent main thread --blocked on--> compositor thread
 compositor thread --blocked on--> content process
 content process --blocked on--> parent main thread

Whups.  The reason compositor thread blocks on content process is just to find out its pid basically, even though it already has it.  So we break the cycle there.

This patch doesn't wait for OnConnected, since we can send/recv IPC just fine before.

Comment 12

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

Attachment: 2: GeckoChildProcessHost can't drop messages on the floor. Queue them and hand them off to the *Channel.

Chapter 2.  After that fix, I noticed the content process would hang on startup sometimes.  The content main thread was blocked on the sync PCrashReporter ctor again.

After some poking, I found that the ctor message was arriving at the parent process and being dispatch to the Channel listener.  We were just never sending a reply, so the content process stayed hung.

Then this hypothesis came to me while eating pizza, and I added a MOZ_NOT_REACHED() to GeckoChildProcessHost::OnMessageReceived(), and it blew up.  So what was happening is that the sync ctor message was being dropped on the floor.

This patch queues up those too-early messages and then hands them off for delivery when we switch channel listeners.

This bug has been latent forever.  I think the combination of SyncLaunch() (waiting for OnConnected) and the sync PCrashReporter() ctor asap after Channel::Open exarcerbated the problem.  But, even without those this bug could still pop up.

Comment 13

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

Attachment: part 3: SyncLaunch() content processes when we're using direct-to-omtc drawing

Same patch as before.

Comment 14

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

I'm going to reuse some machinery Nico built in bug 598868 for finding the right CompositorParent to trigger recomposition for on async updates.

Comment 15

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

Attachment: part 4: Remove duplicated code in Basic*ContainerLayer

Comment 16

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

Attachment: part 5: Pass the layer tree to ShadowLayersUpdate()

Comment 17

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

Attachment: part 6: Allow layer trees to be given IDs so that the referent can be used in another context

This won't compile everywhere but I'll mop up those trivial compilation fixes in parallel.

Comment 18

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

Attachment: part 7: Create a RefLayer type to temporarily contain a foreign layer subtree during composition

Comment 19

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

Attachment: part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor

Not really a good way to split this up more, but it's mostly boilerplate.  The CompositorParent hunks are most interesting.  Please let me know if you don't comfortable reviewing the IPC bits.

Comment 20

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

Attachment: part 9: Hook up the pieces and enable direct compositor

Comment 21

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

Attachment: Rollup, based on m-c rev 1dbe06319739

Includes dependencies.

Comment 22

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

Comment on attachment 642046 [details] [diff] [review]
part 4: Remove duplicated code in Basic*ContainerLayer

Review of attachment 642046 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 641756 [details] [diff] [review]
2: GeckoChildProcessHost can't drop messages on the floor.  Queue them and hand them off to the *Channel.

Review of attachment 641756 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/AsyncChannel.cpp
@@ +715,5 @@
> +
> +    // Dispatch whatever messages the previous listener had queued up.
> +    while (!pending.empty()) {
> +        OnMessageReceived(pending.front());
> +        pending.pop();

Should you pop before calling OnMessageReceived in case it reenters somehow? Something like:

  Message* msg = pending.front();
  pending.pop();
  OnMessageReceived(msg);

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +692,4 @@
>  void
>  GeckoChildProcessHost::OnChannelError()
>  {
>    // XXXbent Notify that the child process is gone?

Should probably file a followup since this can happen too, right?

::: ipc/glue/GeckoChildProcessHost.h
@@ +106,5 @@
>                                    base::ProcessArchitecture arch);
> +
> +  // There's a short window in between we launch the subprocess and we
> +  // hand the channel off that messages can be received by this, while
> +  // it's still the channel listener.  If so, the messages would

Nit: This comment is all sorts of grammatical nightmare. Must have been late ;) Let's clean it up a little.

Comment 24

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

(In reply to ben turner [:bent] from comment #23)
> Comment on attachment 641756 [details] [diff] [review]
> 2: GeckoChildProcessHost can't drop messages on the floor.  Queue them and
> hand them off to the *Channel.
> 
> Review of attachment 641756 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/glue/AsyncChannel.cpp
> @@ +715,5 @@
> > +
> > +    // Dispatch whatever messages the previous listener had queued up.
> > +    while (!pending.empty()) {
> > +        OnMessageReceived(pending.front());
> > +        pending.pop();
> 
> Should you pop before calling OnMessageReceived in case it reenters somehow?

There's no possibility of reentry, but also no reason to use a nonstandard idiom.  Will change.

> >  {
> >    // XXXbent Notify that the child process is gone?
> 
> Should probably file a followup since this can happen too, right?
> 

Sure.  Our wild youth is catching up with us ;).

> ::: ipc/glue/GeckoChildProcessHost.h
> @@ +106,5 @@
> >                                    base::ProcessArchitecture arch);
> > +
> > +  // There's a short window in between we launch the subprocess and we
> > +  // hand the channel off that messages can be received by this, while
> > +  // it's still the channel listener.  If so, the messages would
> 
> Nit: This comment is all sorts of grammatical nightmare. Must have been late
> ;) Let's clean it up a little.

It was both late and a nightmare.

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 641757 [details] [diff] [review]
part 3: SyncLaunch() content processes when we're using direct-to-omtc drawing

Review of attachment 641757 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/ContentParent.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#if defined(ANDROID) || defined(XP_UNIX)
> +# include <sys/time.h>
> +# include <sys/resource.h>

Aha! You're the one who does this crazy indent thing. Ordinarily I'd balk, but you did alphabetize everything... Tough choice.

Comment 26

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #24)
> (In reply to ben turner [:bent] from comment #23)
> > Comment on attachment 641756 [details] [diff] [review]
> > 2: GeckoChildProcessHost can't drop messages on the floor.  Queue them and
> > hand them off to the *Channel.
> > 
> > Review of attachment 641756 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: ipc/glue/AsyncChannel.cpp
> > @@ +715,5 @@
> > > +
> > > +    // Dispatch whatever messages the previous listener had queued up.
> > > +    while (!pending.empty()) {
> > > +        OnMessageReceived(pending.front());
> > > +        pending.pop();
> > 
> > Should you pop before calling OnMessageReceived in case it reenters somehow?
> 
> There's no possibility of reentry, but also no reason to use a nonstandard
> idiom.  Will change.
> 

Oh, now I remember what this was doing.  It's queue<Message> (not Message*), so the impl in the patch avoids a copy when calling OnMessageReceived.  So I didn't change this.

Comment 27

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

Comment on attachment 642049 [details] [diff] [review]
part 6: Allow layer trees to be given IDs so that the referent can be used in another context

Review of attachment 642049 [details] [diff] [review]:
-----------------------------------------------------------------

This could also be considered an r+ if that helps.

Comment 28

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

Comment on attachment 642050 [details] [diff] [review]
part 7: Create a RefLayer type to temporarily contain a foreign layer subtree during composition

Review of attachment 642050 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.h
@@ +1431,5 @@
> +  virtual void InsertAfter(Layer* aChild, Layer* aAfter)
> +  { MOZ_NOT_REACHED("no"); }
> +
> +  virtual void RemoveChild(Layer* aChild)
> +  { MOZ_NOT_REACHED("no"); }

It would be cleaner to not expose RefLayer's ContainerLayer-ness. Private inheritance might work but we don't often use that. This will be much easier to address once layers are split into content-side and compositor-side objects, so let's not worry about it for now.

@@ +1442,5 @@
> +   * Set the ID of the layer's referent.
> +   */
> +  void SetReferentId(int64_t aId)
> +  {
> +    mId = aId;

Assert that aId is >= 0?

::: gfx/layers/ipc/CompositorParent.cpp
@@ +28,5 @@
>  static Thread* sCompositorThread = nsnull;
>  
> +/**
> + */
> +static Layer* GetIndirectShadowTree(int64_t aId);

Remove non-comment

Comment 29

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

Comment on attachment 642054 [details] [diff] [review]
part 9: Hook up the pieces and enable direct compositor

Review of attachment 642054 [details] [diff] [review]:
-----------------------------------------------------------------

I don't understand why we need mIndirectLayerManager. It seems a bit vestigial. Why can't we use some other means to ensure RenderFrameParent creates a RefLayer?

::: layout/ipc/RenderFrameParent.cpp
@@ +552,5 @@
> +
> +    nsRefPtr<RefLayer> layer = aManager->CreateRefLayer();
> +    if (!layer) {
> +      // Probably a temporary layer manager that doesn't know how to
> +      // use ref layers.

Do we actually hit this? We shouldn't, right? Shouldn't this be an NS_ERROR?

Comment 30

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> Comment on attachment 642049 [details] [diff] [review]
> part 6: Allow layer trees to be given IDs so that the referent can be used
> in another context
> 
> Review of attachment 642049 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This could also be considered an r+ if that helps.

Sure!  Thanks.

Comment 31

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> Comment on attachment 642054 [details] [diff] [review]
> part 9: Hook up the pieces and enable direct compositor
> 
> Review of attachment 642054 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand why we need mIndirectLayerManager. It seems a bit
> vestigial. Why can't we use some other means to ensure RenderFrameParent
> creates a RefLayer?
> 

We don't absolutely need it.  I'll reorganize things so that RenderFrameParent doesn't use that layer manager anymore.

> ::: layout/ipc/RenderFrameParent.cpp
> @@ +552,5 @@
> > +
> > +    nsRefPtr<RefLayer> layer = aManager->CreateRefLayer();
> > +    if (!layer) {
> > +      // Probably a temporary layer manager that doesn't know how to
> > +      // use ref layers.
> 
> Do we actually hit this? We shouldn't, right? Shouldn't this be an NS_ERROR?

We do.

Comment 32

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

Pushed 0-2 since those are more generic things we might step on

https://hg.mozilla.org/integration/mozilla-inbound/rev/d84d1d760483
https://hg.mozilla.org/integration/mozilla-inbound/rev/453481dbb828
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3f19e4f90e7

Please leave this bug open.

Comment 33

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #31)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> > Comment on attachment 642054 [details] [diff] [review]
> > part 9: Hook up the pieces and enable direct compositor
> > 
> > Review of attachment 642054 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I don't understand why we need mIndirectLayerManager. It seems a bit
> > vestigial. Why can't we use some other means to ensure RenderFrameParent
> > creates a RefLayer?
> > 
> 
> We don't absolutely need it.  I'll reorganize things so that
> RenderFrameParent doesn't use that layer manager anymore.
> 

Hmm ... I started to change this but it's actually somewhat nontrivial.  How strongly do you feel about changing this for this bug?

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d84d1d760483
https://hg.mozilla.org/mozilla-central/rev/453481dbb828
https://hg.mozilla.org/mozilla-central/rev/d3f19e4f90e7

Comment 35

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

See comment 32.  Thanks for merging!

Comment 36

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #33)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #31)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> > > Comment on attachment 642054 [details] [diff] [review]
> > > part 9: Hook up the pieces and enable direct compositor
> > > 
> > > Review of attachment 642054 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I don't understand why we need mIndirectLayerManager. It seems a bit
> > > vestigial. Why can't we use some other means to ensure RenderFrameParent
> > > creates a RefLayer?
> > > 
> > 
> > We don't absolutely need it.  I'll reorganize things so that
> > RenderFrameParent doesn't use that layer manager anymore.
> > 
> 
> Hmm ... I started to change this but it's actually somewhat nontrivial.  How
> strongly do you feel about changing this for this bug?

Never mind again, I have to do pretty much the same thing for bug 750977, so might as well knock it out here.

Comment 37

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

Attachment: part 9: Hook up the pieces and enable direct compositor, v2

Comment 38

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

With latest inbound and gaia, I'm seeing a ColorLayer being set as the root of the layer tree a content process.  That breaks a lot of things.

Comment 39

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

This doesn't happen on a desktop-b2g build.  Very disturbing.

Comment 40

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

We're hitting

#1  0x406d52c2 in PresShell::Paint (this=0x2187370, aViewToPaint=0x216e5f0, aWidgetToPaint=0x1f1d168, aDirtyRegion=..., aIntDirtyRegion=..., aWillSendDidPaint=true) at /home/cjones/mozilla/inbound/layout/base/nsPresShell.cpp:5293

  nsRefPtr<ColorLayer> root = layerManager->CreateColorLayer();
  if (root) {
    nsPresContext* pc = GetPresContext();
    nsIntRect bounds =
      pc->GetVisibleArea().ToOutsidePixels(pc->AppUnitsPerDevPixel());
    bgcolor = NS_ComposeColors(bgcolor, mCanvasBackgroundColor);
    root->SetColor(bgcolor);
    root->SetVisibleRegion(bounds);
    layerManager->SetRoot(root);

I was under the impression that the root layer must be a container layer, but that's not mentioned anywhere in Layers.h.  OK.

Comment 41

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

Folded this patch

   void TemporarilyCompensateForContentScrollOffset(Layer* aContainer,
                                                    Layer* aShadowContent)
   {
     ContainerLayer* c = aShadowContent->AsContainerLayer();
+    if (!c) {
+      return;
+    }

into part 8.

Comment 42

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #40)
> I was under the impression that the root layer must be a container layer,
> but that's not mentioned anywhere in Layers.h.  OK.

Right, sometimes we paint before we've constructed a root frame.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #31)
> > ::: layout/ipc/RenderFrameParent.cpp
> > @@ +552,5 @@
> > > +
> > > +    nsRefPtr<RefLayer> layer = aManager->CreateRefLayer();
> > > +    if (!layer) {
> > > +      // Probably a temporary layer manager that doesn't know how to
> > > +      // use ref layers.
> > 
> > Do we actually hit this? We shouldn't, right? Shouldn't this be an NS_ERROR?
> 
> We do.

Under what conditions?

Comment 43

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

At least for -moz-element.

Comment 44

[Benoit Girard (:BenWa)]

Comment on attachment 642050 [details] [diff] [review]
part 7: Create a RefLayer type to temporarily contain a foreign layer subtree during composition

Review of attachment 642050 [details] [diff] [review]:
-----------------------------------------------------------------

r- since I'd like to see the clarifications below before it goes in even if they are minor.

::: gfx/layers/Layers.h
@@ +1421,5 @@
> + * Composition:
> + *   look up subtree for GetReferentId()
> + *   ConnectReferentLayer(subtree)
> + *   compose
> + *   ClearReferentLayer()

It's not obvious to me why you want to connect/clear the referent layer for every composite. I think it's worth documenting that here.

@@ +1442,5 @@
> +   * Set the ID of the layer's referent.
> +   */
> +  void SetReferentId(int64_t aId)
> +  {
> +    mId = aId;

Instead of asserting that mId is >= 0 shouldn't we just use uint?

@@ +1462,5 @@
> +  /**
> +   * DRAWING PHASE ONLY
> +   * |aLayer| is the same as the argument to ConnectReferentLayer().
> +   */
> +  void ClearReferentLayer(Layer* aLayer)

Maybe we should rename this to disconnect/detach to better reflect that we're not clearing the referent layer but rather just detaching.

@@ +1491,5 @@
> +
> +  virtual nsACString& PrintInfo(nsACString& aTo, const char* aPrefix);
> +
> +  Layer* mTempReferent;
> +  int64_t mId;

Should we document that '0' is a special value? If for nothing else it's useful for asserting that mId has been set without introducing a debug only flag.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +294,5 @@
> +/**
> + * DRAWING PHASE ONLY
> + *
> + * For reach RefLayer in |aRoot|, look up its referent and connect it
> + * to the layer tree, if found.  On exiting scope, clears all resolved

clear=disconnect?

@@ +313,5 @@
> +
> +private:
> +  enum Op { Resolve, Clear };
> +  template<Op OP>
> +  void WalkTheTree(Layer* aLayer, Layer* aParent)

Why is this a template instead of making Op a parameter to WalkTheTree? As I understand it this will create two instance of this function.

@@ +332,5 @@
> +  }
> +
> +  Layer* mRoot;
> +
> +  AutoResolveRefLayers(const AutoResolveRefLayers&);

MOZ_DELETE

::: gfx/layers/ipc/ShadowLayersParent.cpp
@@ +265,5 @@
>            specific.get_CanvasLayerAttributes().filter());
>          break;
>  
> +      case Specific::TRefLayerAttributes:
> +        MOZ_LAYERS_LOG(("[ParentSide]   canvas layer"));

s/canvas/reference

Comment 45

[Ali Juma [:ajuma]]

Comment on attachment 642053 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor

Review of attachment 642053 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with followups filed for the three "FIXME/bug XXXXX" comments.

Comment 46

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

(In reply to Benoit Girard (:BenWa) from comment #44)

> ::: gfx/layers/Layers.h
> @@ +1421,5 @@
> > + * Composition:
> > + *   look up subtree for GetReferentId()
> > + *   ConnectReferentLayer(subtree)
> > + *   compose
> > + *   ClearReferentLayer()
> 
> It's not obvious to me why you want to connect/clear the referent layer for
> every composite. I think it's worth documenting that here.
> 

 * Clients will usually want to Connect/Clear() on each transaction to
 * avoid difficulties managing memory across multiple layer subtrees.

> @@ +1442,5 @@
> > +   * Set the ID of the layer's referent.
> > +   */
> > +  void SetReferentId(int64_t aId)
> > +  {
> > +    mId = aId;
> 
> Instead of asserting that mId is >= 0 shouldn't we just use uint?
> 

It was supposed to be uint64_t, this is a typo.  But we want to assert that it's != 0, i.e. a valid ID.  Having a RefLayer with the "null ID" is meaningless.

> @@ +1462,5 @@
> > +  /**
> > +   * DRAWING PHASE ONLY
> > +   * |aLayer| is the same as the argument to ConnectReferentLayer().
> > +   */
> > +  void ClearReferentLayer(Layer* aLayer)
> 
> Maybe we should rename this to disconnect/detach to better reflect that
> we're not clearing the referent layer but rather just detaching.
> 

Disconnect() is quite overloaded in gfx/layers.  Switched to Detach*() isn't.  

> @@ +1491,5 @@
> > +
> > +  virtual nsACString& PrintInfo(nsACString& aTo, const char* aPrefix);
> > +
> > +  Layer* mTempReferent;
> > +  int64_t mId;
> 
> Should we document that '0' is a special value? If for nothing else it's
> useful for asserting that mId has been set without introducing a debug only
> flag.
> 

  // 0 is a special value that means "no ID".

> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +294,5 @@
> > +/**
> > + * DRAWING PHASE ONLY
> > + *
> > + * For reach RefLayer in |aRoot|, look up its referent and connect it
> > + * to the layer tree, if found.  On exiting scope, clears all resolved
> 
> clear=disconnect?
> 

s/clear/detach/

> @@ +313,5 @@
> > +
> > +private:
> > +  enum Op { Resolve, Clear };
> > +  template<Op OP>
> > +  void WalkTheTree(Layer* aLayer, Layer* aParent)
> 
> Why is this a template instead of making Op a parameter to WalkTheTree? As I
> understand it this will create two instance of this function.
> 

That's what we want.  Compilers are highly optimized to boil away constant parameters of templates.

> @@ +332,5 @@
> > +  }
> > +
> > +  Layer* mRoot;
> > +
> > +  AutoResolveRefLayers(const AutoResolveRefLayers&);
> 
> MOZ_DELETE
> 

Fixed.

> ::: gfx/layers/ipc/ShadowLayersParent.cpp
> @@ +265,5 @@
> >            specific.get_CanvasLayerAttributes().filter());
> >          break;
> >  
> > +      case Specific::TRefLayerAttributes:
> > +        MOZ_LAYERS_LOG(("[ParentSide]   canvas layer"));
> 
> s/canvas/reference

Fixed.

Comment 47

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

Attachment: part 7: Create a RefLayer type to temporarily contain a foreign layer subtree during composition, v2

Addressed comments, carrying over sr=roc.

Comment 48

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

(In reply to Ali Juma [:ajuma] from comment #45)
> Comment on attachment 642053 [details] [diff] [review]
> part 8: Implement the little boilerplate-y bits and pieces needed for
> cross-process compositor
> 
> Review of attachment 642053 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with followups filed for the three "FIXME/bug XXXXX" comments.

Thanks for the catch!  This slipped off my to-do list.

Comment 49

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

Attachment: part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor, v2

Carrying over r=ajuma.

Comment 50

[Benoit Girard (:BenWa)]

Comment on attachment 642053 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor

Review of attachment 642053 [details] [diff] [review]:
-----------------------------------------------------------------

Posting my review draft I wrote before the patch was updated.

::: gfx/layers/ipc/CompositorChild.cpp
@@ +44,5 @@
> +{
> +  // There's only one compositor per child process.
> +  MOZ_ASSERT(!sCompositor);
> +
> +  nsRefPtr<CompositorChild> cc(new CompositorChild(nsnull));

Not a fan of variable shorthand.

@@ +57,5 @@
> +    NS_RUNTIMEABORT("Couldn't Open() Compositor channel.");
> +    return nsnull;
> +  }
> +  // We release this ref in ActorDestroy().
> +  return sCompositor = cc.forget().get();

Why not make sCompositor a RefPtr?

@@ +78,5 @@
> +CompositorChild::ActorDestroy(ActorDestroyReason aWhy)
> +{
> +  MOZ_ASSERT(sCompositor == this);
> +  sCompositor = NULL;
> +  MessageLoop::current()->PostTask(

Comment to explain why we're posting this as a task would be nice. Is this accurate/
// Release doesn't need to happen synchronously, postpone it.

::: gfx/layers/ipc/CompositorChild.h
@@ +43,4 @@
>  private:
>    nsRefPtr<LayerManager> mLayerManager;
>  
> +  // When we're in a child process, this is the process-global

What about when we're not in a child process? Maybe we can add a getter and assert if this is only valid in the child process?

::: gfx/layers/ipc/CompositorParent.cpp
@@ +28,5 @@
>  namespace mozilla {
>  namespace layers {
>  
> +// FIXME/bug XXXXXX: There can only be one.  No wait, can be *more*
> +// than one.  Fix this.

If you plan on landing this as-is you should clarify the comment.

Comment 51

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

(In reply to Benoit Girard (:BenWa) from comment #50)
> Comment on attachment 642053 [details] [diff] [review]
> part 8: Implement the little boilerplate-y bits and pieces needed for
> cross-process compositor
> 
> ::: gfx/layers/ipc/CompositorChild.cpp
> @@ +44,5 @@
> > +{
> > +  // There's only one compositor per child process.
> > +  MOZ_ASSERT(!sCompositor);
> > +
> > +  nsRefPtr<CompositorChild> cc(new CompositorChild(nsnull));
> 
> Not a fan of variable shorthand.
> 

s/cc/child/

> @@ +57,5 @@
> > +    NS_RUNTIMEABORT("Couldn't Open() Compositor channel.");
> > +    return nsnull;
> > +  }
> > +  // We release this ref in ActorDestroy().
> > +  return sCompositor = cc.forget().get();
> 
> Why not make sCompositor a RefPtr?
> 

It creates static ctors which slow down startup.  We need jlebar's StaticRefPtr to fix this properly.

> @@ +78,5 @@
> > +CompositorChild::ActorDestroy(ActorDestroyReason aWhy)
> > +{
> > +  MOZ_ASSERT(sCompositor == this);
> > +  sCompositor = NULL;
> > +  MessageLoop::current()->PostTask(
> 
> Comment to explain why we're posting this as a task would be nice. Is this
> accurate/
> // Release doesn't need to happen synchronously, postpone it.
> 

  // We don't want to release the ref to sCompositor here, during
  // cleanup, because that will cause it to be deleted while it's
  // still being used.  So defer the deletion to after it's not in
  // use.

> ::: gfx/layers/ipc/CompositorChild.h
> @@ +43,4 @@
> >  private:
> >    nsRefPtr<LayerManager> mLayerManager;
> >  
> > +  // When we're in a child process, this is the process-global
> 
> What about when we're not in a child process? Maybe we can add a getter and
> assert if this is only valid in the child process?
> 

I don't feel very strongly about this, but went ahead and moved the getter into the .cpp and added an assert.

> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +28,5 @@
> >  namespace mozilla {
> >  namespace layers {
> >  
> > +// FIXME/bug XXXXXX: There can only be one.  No wait, can be *more*
> > +// than one.  Fix this.
> 
> If you plan on landing this as-is you should clarify the comment.

Yeah, it was early in the morning

// FIXME/bug 774386: we're assuming that there's only one
// CompositorParent, but that's not always true.  This assumption only
// affects CrossProcessCompositorParent below.

Comment 52

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

Attachment: part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor, v3

Comment 53

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

Comment on attachment 642682 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor, v3

Carrying over r=ajuma.

Comment 54

[Benoit Girard (:BenWa)]

Comment on attachment 642682 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor, v3

Review of attachment 642682 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorParent.cpp
@@ +736,5 @@
> +  static uint64_t ids;
> +  return ++ids;
> +}
> +
> +/** */

Missing a comment here. Maybe how this class differs from PCompositorParent.

@@ +814,2 @@
>  {
> +  LayerTreeMap::const_iterator cit = sIndirectLayerTrees.find(aId);

Quite a few variable acronym in this page. I have no idea what the 'c' stands for here, let's use something more obvious.

Comment 55

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> Comment on attachment 642050 [details] [diff] [review]
> part 7: Create a RefLayer type to temporarily contain a foreign layer
> subtree during composition
> 
> @@ +1442,5 @@
> > +   * Set the ID of the layer's referent.
> > +   */
> > +  void SetReferentId(int64_t aId)
> > +  {
> > +    mId = aId;
> 
> Assert that aId is >= 0?
> 

Done, --> uint64_t and asserted != 0.

> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +28,5 @@
> >  static Thread* sCompositorThread = nsnull;
> >  
> > +/**
> > + */
> > +static Layer* GetIndirectShadowTree(int64_t aId);
> 
> Remove non-comment

I meant to document this ... added

/**
 * Lookup the indirect shadow tree for |aId| and return it if it
 * exists.  Otherwise null is returned.  This must only be called on
 * the compositor thread.
 */

Comment 56

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

(In reply to Benoit Girard (:BenWa) from comment #54)
> Comment on attachment 642682 [details] [diff] [review]
> part 8: Implement the little boilerplate-y bits and pieces needed for
> cross-process compositor, v3
> 
> Review of attachment 642682 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +736,5 @@
> > +  static uint64_t ids;
> > +  return ++ids;
> > +}
> > +
> > +/** */
> 
> Missing a comment here. Maybe how this class differs from PCompositorParent.
> 

/**
 * This class handles layer updates pushed directly from child
 * processes to the compositor thread.  It's associated with a
 * CompositorParent on the compositor thread.  While it uses the
 * PCompositor protocol to manage these updates, it doesn't actually
 * drive compositing itself.  For that it hands off work to the
 * CompositorParent it's associated with.
 */

> @@ +814,2 @@
> >  {
> > +  LayerTreeMap::const_iterator cit = sIndirectLayerTrees.find(aId);
> 
> Quite a few variable acronym in this page. I have no idea what the 'c'
> stands for here, let's use something more obvious.

In this case here, "cit" is an STL idiom that's shorthand for "Const ITerator".

Comment 57

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

Kinda cute: every single try build failed on https://tbpl.mozilla.org/?tree=Try&rev=83b060905e12, but all of my local {mac,android,b2g,linux} builds succeed.  (Even with clobbers.) :|

Comment 58

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

And the rest

https://hg.mozilla.org/integration/mozilla-inbound/rev/71300529a740
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed87a3a43a6c
https://hg.mozilla.org/integration/mozilla-inbound/rev/13059586b02b
https://hg.mozilla.org/integration/mozilla-inbound/rev/1df6cd42330d
https://hg.mozilla.org/integration/mozilla-inbound/rev/100fd0a81f9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/aeafc64692a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a5808967288

Comment 59

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/71300529a740
https://hg.mozilla.org/mozilla-central/rev/ed87a3a43a6c
https://hg.mozilla.org/mozilla-central/rev/13059586b02b
https://hg.mozilla.org/mozilla-central/rev/1df6cd42330d
https://hg.mozilla.org/mozilla-central/rev/100fd0a81f9e
https://hg.mozilla.org/mozilla-central/rev/aeafc64692a5
https://hg.mozilla.org/mozilla-central/rev/4a5808967288