Closed Bug 856079 Opened 7 years ago Closed 7 years ago

merge ShadowLayer and LayerComposite

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: nical, Assigned: nical)

References

Details

Attachments

(1 file, 6 obsolete files)

In order to make rebasing the gfx branch on top of mozilla-central as painless as possible, new layers code went into LayerComposite classes (ImageLayerComposite, etc.) that inherit from ShadowLayer. Once the refactoring lands, we need to remove layer composites and move all their logic into ShadowLayer classes (or vis-versa). This will greatly simplify the code.
Work in progress.
I still need to completely remove *LayerComposite files, move the ShadowLayer classes into their own files and do a bit of cleanup here and there.

I'll finish this stuff as soon as the refactoring lands.
Sorry this comment is a bit late, but I didn't realise you were going to start working on this. I would prefer to keep Composite layers and remove Shadow layers, rather than vice versa. Because, I think we can (long term, not right now) get rid of Shadowable layers too and merge them into basic layers, then we don't have 'Shadow' or 'Shadowable' anywhere, but we still need 'Composite', or at least 'Compositor', 'Compositable', etc.
(In reply to Nick Cameron [:nrc] from comment #2)
> Sorry this comment is a bit late, but I didn't realise you were going to
> start working on this. I would prefer to keep Composite layers and remove
> Shadow layers, rather than vice versa. Because, I think we can (long term,
> not right now) get rid of Shadowable layers too and merge them into basic
> layers, then we don't have 'Shadow' or 'Shadowable' anywhere, but we still
> need 'Composite', or at least 'Compositor', 'Compositable', etc.

Hmm, actually, maybe I don't care that much :-) I'm happy to leave it this way, composite layer, was never that great a name.
I don't mind running a sed script once we settle for the naming convention. I Initially kept "Shadow" over "Composite" because that's the name most people know, and i didn't see a strong reason to change the name, but The only important thing for me here is the few hundred lines of code and the complexity that goes away once the two abstractions are merged.
Duplicate of this bug: 804905
I still need to clean up and check a few little things but most of the work is there.
This patch merges *LayerComposite classes with Shadow*Layer ones (same thing for the Managers) and put the Shadow layer classes in their own files (rather than everything in ShadowLayers.h)

I'll post the final patch for review early tomorrow (nrc, want to review it?).

Other cleanups and renaming will follow in different patches.
Attachment #733322 - Attachment is obsolete: true
(In reply to Nicolas Silva [:nical] from comment #6)
> Created attachment 737588 [details] [diff] [review]
> WIP - merge LayerComposite and ShadowLayer (almost finished)
> 
> I still need to clean up and check a few little things but most of the work
> is there.
> This patch merges *LayerComposite classes with Shadow*Layer ones (same thing
> for the Managers) and put the Shadow layer classes in their own files
> (rather than everything in ShadowLayers.h)
> 

Having thought about this, I would rather avoid moving to ShadowLayers and to remove shadow layers and keep composite - it makes it easier to explain to people the old thing vs the new thing.

> I'll post the final patch for review early tomorrow (nrc, want to review
> it?).

Sure.

> 
> Other cleanups and renaming will follow in different patches.
There is some more renaming to be done but it will come in several followup patches.
Attachment #737588 - Attachment is obsolete: true
Attachment #738087 - Flags: review?(ncameron)
Comment on attachment 738087 [details] [diff] [review]
Remove ShadowLayer classes, LayerComposite becomes LayerHost

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

Maybe controversial, but I would prefer HostLayer to LayerHost, because the latter sounds like an object which hosts a layer, rather than the layer itself. Unfortunately that means we have a different naming convention to compositables and textues, and the other layer types, which is kind of sad.

::: gfx/layers/Layers.h
@@ +165,5 @@
>  
>    virtual ShadowLayerForwarder* AsShadowForwarder()
>    { return nullptr; }
>  
> +  virtual LayerManagerHost* AsShadowManager()

Please rename the method too (also AsShadowForwarder and ShadowLayerForwarder)

@@ +1515,5 @@
>    // be part of mTransform.
>    float mInheritedXScale;
>    float mInheritedYScale;
>    bool mUseIntermediateSurface;
> +

nit: remove extra newline

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +1160,2 @@
>                                          void* aCallbackData,
>                                          EndTransactionFlags aFlags)

indent

@@ +1309,4 @@
>                                                     gfx::Rect& aViewport,
>                                                     float& aScaleX,
>                                                     float& aScaleY,
>                                                     bool aDrawingCritical)

indent

::: gfx/layers/basic/BasicLayers.h
@@ +203,5 @@
>    bool mTransactionIncomplete;
>    bool mCompositorMightResample;
>  };
>  
> +class BasicLayerManagerHost : public BasicLayerManager,

Hmm, so I don't think this name change is right, because despite the name I think BasicShadowLayerManager is actually a shadowable manager, or possiblty both, I don't quite recall. In any case 'Host' is wrong here.

@@ +208,1 @@
>                                  public ShadowLayerForwarder

indent

::: gfx/layers/basic/BasicThebesLayer.cpp
@@ +310,5 @@
>    mContentClient = nullptr;
>    BasicShadowableLayer::Disconnect();
>  }
>  
> +class ThebesLayerHostBuffer : public ThebesLayerBuffer

that doesn't really roll off the tongue! I think it should be ThebesLayerBufferHost, since it is a ThebesLayerBuffer on the host side, not a Buffer for ThebesLayerHosts.

::: gfx/layers/client/TextureClient.h
@@ -25,4 @@
>  class ContentClient;
>  class PlanarYCbCrImage;
>  class Image;
> -class PTextureChild;

LOL! Not one, but two unnecessary declarations. Good catch.

::: gfx/layers/composite/CanvasLayerComposite.cpp
@@ +55,1 @@
>                                    const nsIntRect& aClipRect)

indent

@@ +86,5 @@
>                          clipRect);
>  }
>  
>  CompositableHost*
> +CanvasLayerHost::GetCompositableHost() {

nit: brace on newline

::: gfx/layers/composite/CanvasLayerComposite.h
@@ +11,5 @@
>  namespace mozilla {
>  namespace layers {
>  
> +class CanvasLayerHost : public LayerHost,
> +                          public CanvasLayer

indent

@@ -16,5 @@
>  namespace mozilla {
>  namespace layers {
>  
> -// Canvas layers use ImageHosts (but CanvasClients) because compositing a
> -// canvas is identical to compositing an image.

I think it is worth keeping this comment somewhere

@@ +48,2 @@
>    {
>      NS_ERROR("Should never be called");

can't we get rid of swap? Is it ever used now?

@@ +70,1 @@
>    virtual nsACString& PrintInfo(nsACString& aTo, const char* aPrefix) MOZ_OVERRIDE;

should this be public?

@@ +70,4 @@
>    virtual nsACString& PrintInfo(nsACString& aTo, const char* aPrefix) MOZ_OVERRIDE;
>  #endif
>  
> +  void EnsureImageHost(CompositableType aHostType);

where did this come from? and why?

::: gfx/layers/composite/ColorLayerComposite.cpp
@@ +27,3 @@
>  void
> +ColorLayerHost::RenderLayer(const nsIntPoint& aOffset,
> +                              const nsIntRect& aClipRect)

indent

::: gfx/layers/composite/ColorLayerComposite.h
@@ +10,5 @@
>  namespace mozilla {
>  namespace layers {
>  
> +class ColorLayerHost : public LayerHost,
> +                         public ColorLayer

indent

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +141,1 @@
>                                           effectChain,

indent

@@ +154,5 @@
>  }
>  
> +ContainerLayerHost::ContainerLayerHost(LayerManagerHost* aManager)
> +: LayerHost(aManager)
> +, ContainerLayer(aManager, nullptr)

indent

@@ +319,1 @@
>                                       const nsIntRect& aClipRect)

indent

@@ +330,5 @@
>    }
>  }
>  
> +RefLayerHost::RefLayerHost(LayerManagerHost* aManager)
> +: RefLayer(aManager, nullptr)

indent

@@ +365,1 @@
>                                 const nsIntRect& aClipRect)

indent

::: gfx/layers/composite/ContainerLayerComposite.h
@@ +11,5 @@
>  namespace mozilla {
>  namespace layers {
>  
> +class ContainerLayerHost : public LayerHost,
> +                             public ContainerLayer

indent

@@ +44,2 @@
>  
> +  virtual void ComputeEffectiveTransforms(const gfx3DMatrix& aTransformToSurface)

MOZ_OVERRRIDE? If not, does it have to be virtual?

@@ +46,5 @@
>    {
>      DefaultComputeEffectiveTransforms(aTransformToSurface);
>    }
>  
> +  virtual void CleanupResources();

and the same here

@@ +54,5 @@
>  };
>  
> +
> +class RefLayerHost : public LayerHost,
> +                       public RefLayer

indent

@@ -81,2 @@
>  
> -  void Destroy() MOZ_OVERRIDE;

no destroy?

@@ +81,2 @@
>  
> +  virtual void ComputeEffectiveTransforms(const gfx3DMatrix& aTransformToSurface)

MOZ_OVERRIDE again

@@ +87,5 @@
>    virtual void CleanupResources() MOZ_OVERRIDE;
>  
>    // ref layers don't use a compositable
>    CompositableHost* GetCompositableHost() MOZ_OVERRIDE { return nullptr; }
> +protected:

doesn't look like we need this

::: gfx/layers/composite/ImageHost.h
@@ +6,5 @@
>  #ifndef MOZILLA_GFX_IMAGEHOST_H
>  #define MOZILLA_GFX_IMAGEHOST_H
>  
>  #include "CompositableHost.h"
> +#include "LayerManagerHost.h"

If nothing uses this, I suspect it can be removed, or moved to the cpp maybe

::: gfx/layers/composite/ImageLayerComposite.cpp
@@ +22,5 @@
>  namespace layers {
>  
> +ImageLayerHost::ImageLayerHost(LayerManagerHost* aManager)
> +: LayerHost(aManager)
> +, ImageLayer(aManager, nullptr)

indent

@@ +68,1 @@
>                                   const nsIntRect& aClipRect)

indent

::: gfx/layers/composite/ImageLayerComposite.h
@@ +11,5 @@
>  namespace mozilla {
>  namespace layers {
>  
> +class ImageLayerHost : public LayerHost,
> +                         public ImageLayer

indent

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +174,1 @@
>                                        void* aCallbackData,

indent

@@ +364,1 @@
>                                                        nsIntRegion& aScreenRegion,

indent

@@ +564,2 @@
>  {
> +  if (LayerManagerHost::mDestroyed) {

why do we need the qualification here and in the next few methods?

@@ +636,1 @@
>                                          SurfaceFormat aFormat)

indent

::: gfx/layers/composite/LayerManagerComposite.h
@@ +13,5 @@
> +namespace gl {
> +class GLContext;
> +class TextureImage;
> +} // namespace gl
> +} // namespace mozilla

I don't think we need the comments here, it is kinda obvious

@@ -46,5 @@
> - * CompositableClient to CompositableHost without interacting with the layer.
> - *
> - * mCompositor is stored in ShadowLayerManager.
> - *
> - * Post-landing TODO: merge LayerComposite with ShadowLayer

can we keep some of this comment?

@@ +77,5 @@
>     */
>    bool Initialize();
>  
>    /**
> +   * Sets the clipping region for this layer manager. This is important on 

trailing whitespace

@@ +94,3 @@
>    void UpdateRenderBounds(const nsIntRect& aRect);
>  
> +  void BeginTransaction();

why lose all of these MOZ_OVERRIDEs?

If we override a virtual method, the overriding method should be declared virtual too. If we don't override anything, please remove the virtual-ness.

@@ +133,3 @@
>    {
> +    MOZ_ASSERT(false, "Shouldn't be called for shadow layer manager");
> +    name.AssignLiteral("Shadow");

'Host'?

@@ +201,5 @@
>  
>  private:
> +
> +  bool PlatformDestroySharedSurface(SurfaceDescriptor* aSurface);
> +  RefPtr<Compositor> mCompositor;

Let's sort the ordering of the private members here - methods first then fields, the latter in decreasing size order.

::: gfx/layers/composite/ThebesLayerComposite.cpp
@@ +25,5 @@
>  namespace mozilla {
>  namespace layers {
>  
> +ThebesLayerHost::ThebesLayerHost(LayerManagerHost* aManager)
> +: LayerHost(aManager)

indent

@@ +107,1 @@
>                                    const nsIntRect& aClipRect)

indent

::: gfx/layers/composite/ThebesLayerComposite.h
@@ +12,5 @@
>  namespace layers {
>  
>  /**
>   * Thebes layers use ContentHosts for their compsositable host.
> + * By using different ContentHosts, ThebesLayerHost support tiled and

nit: support_s_

@@ +17,4 @@
>   * non-tiled Thebes layers and single or double buffering.
>   */
> +class ThebesLayerHost : public LayerHost,
> +                          public ThebesLayer

indent

@@ +22,5 @@
>  public:
> +  ThebesLayerHost(LayerManagerHost* aManager);
> +  ~ThebesLayerHost();
> +
> +  virtual void InvalidateRegion(const nsIntRegion& aRegion)

why did SetValidRegion turn into InvalidateRegion here?

@@ +64,5 @@
> +  virtual void
> +  Swap(const ThebesBuffer& aNewFront, const nsIntRegion& aUpdatedRegion,
> +       OptionalThebesBuffer* aNewBack, nsIntRegion* aNewBackValidRegion,
> +       OptionalThebesBuffer* aReadOnlyFront, nsIntRegion* aFrontUpdatedRegion) {
> +    NS_RUNTIMEABORT("should not use layer swap");

Can we just get rid of swap now?

::: gfx/layers/ipc/CompositableTransactionParent.cpp
@@ +55,5 @@
>        CompositableHost* compositable =
>          compositableParent->GetCompositableHost();
>  
>        Layer* layer = compositable ? compositable->GetLayer() : nullptr;
> +      LayerHost* shadowLayer = layer ? layer->AsLayerHost() : nullptr;

var name

::: gfx/layers/ipc/CompositorParent.cpp
@@ +667,5 @@
>      }
>      layerTransform.ScalePost(1.0f/aLayer->GetPostXScale(),
>                               1.0f/aLayer->GetPostYScale(),
>                               1);
> +    LayerHost* shadow = aLayer->AsLayerHost();

please rename var too

@@ +694,5 @@
>  static void
>  SetShadowProperties(Layer* aLayer)
>  {
>    // FIXME: Bug 717688 -- Do these updates in ShadowLayersParent::RecvUpdate.
> +  LayerHost* shadow = aLayer->AsLayerHost();

and here

@@ +1130,5 @@
>      mLayerManager =
> +      new LayerManagerHost(new CompositorOGL(mWidget,
> +                                               mEGLSurfaceSize.width,
> +                                               mEGLSurfaceSize.height,
> +                                               mRenderToEGLSurface));

indent

::: gfx/layers/ipc/ShadowLayerUtilsD3D10.cpp
@@ +82,2 @@
>                                                       const SurfaceDescriptor&,
>                                                       GLenum)

indent

::: gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
@@ +224,2 @@
>                                                       const SurfaceDescriptor& aDescriptor,
>                                                       GLenum aWrapMode)

indent

::: gfx/layers/ipc/ShadowLayerUtilsMac.cpp
@@ +87,2 @@
>                                                       const SurfaceDescriptor&,
>                                                       GLenum)

indent

::: gfx/layers/ipc/ShadowLayerUtilsX11.cpp
@@ +201,2 @@
>                                                       const SurfaceDescriptor&,
>                                                       GLenum)

indent

::: gfx/layers/ipc/ShadowLayers.cpp
@@ +32,5 @@
> +#include "mozilla/layers/ThebesLayerHost.h"
> +#include "mozilla/layers/CanvasLayerHost.h"
> +#include "mozilla/layers/ColorLayerHost.h"
> +#include "mozilla/layers/ContainerLayerHost.h"
> +#include "mozilla/layers/LayerManagerHost.h"

why do you add these?

@@ +635,2 @@
>                                                       const SurfaceDescriptor&,
>                                                       GLenum)

indent

::: gfx/layers/ipc/ShadowLayers.h
@@ +23,5 @@
> +#include "gfxContext.h"
> +#include "gfx3DMatrix.h"
> +#ifdef XP_WIN
> +#include <windows.h>
> +#endif

why do we need to add all these includes?

@@ +66,5 @@
>  class CanvasClient;
>  class ContentClient;
> +class ImageHost;
> +class CanvasHost;
> +class ContentHost;

and these?

@@ +458,5 @@
>   * A ShadowableLayer is a Layer can be shared with a parent context
>   * through a ShadowLayerForwarder.  A ShadowableLayer maps to a
>   * Shadow*Layer in a parent context.
>   *
> + * Note that LayerHosts can themselves be ShadowableLayers.

I don't think this is true any more, is it?

@@ +505,5 @@
> + *
> + * Mostly, layers are updated during the layers transaction. This is done from
> + * CompositableClient to CompositableHost without interacting with the layer.
> + *
> + * The reference to the compositor is stored in LayerManagerHost.

I find the use of 'shadow layers' and 'LayerHost' confusing, please explain it here, or remove 'shadow layers' references

@@ +510,2 @@
>   */
> +class LayerHost

do we need this here, rather than in LayerManagerHost.h? I believe the latter is more approriate

::: gfx/layers/ipc/ShadowLayersParent.cpp
@@ +404,2 @@
>  
> +      LayerHost* compositeLayer = shadow->AsLayer()->AsLayerHost();

change the variable name too

::: gfx/layers/ipc/ShadowLayersParent.h
@@ +9,5 @@
>  #define mozilla_layers_ShadowLayersParent_h
>  
>  #include "mozilla/layers/PLayersParent.h"
>  #include "ShadowLayers.h"
> +#include "LayerManagerHost.h"

not required

::: gfx/layers/opengl/LayerManagerOGL.h
@@ +35,5 @@
> +class ThebesLayerHost;
> +class ContainerLayerHost;
> +class ImageLayerHost;
> +class CanvasLayerHost;
> +class ColorLayerHost;

remove these, they aren't needed

::: widget/cocoa/nsChildView.mm
@@ +1829,5 @@
>  nsChildView::CreateCompositor()
>  {
>    nsBaseWidget::CreateCompositor();
>    if (mCompositorChild) {
> +    ShadowLayerManager *manager =

Shouldn't this be LayerManagerHost?
Attachment #738087 - Flags: review?(ncameron)
(In reply to Nick Cameron [:nrc] from comment #9)
> Comment on attachment 738087 [details] [diff] [review]
> Remove ShadowLayer classes, LayerComposite becomes LayerHost
> 
> Review of attachment 738087 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +70,1 @@
> >    virtual nsACString& PrintInfo(nsACString& aTo, const char* aPrefix) MOZ_OVERRIDE;
> 
> should this be public?

I don't know fro sure, It was protected before.

> > +  void EnsureImageHost(CompositableType aHostType);
> 
> where did this come from? and why?

It comes from CanvasLayerComposite, not sure why, I tried to not change the old code too much to make this patch as simple as possible.
If there is some dead code or redundant api resulting from merging Shadow and Composite layers, I suggest that we fix it in a followup patch.

> @@ -46,5 @@
> > - * CompositableClient to CompositableHost without interacting with the layer.
> > - *
> > - * mCompositor is stored in ShadowLayerManager.
> > - *
> > - * Post-landing TODO: merge LayerComposite with ShadowLayer
> 
> can we keep some of this comment?

This comment went in LayerHost since it is more about LayerHost than the manager.

> why did SetValidRegion turn into InvalidateRegion here?

SetValidRegion still exists. InvalidateRegion used to be overriden by ShadowThebesLayer because it's pure virtual in ThebesLayer.


> @@ +505,5 @@
> > + *
> > + * Mostly, layers are updated during the layers transaction. This is done from
> > + * CompositableClient to CompositableHost without interacting with the layer.
> > + *
> > + * The reference to the compositor is stored in LayerManagerHost.
> 
> I find the use of 'shadow layers' and 'LayerHost' confusing, please explain
> it here, or remove 'shadow layers' references

There is bug 861377 for layers documentation, let's fix this there.

> @@ +510,2 @@
> >   */
> > +class LayerHost
> 
> do we need this here, rather than in LayerManagerHost.h? I believe the
> latter is more approriate

I moved it to it's own HostLayer.h file.
This is not the final patch as I still need to rename LayerHost into HostLayer and apply this to the other layer types (ImageLayerHost, etc).

Apart from that It addresses all the review comments.
Attachment #738087 - Attachment is obsolete: true
(In reply to Nicolas Silva [:nical] from comment #10)
> (In reply to Nick Cameron [:nrc] from comment #9)
> > Comment on attachment 738087 [details] [diff] [review]
> > Remove ShadowLayer classes, LayerComposite becomes LayerHost
> > 
> > Review of attachment 738087 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > @@ +70,1 @@
> > >    virtual nsACString& PrintInfo(nsACString& aTo, const char* aPrefix) MOZ_OVERRIDE;
> > 
> > should this be public?
> 
> I don't know fro sure, It was protected before.
> 

fair enough, leave it that way

> > > +  void EnsureImageHost(CompositableType aHostType);
> > 
> > where did this come from? and why?
> 
> It comes from CanvasLayerComposite, not sure why, I tried to not change the
> old code too much to make this patch as simple as possible.
> If there is some dead code or redundant api resulting from merging Shadow
> and Composite layers, I suggest that we fix it in a followup patch.
> 

It appears as new code in the patch and I didn't see it removed from anywhere, maybe I just missed that.

> > @@ -46,5 @@
> > > - * CompositableClient to CompositableHost without interacting with the layer.
> > > - *
> > > - * mCompositor is stored in ShadowLayerManager.
> > > - *
> > > - * Post-landing TODO: merge LayerComposite with ShadowLayer
> > 
> > can we keep some of this comment?
> 
> This comment went in LayerHost since it is more about LayerHost than the
> manager.
> 
> > why did SetValidRegion turn into InvalidateRegion here?
> 
> SetValidRegion still exists. InvalidateRegion used to be overriden by
> ShadowThebesLayer because it's pure virtual in ThebesLayer.
> 
> 
> > @@ +505,5 @@
> > > + *
> > > + * Mostly, layers are updated during the layers transaction. This is done from
> > > + * CompositableClient to CompositableHost without interacting with the layer.
> > > + *
> > > + * The reference to the compositor is stored in LayerManagerHost.
> > 
> > I find the use of 'shadow layers' and 'LayerHost' confusing, please explain
> > it here, or remove 'shadow layers' references
> 
> There is bug 861377 for layers documentation, let's fix this there.

I think you should fix this now - all you need to do is change 'shadow layers' to 'host layers' or something similar, I don't think this is worth postponing.

> 
> > @@ +510,2 @@
> > >   */
> > > +class LayerHost
> > 
> > do we need this here, rather than in LayerManagerHost.h? I believe the
> > latter is more approriate
> 
> I moved it to it's own HostLayer.h file.

I think (but I'm not 100%) that all the other layer types have their Layer definition in the LayerManager header, if that is true, then I would prefer to follow that convention for host layers too.
Because, huh, it's NOT a ShadowLayerManager and it actually is a ShadowableLayerManager...

Simple patch, I had to do this in order for the ShadowLayer part to not get in the way of the next patch's making (largely sed-jutsu), it's good to take because it solves a unnecessary and big source of confusion in the code.
Attachment #739627 - Flags: review?(ncameron)
This is a complete rewrite of the previous version because it was bit-rotted to a point where rewriting was just easier.

I tried to take into account the reviews of the previous version, so I hope I didn't let anything slip.

This version keeps FooLayerComposite as the current compositor-side layer naming convention.
Attachment #738353 - Attachment is obsolete: true
Attachment #739630 - Flags: review?(ncameron)
Oh, and both are rebased on top of bug 863324 so we probably need to land that first.
Comment on attachment 739627 [details] [diff] [review]
Rename BasicShadowLayerManager into BasicShadowableLayerManager

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

r=me with the indents fixed.

cc: mattwoodrow just to double check this name change is sane and because he is splitting BasicLayerManager and this will interfere.

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +1160,1 @@
>                                          void* aCallbackData,

indent

@@ +1309,1 @@
>                                                     gfx::Rect& aViewport,

indent

::: gfx/layers/basic/BasicLayers.h
@@ +208,1 @@
>                                  public ShadowLayerForwarder

indent
Attachment #739627 - Flags: review?(ncameron)
Attachment #739627 - Flags: review+
Attachment #739627 - Flags: feedback?(matt.woodrow)
Comment on attachment 739630 [details] [diff] [review]
Merge LayerComposite and ShadowLayer

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

::: gfx/layers/CompositorTypes.h
@@ +131,5 @@
>             mTextureFlags == aOther.mTextureFlags;
>    }
>  };
>  
> +enum OpenMode {

Please add a comment stating what this is used for/where it is used

::: gfx/layers/Layers.h
@@ +166,5 @@
>  
>    virtual ShadowLayerForwarder* AsShadowForwarder()
>    { return nullptr; }
>  
> +  virtual LayerManagerComposite* AsManagerComposite()

AsLayerManagerComposite

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +706,5 @@
> +#ifndef MOZ_HAVE_PLATFORM_SPECIFIC_LAYER_BUFFERS
> +
> +/*static*/ already_AddRefed<TextureImage>
> +LayerManagerComposite::OpenDescriptorForDirectTexturing(GLContext*,
> +                                                     const SurfaceDescriptor&,

indent

::: gfx/layers/ipc/LayerTransactionParent.cpp
@@ +134,1 @@
>                                         ShadowLayersManager* aLayersManager,

indent

::: gfx/layers/ipc/LayerTransactionParent.h
@@ +26,1 @@
>  class ShadowLayerParent;

I guess we should do something about ShadowLayerParent at some point, but probably not right now.

::: gfx/layers/ipc/ShadowLayerUtilsD3D10.cpp
@@ +82,1 @@
>                                                       const SurfaceDescriptor&,

indent

::: gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
@@ +225,1 @@
>                                                       const SurfaceDescriptor& aDescriptor,

indent

::: gfx/layers/ipc/ShadowLayerUtilsMac.cpp
@@ +88,1 @@
>                                                       const SurfaceDescriptor&,

indent

::: gfx/layers/ipc/ShadowLayerUtilsX11.cpp
@@ +204,1 @@
>                                                       const SurfaceDescriptor&,

indent

::: gfx/layers/moz.build
@@ +87,5 @@
> +    'ImageLayerComposite.h',
> +    'CanvasLayerComposite.h',
> +    'ColorLayerComposite.h',
> +    'ContainerLayerComposite.h',
> +    'LayerManagerComposite.h',

Please put in alphabetical order

::: widget/cocoa/nsChildView.mm
@@ +2796,5 @@
>    // Force OpenGL to refresh the very first time we draw. This works around a
>    // Mac OS X bug that stops windows updating on OS X when we use OpenGL.
>    LayerManager *layerManager = mGeckoChild->GetLayerManager(nullptr);
>    if (mUsingOMTCompositor && painted && !mDidForceRefreshOpenGL &&
> +      layerManager->AsManagerComposite()) {

AsLayerManagerComposite
Attachment #739630 - Flags: review?(ncameron) → review+
Comment on attachment 739627 [details] [diff] [review]
Rename BasicShadowLayerManager into BasicShadowableLayerManager

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

I'd prefer if you could just skip this for now, since I'm going to remove all this code and don't want to deal with the merge :)
(In reply to Matt Woodrow (:mattwoodrow) from comment #18)
> Comment on attachment 739627 [details] [diff] [review]
> Rename BasicShadowLayerManager into BasicShadowableLayerManager
> 
> Review of attachment 739627 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd prefer if you could just skip this for now, since I'm going to remove
> all this code and don't want to deal with the merge :)

sure, removing it is much better than fixing it's name.
here is a rebased version with the review comments addressed.
Attachment #739627 - Attachment is obsolete: true
Attachment #739630 - Attachment is obsolete: true
Attachment #739627 - Flags: feedback?(matt.woodrow)
Backed out for random test crashing.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9219b3b1edda

This might be a needs-clobber issue, but we couldn't hold the tree closed waiting on a Try link showing that what landed was green there earlier.

Logs:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4b7de1f2b4a0
Turned out that for my previous landing attempt I had missed a line when rebasing.

Pushed again https://hg.mozilla.org/integration/mozilla-inbound/rev/16ebd3f796c1

after try https://tbpl.mozilla.org/?tree=Try&rev=43962b33583f
https://hg.mozilla.org/mozilla-central/rev/16ebd3f796c1
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.